Skip to content

Add Toolbar customization - #24309

Open
FTA7700 wants to merge 25 commits into
qbittorrent:masterfrom
FTA7700:feature/toolbar-customization
Open

Add Toolbar customization#24309
FTA7700 wants to merge 25 commits into
qbittorrent:masterfrom
FTA7700:feature/toolbar-customization

Conversation

@FTA7700

@FTA7700 FTA7700 commented May 19, 2026

Copy link
Copy Markdown
Contributor

Introduction

This PR adds full toolbar customization to qBittorrent. Users can now show/hide individual buttons, rearrange toolbar buttons by dragging, add/remove separators, and lock the toolbar to prevent accidental changes. All customization state is persisted immediately on every change and restored on startup.

This addresses long-standing feature requests - #263 #1145 #5783 #16234 #19289 #20555 #22900 and many more, for that or similar functions concerning custom layouts in the toolbar area . Also discussed in PR #24090 and that motivated this feature PR.



Major new functionalities added (further explained later):

  • Toolbar buttons right-click context menu
  • Context-aware add/remove separator function
  • Show/Hide checkbox menu (additional buttons can be added in future)
  • Reset to Default option
  • Button Lock checkbox
{3845FC3B-EE0C-47AA-BD18-973C16E0A02C} {62087366-1C86-4F89-8F5A-20B558B0F1ED}



Animation



User-facing changes

Drag to reorder (when toolbar is unlocked)
Click and hold any toolbar button, then drag it left or right. Other buttons shuffle aside one at a time as the dragged button crosses their centre — the same interaction model used by browser tab bars and mobile home screens. Release to drop the button in its new position.

Show/Hide buttons
Right-click any toolbar button → Show/Hide Buttons. A submenu lists all customizable buttons with checkboxes. Toggling a checkbox immediately shows or hides that button. Reset to Default restores the default button set, order, and separators.

Add/Remove separators
Right-click any toolbar button → Add separator before / Remove separator before (and the equivalent for after). The menu is context-aware: it only shows the option that applies to the current button's actual neighbours, accounting for hidden actions that may sit between visible ones.

Lock Toolbar
Right-click any toolbar button → Lock Toolbar. When locked (the default), drag reordering is disabled. The right-click menu for separators and show/hide remains available regardless of lock state. Lock state is persisted.

Locked zone (Intentionally left out of scope)
The spacer, column filter widget, and lock button are permanently fixed on the right side of the toolbar. They cannot be dragged, hidden, or have separators added around them. This is enforced both visually (the drag float cannot enter the locked zone) and structurally (they are registered via lockAction() and excluded from all customization logic). Though, that can be changed, depending on the team and community views.



Implementation

1. New class: CustomizableToolBar (subclasses QToolBar)

QToolBar's child buttons are QToolButton widgets that consume mouse events before the toolbar sees them. There is no signal or virtual method on QToolBar that fires when a child button is right-clicked. The only reliable interception point is actionEvent(), which fires whenever an action is added or removed from the toolbar — this is used to install an event filter on each new button widget. A subclass is the natural home for this override, keeping the logic self-contained and out of MainWindow.

CustomizableToolBar is intentionally minimal. It installs event filters on child button widgets via actionEvent, and maintains a list of locked actions and a locked state flag. All context menu logic remains in MainWindow::toolbarMenuRequested(), consistent with how every other context menu in the application is built.

CustomizableToolBar handles all drag logic internally.

2. Context menu

Right-clicking a toolbar button shows a context-aware menu built dynamically on each invocation. Separator options scan backward and forward through the action list, skipping hidden non-separator actions, to find the actual nearest separator — this correctly handles queue actions that are hidden when queuing is disabled but remain in the action list:

Add separator before      ← or "Remove separator before" if one exists
Add separator after       ← or "Remove separator after" if one exists
─────────────────────────
Show/Hide Buttons    ▶   Reset to Default
                         ─────────────────
                         ✓ Add Torrent File
                         ✓ Add Torrent Link
                         ✓ Remove
                         ...
─────────────────────────
✓ Lock Toolbar
  • Right-clicking the empty toolbar area still shows the existing style menu (Icons Only, Text Only, etc.), unchanged.

Show/Hide buttons. Hiding a toolbar button uses removeAction/insertAction on the toolbar rather than QAction::setVisible(false). This is necessary because setVisible(false) affects every widget the action is attached to — toolbar, menu bar, and context menus all at once, disabling the hotkey along with it. With removeAction, the action disappears only from the toolbar while remaining fully functional in the menu bar.
The Show/Hide submenu is built from a master list m_allToolbarActions, populated once at startup before loadSettings() runs. This is necessary because hidden buttons are absent from toolBar->actions() and would otherwise not appear in the submenu at all.
When a button is hidden, its current slot index is saved to m_hiddenToolbarActions. Re-showing it reinserts it at that saved index. If other buttons were reordered while it was hidden, the saved index now has different neighbours — this is a known ambiguity with no universally correct solution. Saving by neighbour name is an equally arbitrary alternative. The team can decide on a preferred strategy.

Separator detection skips hidden actions. When the queuing system is disabled, the queue-related actions (actionTopQueuePos, actionIncreaseQueuePos, etc.) are hidden but remain in the action list. A naive check of acts[idx - 1]->isSeparator() would return false even though visually the button is adjacent to a separator. The fix scans backward and forward through the action list, skipping hidden non-separator actions, to find the nearest visible separator. The remove lambdas operate on the actual separator found by the scan, not a hardcoded offset.

Why lockAction() calls are not in addToolbarContextMenu(): addToolbarContextMenu() is called early in the MainWindow constructor, before m_spacerAction is assigned. The spacer widget is created later in the constructor after the transfer list and filter widgets are set up. Calling lockAction(m_spacerAction) inside addToolbarContextMenu() would register a null pointer and the spacer would never be recognized as locked. The three lockAction() calls are placed immediately after m_spacerAction is assigned in the constructor.

3. Drag implementation

Why not QDrag: QDrag propagates drop events to any widget under the cursor. In testing, releasing a drag over the torrent list triggered unintended actions (e.g. opening Torrent Creator). The custom mouse-event approach confines all drag interaction to the toolbar.

Press and threshold: On MouseButtonPress, the action under the cursor is identified. Dragging only begins after the cursor moves more than QApplication::startDragDistance() (4px), preventing accidental drags on normal clicks.

Float label: When drag starts, QWidget::grab() snapshots the button before it is made transparent. A frameless top-level QLabel is created as the drag float. It has Qt::WA_TransparentForMouseEvents (so it does not steal mouse events) and Qt::WA_ShowWithoutActivating (so it does not steal focus). It follows the cursor globally and is clamped to the toolbar's draggable zone.

Opacity instead of hide: Rather than calling setVisible(false) on the dragged button — which would cause Qt's toolbar layout to immediately reflow all other buttons — a QGraphicsOpacityEffect with opacity 0.0 is applied. The button remains in the layout, holds its space, and other buttons do not move.

Timer-based cursor polling: Mouse move events from eventFilter are unreliable during drag because the float label sits above the toolbar and the cursor may leave the toolbar area entirely. A QTimer at 16ms intervals polls QCursor::pos() and QApplication::mouseButtons(). This also catches mouse button releases that occur outside the toolbar.

Live shuffle: As the float moves, updateDrag() finds the action whose centre the float has crossed. When the target slot changes, one atomic removeAction()/insertAction() is performed. The opacity effect is removed before the swap and reapplied to the new widget after, masking the reflow. Only the single neighbour at the crossing point moves:

Before drag:  [A] [B] [C] [D]
Drag C left, float crosses B's centre:
              [A] [_] [B] [D]   ← B slides right, gap opens under float
Drop:         [A] [C] [B] [D]

Boundary enforcement: findBoundaryIndex() scans actions() for the first locked action (the spacer). The float's right edge is clamped to the spacer's left edge. The drop logic never inserts before a locked action.

Drop and persistence: On release, the action is already in its correct position from the live shuffle. The opacity effect is removed, the float is destroyed, and actionOrderChanged() is emitted. This signal is connected to MainWindow::saveToolbarState(), which persists the new order immediately.

Edge separator handling: In case there are separators at the leftmost or rightmost position of the toolbar, now they are fully reachable. The drag float's clamp is extended slightly past the spacer boundary to allow the float to cross the last separator, and separator hit detection uses directional logic — the float's left edge when moving left, right edge when moving right — so separators respond immediately on contact rather than requiring full overlap.

Drag feel refinements: Two edge cases in the direction-detection logic have been corrected. When a button is dragged to the toolbar's leftmost position, the float clamps against the wall and briefly stops moving — this used to be misread as a direction change, causing a visible bounce-back before the button settled. Separately, the very first frame of any new drag previously defaulted to assuming rightward motion regardless of actual cursor direction, which made separator crossings feel one frame slower when dragging left compared to right. Both cases are now detected correctly from the first frame of the drag.

4. Persistence

Toolbar state is saved under MainWindow/toolbarState as a comma-separated string:

actionOpen:1,actionDownloadFromURL:1,actionDelete:1,separator,actionStart:1,actionStop:1,separator,actionOpenDestinationFolder:1,separator,actionCreateTorrent:1,actionOptions:1

saveToolbarState() calls Preferences::instance()->apply() explicitly to flush to disk immediately, since SettingsStorage batches writes and would otherwise only flush on clean exit.

On startup, loadSettings() builds the action map from a snapshot taken before any removals, then removes and reinserts non-locked actions in saved order before the spacer anchor. The locked zone is never touched during this process.

QMainWindow::saveState() was not used because it handles toolbar docking position and visibility as a whole unit, but does not save individual action order, individual action visibility, or separator positions.

5. Queue actions (Top/Move Up/Move Down/Bottom of Queue)

Queue actions are handled as a special case within the show/hide system, because their visibility is also controlled by the torrent queuing setting in preferences.

They are always present in the Show/Hide submenu, separated from the regular buttons at the bottom of the list
When torrent queuing is disabled, they appear grayed out in the submenu with no checkbox — indicating they are controlled by the queuing setting, not by toolbar customization
When torrent queuing is enabled, they appear at the end of the toolbar as a group with a separator before them (added dynamically, only if one isn't already there), and can be individually shown or hidden via the submenu like any other button
Reset to Default hides them and resets their position; if queuing is on they are immediately restored to the end of the toolbar.

{8AFFE8ED-2171-4AB9-8671-D2B31C3B8FCE}

6. Reset to Default

  • Reset builds the action map before removing anything, then removes all non-locked actions and separators and reinserts them in the hardcoded default order using reverse iteration with a moving anchor. This produces the correct left-to-right order without needing to reverse the list.
  • The default button order and separators ship unchanged from the current mainwindow.ui. Existing users who have never customized their toolbar will see no difference. The lock is on by default, matching current behaviour exactly.
  • Queue actions are hidden on reset if torrent queuing is disabled. If enabled loadPreferences() immediately restores them to the end of the toolbar as a group with a separator before them.
{EAA55499-8074-4530-806A-0E3785A1563D}




Files changed

File Change
src/gui/customizabletoolbar.h NEW
src/gui/customizabletoolbar.cpp NEW
src/gui/mainwindow.h Added m_spacerAction, m_allToolbarActions, m_hiddenToolbarActions, m_queueActionsShown, saveToolbarState()
src/gui/mainwindow.cpp Context menu, state save/restore, lock registration
src/gui/mainwindow.ui Toolbar class changed to CustomizableToolBar, added customwidgets declaration
src/gui/CMakeLists.txt Added new source files, added CMAKE_CURRENT_SOURCE_DIR to include path
src/base/preferences.h Added toolbar state and lock preference declarations
src/base/preferences.cpp Implemented toolbar state and lock preferences

Ready for questions and discussions!

@FTA7700 FTA7700 changed the title Add toolbar customization: separator, show/hide buttons, lock Full Toolbar Customization: Reorder/Add/Remove buttons, separators May 19, 2026
@FTA7700
FTA7700 marked this pull request as ready for review May 20, 2026 21:29
@thalieht thalieht added the GUI GUI-related issues/changes label May 21, 2026
@thalieht

Copy link
Copy Markdown
Contributor

Drag to reorder (when toolbar is unlocked)

Doesn't work for me. That's the only problem i can find.

Well there's also the case when you have added separators between the queue buttons and you disable the queueing system. It leaves some sequential separators (one after another). It wouldn't be a problem except that it doesn't seem possible to do that manually.
Personally i don't think it's something worth fixing but i thought i should mention it.

@FTA7700

FTA7700 commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

That's a major problem, basically that's the core feature here. I'll investigate. What OS you tested on?

P.S. I figured the culprit. I worked that on two parts, the reorder part was the latest and accidentally pushed only the first stage. Now should be good.

@thalieht

Copy link
Copy Markdown
Contributor

One last detail from me.
I find it limiting that i can't drag an icon before or after a separator if the separator is at the beginning or end.

@FTA7700

FTA7700 commented May 23, 2026

Copy link
Copy Markdown
Contributor Author

Fixed, the issue was two-fold:

Right-most separator — the float's clamp was stopping it before it could reach the last separator's centre, so the swap condition never triggered. The clamp is now extended slightly past the spacer boundary to give the float enough room to cross the last separator.
Left-most separator — separator hit detection now uses directional logic: the float's left edge as the trigger when moving left, and centre when moving right. This makes separators respond immediately on contact rather than requiring the float to fully overlap them.

Both edge cases are now fully reachable. The PR description is updated.

Animation

@thalieht

Copy link
Copy Markdown
Contributor

Before the last commit:
After an icon is visually detached from it's position when dragging, it took 7 movements in each direction to swap places with a separator (the smallest visual movement of the icon while dragging it on my machine).

After the last commit:
It takes 9 movements to the right and to the left it is instantly swapped on visual detachment of the icon (the ideal behavior IMO).

@FTA7700

FTA7700 commented May 23, 2026

Copy link
Copy Markdown
Contributor Author

Try it now. Now movement feels even more natural, I think.

thalieht
thalieht previously approved these changes May 24, 2026
@stalkerok

Copy link
Copy Markdown
Member
2026-05-25.19-20-19.mp4
2026-05-25.19-26-52.mp4

@FTA7700

FTA7700 commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

2026-05-25.19-20-19.mp4
2026-05-25.19-26-52.mp4

Thanks for flagging this. I wasn't aware those buttons were already controlled by the queuing setting (never used them) — I only knew them as actions in the code, which is why they ended up in the Show/Hide submenu without special handling.

This is now addressed: Queue actions are always present in the Show/Hide submenu at the bottom of the list, separated from the regular buttons. When torrent queuing is disabled, they appear grayed out with no checkbox — indicating they are controlled by the queuing setting, not by toolbar customization. When queuing is enabled, they appear at the end of the toolbar as a group with a separator before them (added dynamically, only if one isn't already there), and can be individually shown or hidden via the submenu. Reset to Default also respects the queuing state.

Check if you like it that way.

@thalieht

Copy link
Copy Markdown
Contributor
  • Hiding buttons also removes their corresponding entry from the menu bar.
  • Enabling the queueing system always appends the queue control buttons at the end of the buttons list even if they were previously moved elsewhere, disabled the queue system and re-enabled it.

@FTA7700

FTA7700 commented May 26, 2026

Copy link
Copy Markdown
Contributor Author
  • Hiding buttons also removes their corresponding entry from the menu bar.

Which buttons we're talking about? Can you show me?

  • Enabling the queueing system always appends the queue control buttons at the end of the buttons list even if they were previously moved elsewhere, disabled the queue system and re-enabled it.

So, you prefer them to be dynamically shown, depending on where they were

The current behavior repositions them to the end every time queuing is re-enabled. The desired behavior would be:

  • repositioning to the end only happens on first show (no saved state) or after Reset to Default.
  • After that, their position is saved and respected — re-enabling queuing just makes them visible in place wherever they are, not moves them.

Would that work for you?

@glassez

glassez commented May 26, 2026

Copy link
Copy Markdown
Member
  • After that, their position is saved and respected — re-enabling queuing just makes them visible in place wherever they are, not moves them.

How about the case when you put them in some place, hide, reorder/hide/show other buttons and then show them again? What is their position supposed to be?

@FTA7700

FTA7700 commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

Their position in the action list is preserved while hidden — showing them again makes them visible exactly where they are in the list, regardless of what happened around them. For example: user moves queue buttons between Start and Stop, then hides them, then reorders other buttons. When shown again, queue buttons reappear between whatever actions now occupy those neighbouring slots — not necessarily Start and Stop anymore if those moved, but at the same index in the list. Same behavior as any other hidden button in the system; hiding never moves them, showing never repositions them.

@glassez

glassez commented May 26, 2026

Copy link
Copy Markdown
Member

@FTA7700
How exactly are the button positions saved? Are they always absolute?

  1. What if button3 is hidden, and then you move button5 to position 3 (among the visible ones), so that the following order is obtained: button1, button2, button5, button4, and then show button3?
  2. Or if button3 is hidden, and then you move button1 to position 3 (among the visible ones) so the order is as follows: button2, button4, button1, button5, and then show button3?

@thalieht

Copy link
Copy Markdown
Contributor
  • Hiding buttons also removes their corresponding entry from the menu bar.

Which buttons we're talking about? Can you show me?

For example i hid the Preferences button. Not even the hotkey Alt+O works.

Screenshot_20260526_191337

@FTA7700

FTA7700 commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

@stalkerok

@FTA7700
How exactly are the button positions saved? Are they always absolute?

  1. What if button3 is hidden, and then you move button5 to position 3 (among the visible ones), so that the following order is obtained: button1, button2, button5, button4, and then show button3?
  2. Or if button3 is hidden, and then you move button1 to position 3 (among the visible ones) so the order is as follows: button2, button4, button1, button5, and then show button3?

Position is stored as an absolute slot index at the moment of hiding. When the button is shown again, it returns to that same slot number — whatever buttons now occupy the neighbouring slots.
Example: toolbar is [Open] [Download] [Delete] [Start] [Stop]. User hides Delete — slot 2 is saved. Then the user reorders others to [Stop] [Open] [Download] [Start]. Then shows Delete — it reappears at slot 2, giving [Stop] [Open] [Delete] [Download] [Start].

The alternative would be saving position relative to a named neighbour, but that breaks differently — if the named neighbour was also moved or hidden, the insertion point is equally arbitrary. There is no universally correct behavior when the user reshuffles things while a button is hidden. The absolute index approach is simple and predictable. Happy to change the strategy if you have a preference.


@thalieht

  • Hiding buttons also removes their corresponding entry from the menu bar.

Which buttons we're talking about? Can you show me?

For example i hid the Preferences button. Not even the hotkey Alt+O works.

Screenshot_20260526_191337

Fixed — hiding a toolbar button now uses removeAction/insertAction instead of setVisible, so the action is removed only from the toolbar while remaining fully functional in the menu bar and with its hotkey intact. Verified with Alt+O for Preferences.

@FTA7700
FTA7700 force-pushed the feature/toolbar-customization branch 2 times, most recently from 6699943 to 0558715 Compare May 27, 2026 02:06
@FTA7700

FTA7700 commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

Fixed some small discrepancy and also updated the description.

stalkerok
stalkerok previously approved these changes May 27, 2026

@stalkerok stalkerok left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current result is the best one.

@FTA7700

FTA7700 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Added the "Reset Toolbar Buttons" option. It fixes the all-hidden dead-end — you can hide every button and still get back to default from there.

{2A6807E4-39BE-4367-A63B-29D36B49C71D}

Comment thread src/gui/mainwindow.cpp
}

void MainWindow::toolbarMenuRequested()
void MainWindow::toolbarMenuRequested(const QPoint &pos)
stalkerok
stalkerok previously approved these changes Jul 21, 2026
@stalkerok
stalkerok requested review from Chocobo1 and glassez July 21, 2026 16:41

@glassez glassez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some preliminary remarks. Could you extend them to all similar places in the code and fix them before I continue my review?

Comment thread src/gui/customizabletoolbar.h Outdated
Comment thread src/gui/customizabletoolbar.h Outdated
Comment thread src/gui/customizabletoolbar.h Outdated
Comment thread src/gui/CMakeLists.txt Outdated
Comment thread src/gui/customizabletoolbar.cpp
Comment thread src/gui/customizabletoolbar.cpp Outdated
Comment thread src/gui/customizabletoolbar.cpp Outdated
{
m_dragJustFinished = false;

auto *me = static_cast<QMouseEvent *>(event);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you not use such shorthands?

Suggested change
auto *me = static_cast<QMouseEvent *>(event);
auto *mouseEvent = static_cast<QMouseEvent *>(event);

Comment thread src/gui/customizabletoolbar.cpp Outdated
Comment thread src/gui/customizabletoolbar.cpp Outdated
Comment thread src/gui/customizabletoolbar.h
@FTA7700

FTA7700 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Applied all suggested changes:

  • Removed redundant = default destructor
  • setLocked declaration matches definition (const only in .cpp)
  • actionEvent/eventFilter moved to private
  • Extracted switch-case bodies in eventFilter into handleMousePress/handleMouseMove/handleMouseRelease
  • Collapsed the widgetForAction null-check in actionEvent
  • Renamed memouseEvent throughout
  • Wrapped the long guard condition onto multiple lines
  • Dropped the unused parameter name in endDrag
  • Added GPL license header to customizabletoolbar.h/.cpp

Also replied on the CMakeLists.txt question above.

Comment thread src/gui/mainwindow.ui Outdated
Comment thread src/gui/mainwindow.cpp
Comment on lines +1085 to +1087
if ((action == m_spacerAction) || (action == m_columnFilterAction)
|| (action->objectName() == u"actionLock"_s))
break;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broken Coding style.

Suggested change
if ((action == m_spacerAction) || (action == m_columnFilterAction)
|| (action->objectName() == u"actionLock"_s))
break;
if ((action == m_spacerAction) || (action == m_columnFilterAction)
|| (action->objectName() == u"actionLock"_s))
{
break;
}

Comment thread src/gui/mainwindow.cpp Outdated
Comment on lines +451 to +455
if (a->isSeparator() || a->text().isEmpty()
|| (a == m_spacerAction) || (a == m_columnFilterAction)
|| (a->objectName() == u"actionLock"_s))
continue;
m_allToolbarActions.append(a);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broken coding style.

Suggested change
if (a->isSeparator() || a->text().isEmpty()
|| (a == m_spacerAction) || (a == m_columnFilterAction)
|| (a->objectName() == u"actionLock"_s))
continue;
m_allToolbarActions.append(a);
if (a->isSeparator() || a->text().isEmpty()
|| (a == m_spacerAction) || (a == m_columnFilterAction)
|| (a->objectName() == u"actionLock"_s))
{
continue;
}
m_allToolbarActions.append(a);

Comment thread src/gui/mainwindow.cpp Outdated
Comment on lines +672 to +675
if ((a == m_spacerAction) || (a == m_columnFilterAction)
|| (a->objectName() == u"actionLock"_s))
break;
m_ui->toolBar->removeAction(a);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broken coding style.

Suggested change
if ((a == m_spacerAction) || (a == m_columnFilterAction)
|| (a->objectName() == u"actionLock"_s))
break;
m_ui->toolBar->removeAction(a);
if ((a == m_spacerAction) || (a == m_columnFilterAction)
|| (a->objectName() == u"actionLock"_s))
{
break;
}
m_ui->toolBar->removeAction(a);

@FTA7700

FTA7700 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Pushed fixes for all of today's feedback:

  • Reverted the unnecessary CMakeLists.txt include-dir change — real fix was the header path in mainwindow.ui
  • Added braces to the three multi-line if conditions flagged (saveToolbarState, resetToolbarToDefault, the toolbar snapshot loop)
  • Also found and fixed the same brace pattern in loadSettings()

@glassez glassez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet another review step from me.

As I can see many code in mainwindow.cpp should be encapsulated in CustomizableToolBar class.

Comment thread src/gui/CMakeLists.txt Outdated
log/logfiltermodel.h
log/loglistview.h
log/logmodel.h
customizabletoolbar.h

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect order.

Comment thread src/gui/CMakeLists.txt Outdated
log/logfiltermodel.cpp
log/loglistview.cpp
log/logmodel.cpp
customizabletoolbar.cpp

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Comment thread src/gui/mainwindow.cpp Outdated

// Load Window state and sizes
// Snapshot all customizable toolbar actions before loadSettings may reorder them
for (QAction *a : asConst(m_ui->toolBar->actions()))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you not use such shorthands? (I think I've already asked for this.)

Suggested change
for (QAction *a : asConst(m_ui->toolBar->actions()))
for (QAction *action : asConst(m_ui->toolBar->actions()))

Comment thread src/gui/mainwindow.cpp
a->setVisible(false);
}
m_queueActionsShown = false;
loadPreferences();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need it here?

Comment thread src/gui/mainwindow.cpp Outdated

// Find nearest visible action/separator after this one
int nextIdx = idx + 1;
while (nextIdx < acts.size() && !acts[nextIdx]->isSeparator() && !acts[nextIdx]->isVisible())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
while (nextIdx < acts.size() && !acts[nextIdx]->isSeparator() && !acts[nextIdx]->isVisible())
while ((nextIdx < acts.size()) && !acts[nextIdx]->isSeparator() && !acts[nextIdx]->isVisible())

Comment thread src/gui/mainwindow.cpp Outdated
Comment on lines +804 to +805
if (queueActionNames.contains(a->objectName()))
continue;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (queueActionNames.contains(a->objectName()))
continue;
if (queueActionNames.contains(a->objectName()))
continue;

Comment thread src/gui/mainwindow.cpp Outdated
Comment on lines +835 to +836
if (!queueActionNames.contains(a->objectName()))
continue;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!queueActionNames.contains(a->objectName()))
continue;
if (!queueActionNames.contains(a->objectName()))
continue;

Comment thread src/gui/mainwindow.cpp
if (!a->isSeparator() && !a->objectName().isEmpty())
actionMap[a->objectName()] = a;
}
const QList<QAction *> currentActions = m_ui->toolBar->actions();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const QList<QAction *> currentActions = m_ui->toolBar->actions();
const QList<QAction *> currentActions = m_ui->toolBar->actions();

Comment thread src/gui/mainwindow.cpp
if (a->isSeparator() || actionMap.contains(a->objectName()))
m_ui->toolBar->removeAction(a);
}
for (const QString &entry : savedState)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (const QString &entry : savedState)
for (const QString &entry : savedState)

Comment thread src/gui/mainwindow.cpp
const QStringList parts = entry.split(u":"_s);
if (parts.size() != 2)
continue;
const QString &name = parts[0];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const QString &name = parts[0];
const QString &name = parts[0];

- Fix CMakeLists.txt alphabetical ordering for customizabletoolbar.h/.cpp
- Rename shorthand variables (a, w, bw) to descriptive names across
  mainwindow.cpp and customizabletoolbar.cpp
- Use brace-init for QMenu construction per coding guidelines
- Add parentheses around while-loop conditions
- Add blank-line spacing per suggestions
- Move m_spacerAction next to other toolbar-related member variables
@FTA7700

FTA7700 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Why do you need it here?

The queue-sync logic already existed in loadPreferences() before Reset to Default did. It was faster to call the existing function than duplicate that block, and I never went back to split it out properly. Fair point that pulling in the whole function for that one side effect is more than necessary. I'll extract the queue-sync block into its own method that both loadPreferences() and resetToolbarToDefault() call. Just give me a green light.

Yet another review step from me.

As I can see many code in mainwindow.cpp should be encapsulated in CustomizableToolBar class.

Went through every toolbar-related function in mainwindow.cpp to see what's actually feasible to move. Most of them only depend on Preferences (for persistence) or on MainWindow-owned state that's really toolbar bookkeeping (m_allToolbarActions, m_hiddenToolbarActions, m_spacerAction, m_columnFilterAction) — if that state moved onto CustomizableToolBar, most of this could move with it: addToolbarContextMenu(), resetToolbarToDefault(), toolbarMenuRequested(), saveToolbarState(), and the five toolbarIconsOnly/TextOnly/... style setters.

One clear exception: toolbarMenuRequested() checks BitTorrent::Session::instance()->isQueueingSystemEnabled() to build the queue-actions submenu. That's session/business logic — I don't think a toolbar widget should depend on it directly, so I'd keep that check in MainWindow and have it feed the result into the toolbar rather than moving it.

One open question: should CustomizableToolBar read Preferences directly itself, or should MainWindow keep owning all Preferences access and hand data to the toolbar instead? Curious which you'd prefer before I start moving things.

Does the overall scope match what you had in mind, or is there a narrower/different set of functions you were thinking of?

@glassez

glassez commented Aug 1, 2026

Copy link
Copy Markdown
Member

Does the overall scope match what you had in mind, or is there a narrower/different set of functions you were thinking of?

I don't have enough time to analyze it in detail right now. I'll come back to this later. But for now, I can give you some general advice that might help you figure it out for yourself.
If we consider a generic CustomizableToolBar class that properly encapsulates all relevant data and methods, then you could use it in several places with a different set of actions. Just imagine that you need to re-use it, and try to implement it in such a way that it becomes possible.

@glassez

glassez commented Aug 1, 2026

Copy link
Copy Markdown
Member

One clear exception: toolbarMenuRequested() checks BitTorrent::Session::instance()->isQueueingSystemEnabled() to build the queue-actions submenu.

What submenu do you talk about?

@FTA7700

FTA7700 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

One clear exception: toolbarMenuRequested() checks BitTorrent::Session::instance()->isQueueingSystemEnabled() to build the queue-actions submenu.

What submenu do you talk about?

Sorry, bad wording, I meant the queue section inside "Show/Hide Buttons."

@glassez

glassez commented Aug 2, 2026

Copy link
Copy Markdown
Member

One clear exception: toolbarMenuRequested() checks BitTorrent::Session::instance()->isQueueingSystemEnabled() to build the queue-actions submenu.

What submenu do you talk about?

Sorry, bad wording, I meant the queue section inside "Show/Hide Buttons."

But should it really depend on business logic? Toolbar has to deal with its current actions. The caller adds "queue" actions to the toolbar if "queueing" is enabled, and deletes (does not add) them if "queueing" is disabled. Toolbar should not know what kind of actions it manages.

@FTA7700

FTA7700 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Pushed moving toolbar bookkeeping (action registry, separators, reset-to-default) into CustomizableToolBar, per your reuse-test suggestion.

On the queueing question - right now when queueing is off, the queue buttons still show up in the Show/Hide menu, grayed out - it's a hint that the feature state is "off". If the toolbar stops knowing about queueing and just reflects whatever actions are actually added to it, that hint goes away: queue buttons would simply not appear in the menu while queueing is off, and appear normally once it's turned on. The present state feels more natural for me as a user. But your call.

Edit:

Actually, we can probably keep the grayed-out hint without CustomizableToolBar itself knowing about queueing.

The isQueueingSystemEnabled() check doesn't have to live in the toolbar class - it can stay in MainWindow's own menu-building code, which already talks to BitTorrent::Session everywhere else. The toolbar just needs to actually remove queue actions when queueing turns off (instead of only hiding them), so its action list reflects reality. MainWindow can still decide, on its own, whether to show a missing queue action as a grayed-out hint or leave it out entirely.

@glassez

glassez commented Aug 3, 2026

Copy link
Copy Markdown
Member

I still believe the CustomizableToolBar can (should) handle it in an abstract way. Caller is free to enable/disable any of actions it added to toolbar. The toolbar shows appropriate menu items as enabled/disabled and doesn't show the items for disabled actions in the toolbar itself (if we were talking about a library class, this behavior could be made configurable, for example, by providing hideDisabledActions property.).

@FTA7700

FTA7700 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Ok, here are the latest updates:

CustomizableToolBar now listens to QAction::enabledChanged on every action it manages, and shows/hides the button on the bar automatically based on that - no knowledge of what the action is or why it's enabled/disabled, just reacting to the plain Qt switch, per your suggestion.

toolbarMenuRequested() no longer checks isQueueingSystemEnabled(). The Show/Hide menu row for each queue action just checks that action's own isEnabled() now, same as the toolbar itself does.

loadPreferences() now also calls setEnabled()/setEnabled(false) on the queue actions, alongside the existing setVisible() calls. Kept setVisible() too - it's what makes the queue entries disappear from the Edit menu when queueing is off, which is separate from the toolbar and didn't need to change.

resetToolbarToDefault() had a manual "hide queue actions" loop that became dead code once loadPreferences() started doing this properly - removed it instead of updating it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Implement new feature/subsystem GUI GUI-related issues/changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Customize Toolbar GUI Allow editing the top toolbar More buttons on toolbar Toolbar customization

5 participants