feat(web-ui): resizable columns + reset-to-defaults for all data tables - #648
Conversation
Completes amule-org#361 -- show/hide columns and localStorage persistence already shipped in amule-org#584, but manual column resizing and a way back to defaults were still missing. Implemented once in the shared table.js (useTablePrefs + VirtualTable + ColumnPicker), so every table built on it -- Downloads, Shared, Search, Servers, Clients -- gets both for free; each view only needed a few lines to thread the new widths/setWidth/resetPrefs through. - useTablePrefs: adds a `widths` map (column key -> px) to the persisted per-table prefs object, plus setWidth() and resetPrefs() (clears sort/hidden/widths back to the view's declared defaults rather than merging them back in, so a stale key from a since-removed column doesn't linger in localStorage). - VirtualTable: a drag handle on each resizable <th> mutates the <col> width directly on mousemove (bypassing state/re-render for a smooth drag) and only commits to onResize on mouseup. The one flexible/no-declared-width column (typically "name") is resizable too -- it starts the drag from its actual rendered width rather than a declared one, and becomes an ordinary fixed-width column once the user drags it. - ColumnPicker: optional onReset prop adds a "Reset columns" action below a separator, kept visually apart from the checkboxes above so a one-shot destructive action doesn't read as another toggle in the same group. - app.css: a header-only border-right marks column boundaries -- the body stays borderless/clean, but without it the resize handles have nothing marking where they live, so dragging felt like guesswork. Manually verified end-to-end (drag-resize, reload persistence, reset, show/hide regression check) across multiple tables, since this is frontend interaction code a unit test wouldn't meaningfully cover.
There was a problem hiding this comment.
Really cleanly factored — doing it once in table.js and threading a few props through each view keeps the per-view diff almost mechanical, and drag-mutates-<col>-directly / commit-once-on-mouseup is the right performance call. A few things I'd like addressed before merge:
1. A stray click on the handle commits a width — and the flexible column loses its flex. startResize initializes finalWidth = startWidth and onUp always calls onResize, so a mousedown+mouseup on the handle with no drag persists the current width. For fixed columns that's a harmless dirty-write, but for the flexible "name" column it converts it to a fixed width on a mis-click (it stops absorbing space until a reset). Guard the commit on an actual change:
const onUp = () => {
window.removeEventListener("mousemove", onMove);
window.removeEventListener("mouseup", onUp);
if (finalWidth !== startWidth) onResize(key, Math.round(finalWidth));
};2. Resize is mouse-only, but the CSS opts into touch. .col-resize-handle sets touch-action: none, yet startResize binds only mousedown/mousemove/mouseup, so it doesn't work on touch devices. Switching to Pointer Events covers mouse + touch in one path and, via setPointerCapture, also fixes # 3:
const startResize = (e, key) => {
if (!onResize) return;
e.preventDefault(); e.stopPropagation();
const col = colRefs.current[key];
if (!col) return;
const handle = e.currentTarget;
const startX = e.clientX;
const startWidth = col.getBoundingClientRect().width;
let finalWidth = startWidth;
handle.setPointerCapture(e.pointerId);
const onMove = (ev) => {
finalWidth = Math.max(MIN_COL_WIDTH, startWidth + (ev.clientX - startX));
col.style.width = finalWidth + "px";
};
const onUp = () => {
handle.removeEventListener("pointermove", onMove);
handle.removeEventListener("pointerup", onUp);
if (finalWidth !== startWidth) onResize(key, Math.round(finalWidth));
};
handle.addEventListener("pointermove", onMove);
handle.addEventListener("pointerup", onUp);
};…and onPointerDown instead of onMouseDown on the handle. With pointer capture the listeners live on the handle element, so this also removes the window-listener path entirely.
3. Listener leak if the view unmounts mid-drag. Today the window mousemove/mouseup listeners are only removed in onUp; if the component unmounts before mouseup they persist and fire onResize on an unmounted component. The pointer-capture change in # 2 resolves this; otherwise a useEffect cleanup that tears down an in-flight drag would do.
Minor / optional:
- The handle relies on
<th>being a positioned containing block — it works becausethisposition: sticky, but aposition: relativeon the resizablethwould harden it against a future change to the sticky header. - The handle isn't keyboard-accessible (a
<span>with a pointer handler) — not a blocker, just flagging for a later a11y pass.
Everything else looks good — type="button" inside the servers <form>, resetPrefs wiping the stored object to avoid stale keys, and the optional onReset keeping old callers working are all the right calls.
Mutating the <col> element's width directly on pointermove bypassed Preact, so a re-render triggered mid-drag by an unrelated prop update (the SSE stream ticks rows every second or so) reconciled the <colgroup> from the last committed width and stomped the in-progress drag back to it -- the longer the drag, the more likely a tick landed inside it, so the column border (and the cursor tracking it) visibly snapped backward while the pointer kept moving. Route the live drag width through component state instead, so every render -- including one forced by an unrelated prop change -- reflects it. Also compute the flexible column's own width from state consistently via colWidth() rather than a separate effWidth()-based reduction, and drop the now-unused colRefs ref. Addresses the position:relative / sticky note from got3nks's review on amule-org#648 with a comment: sticky already establishes a containing block for the resize handle, and position can't be both relative and sticky on the same element, so no rule change was needed.
|
Thanks for the thorough review! All three blocking items were already in place from the earlier revert-guard/Pointer-Events pass:
While testing this by hand I found a related regression the review didn't flag: on a long drag, the column border (and the cursor tracking it) would visibly snap backward mid-drag. Root cause was that On the optional |
got3nks
left a comment
got3nks
left a comment
There was a problem hiding this comment.
All three review items are addressed in 34ca796 — Pointer Events + setPointerCapture, the commit-only-on-actual-change guard, and the mid-drag-unmount leak folded away by capturing on the handle. Nice catch on the SSE-tick snap-back as well: routing the live width through dragging state and giving every <col> an explicit width via colWidth() is the right fix. And you're correct on the position note — th is already position: sticky, which is the containing block, so my suggestion was off. LGTM.
Mechanical rebase to resolve the po/ catalog conflicts against master's amule-org#648/amule-org#650/amule-org#651 (per got3nks's note on amule-org#643) -- no source changes here, scripts/update-po.sh output only, so the diff is just the tree back in sync with the current strings.
|
@LSalami good contribution. thank you |
Mechanical rebase to resolve the po/ conflicts against master's amule-org#643/amule-org#646/amule-org#648/amule-org#650/amule-org#651 -- no source changes here. Confirms got3nks's prediction on the Alt+<letter>-outside-_() fix: the only genuinely new msgid in the diff is "Navigate" (the new macOS menu itself); every other hunk is line-number-comment churn from the rebase, not orphaned or duplicated translations.
Mechanical rebase to resolve the po/ catalog conflicts against master's amule-org#648/amule-org#650/amule-org#651 (per got3nks's note on amule-org#643) -- no source changes here, scripts/update-po.sh output only, so the diff is just the tree back in sync with the current strings.
* feat(search): persist search history across restarts (#641) The search field was a plain text control with no memory of past searches. Swap it for a wxComboBox (a wxTextEntry, like wxTextCtrl, so the existing GetValue()/Clear() call sites keep working via that shared base) and persist submitted terms to wxConfig, most-recent first, capped at 20 entries and deduplicated case-insensitively. Right-clicking the field opens a small menu ("Remember search history" checkbox + "Clear search history") so history can be paused or wiped without a dedicated Preferences page. i18n: ran scripts/update-po.sh to register the two new menu strings. * style(search): separate the destructive Clear item from the toggle above A separator between the checkbox and "Clear search history" keeps the one-shot destructive action visually distinct from the persistent on/off state, instead of reading as a second toggle in the same group. * style(search): use nullptr instead of NULL for the new combo box Fixes the modernize-use-nullptr clang-tidy finding on the changed line; matches the nullptr convention already used a couple of wxComboBox constructions below in the same file. * feat(search): rework query history per #643 review — file, prefs toggle, edit menu Addresses all four blocking points from got3nks's review, plus both optional suggestions: 1. Edit context menu restored. Overriding the field's context menu no longer drops Cut/Copy/Paste/Select All -- same custom-Paste-ID idiom as CMuleTextCtrl::OnRightDown (wxMenu over-permits wxID_PASTE, so it gets a manual clipboard-content check), plus a separator, plus the history's own Clear action. 2. Persisted to a dedicated searchhistory.dat (via CTextFile, one term per line) instead of amule.conf -- mirrors eMule's AC_SearchStrings.dat. Query terms no longer bloat the main config or travel with config backups / the --amule-config-file push to amuleweb/amuleapi. 3. "Remember search history" moved into Preferences > General as an ordinary Cfg_Bool checkbox (IDC_SEARCHHISTORYENABLED), same NewCfgItem wiring as every other GUI-behavior toggle on that page. It's a client-side-only setting, so the one checkbox works unchanged in both amule and amuleGUI. The context menu keeps only Clear (a one-shot action, not a setting to toggle from a hidden menu). 4. Cap raised from 20 to 30, matching eMule's CCustomAutoComplete default. Optional, done anyway: - wxComboBox::AutoComplete() wired up (eMule's ACO_AUTOSUGGEST equivalent), re-armed after every load/record/clear so it always reflects the current entry set. - Explicitly framed as *query* history throughout (comments, tooltip) to distinguish it from the separate, not-yet-implemented result persistence that's the rest of #641. The dedup/move-to-front/cap logic that used to live inline in CSearchDlg::RecordSearchHistory is extracted into a pure function, ApplySearchHistoryEntry() (SearchHistory.h/.cpp), decoupled from wxComboBox/wxConfig specifically so it's unit-testable without a wx event loop -- new SearchHistoryTest covers empty-list insert, reorder of an existing term, case-insensitive dedup, empty-term no-op, and capping (including locking in the actual 30-entry constant). Rebased onto current master; po/ catalogs regenerated as the final step so the diff is just the new/changed strings in sync with the tree. Verified for real: built and ran both amule and amulegui, plus the full unit test suite (27/27 passing, including the new SearchHistoryTest's 7 cases). * fix(tests): link Format.cpp + strerror_r.c into SearchHistoryTest Same Linux/mingw-only link failure as CMuleCollectionTest/MagnetURITest already work around: muleunit's MuleDebug.cpp needs CFormat for its glibc backtrace path, which isn't compiled in on macOS -- so the target linked fine locally but failed on the Ubuntu/mingw CI builds (and the clang-tidy jobs, which build the tree first). * i18n: regenerate po/ catalogs after rebasing onto current master Mechanical rebase to resolve the po/ catalog conflicts against master's #648/#650/#651 (per got3nks's note on #643) -- no source changes here, scripts/update-po.sh output only, so the diff is just the tree back in sync with the current strings. * fix(search): avoid an unnamed EReadTextFile bitmask cast in history load txtIgnoreEmptyLines|txtStripWhitespace has no single named enumerator to cast to -- clang-tidy Tier-1 flagged it (clang-analyzer-optin.core. EnumCastOutOfRange), correctly: EReadTextFile isn't a flag enum, so a synthesized OR'd value is genuinely out of its declared range even though CTextFile::ReadLines treats it as bitflags at runtime. txtReadDefault would dodge the cast but also drops '#'-led lines, silently eating a legitimate search term that happens to start with one. Read unfiltered (txtReadAll, a real enumerator) and do the trim/empty-line-drop by hand instead -- same behavior, no cast, no lost terms. * i18n: regenerate po/ catalogs after rebasing onto current master Mechanical rebase to resolve the po/ conflicts against master's #642 (just merged) -- no source changes here.
3 participants
Summary
Completes #361 — show/hide columns and localStorage persistence already
shipped in #584, but manual column resizing and a way back to defaults
were still missing.
Implemented once in the shared
table.js(useTablePrefs+VirtualTableColumnPicker), so every table built on it — Downloads, Shared, Search,Servers, Clients — gets both for free; each view only needed a few lines
to thread the new
widths/setWidth/resetPrefsthrough.Changes
useTablePrefs: adds awidthsmap (column key → px) to thepersisted per-table prefs object, plus
setWidth()andresetPrefs()(clears sort/hidden/widths back to the view's declared defaults rather
than merging them back in, so a stale key from a since-removed column
doesn't linger in localStorage).
VirtualTable: a drag handle on each resizable<th>mutates the<col>width directly on mousemove (bypassing state/re-render for asmooth drag) and only commits via
onResizeon mouseup. The oneflexible/no-declared-width column (typically "name") is resizable too —
it starts the drag from its actual rendered width rather than a
declared one, and becomes an ordinary fixed-width column once dragged.
ColumnPicker: optionalonResetprop adds a "Reset columns"action below a separator, kept visually apart from the checkboxes
above so a one-shot destructive action doesn't read as another toggle
in the same group.
app.css: a header-onlyborder-rightmarks column boundaries —the body stays borderless/clean, but without it the resize handles had
nothing marking where they live, making dragging feel like guesswork
(caught during manual testing).
Test plan
node --checkon all modified JS filesamuleapiinstance: drag-resize on multiple tables (Servers,Downloads), including the flexible "name" column
unaffected
testing surfaced that resize handles were undiscoverable without it)
This is frontend interaction code a unit test wouldn't meaningfully
cover, so verification was manual/visual rather than automated.