fix: clean up stray toolbar/menu entries from the profile-load bug#9200
Draft
vadi2 wants to merge 7 commits into
Draft
fix: clean up stray toolbar/menu entries from the profile-load bug#9200vadi2 wants to merge 7 commits into
vadi2 wants to merge 7 commits into
Conversation
|
Hey there! Thanks for helping Mudlet improve. 🌟 Test versionsYou can directly test the changes here:
No need to install anything - just unzip and run. |
Contributor
|
Comment on lines
+10087
to
+10110
| for (TAction* pRoot : bogus) { | ||
| const int id = pRoot->getID(); | ||
| if (QTreeWidgetItem* pItem = findItemByID(mpActionBaseItem, id)) { | ||
| // The bogus root is a child of the synthetic "Buttons" base item, not | ||
| // a true top-level item, so take+delete through its parent. | ||
| if (QTreeWidgetItem* pParentItem = pItem->parent()) { | ||
| delete pParentItem->takeChild(pParentItem->indexOfChild(pItem)); | ||
| } else { | ||
| delete treeWidget_actions->takeTopLevelItem(treeWidget_actions->indexOfTopLevelItem(pItem)); | ||
| } | ||
| } | ||
| clearEditorState(EditorViewType::cmActionView, id); | ||
| // ~TAction pops from its parent and recursively deletes children. | ||
| delete pRoot; | ||
| } | ||
|
|
||
| if (mCurrentView == EditorViewType::cmActionView) { | ||
| mpCurrentActionItem = nullptr; | ||
| clearActionForm(); | ||
| } | ||
| pActionUnit->updateToolbar(); | ||
|
|
||
| hideSystemMessageArea(); | ||
| mudlet::self()->announce(tr("Removed %n leftover toolbar entry.", "", bogus.size())); |
Contributor
There was a problem hiding this comment.
Cleanup is non-undoable without user warning
slot_cleanupBogusActions deletes items directly without pushing an EditorDeleteItemCommand to mpUndoStack, unlike the normal delete_action() path. After clicking "Yes" in the dialog, the user cannot recover the removed entries via Ctrl+Z. Consider updating the dialog text to make this explicit so users don't expect Undo to work:
Suggested change
| for (TAction* pRoot : bogus) { | |
| const int id = pRoot->getID(); | |
| if (QTreeWidgetItem* pItem = findItemByID(mpActionBaseItem, id)) { | |
| // The bogus root is a child of the synthetic "Buttons" base item, not | |
| // a true top-level item, so take+delete through its parent. | |
| if (QTreeWidgetItem* pParentItem = pItem->parent()) { | |
| delete pParentItem->takeChild(pParentItem->indexOfChild(pItem)); | |
| } else { | |
| delete treeWidget_actions->takeTopLevelItem(treeWidget_actions->indexOfTopLevelItem(pItem)); | |
| } | |
| } | |
| clearEditorState(EditorViewType::cmActionView, id); | |
| // ~TAction pops from its parent and recursively deletes children. | |
| delete pRoot; | |
| } | |
| if (mCurrentView == EditorViewType::cmActionView) { | |
| mpCurrentActionItem = nullptr; | |
| clearActionForm(); | |
| } | |
| pActionUnit->updateToolbar(); | |
| hideSystemMessageArea(); | |
| mudlet::self()->announce(tr("Removed %n leftover toolbar entry.", "", bogus.size())); | |
| confirm.setText(tr("Remove the following %n entry from this profile's Buttons tree? This cannot be undone.", "", bogus.size())); |
…le load
#### Brief overview of PR changes/additions
Discord user **Bob (bodega)** reported 2026/04/15 11:28 UTC:
> good day, can anyone replicate - Each time I open a profile, a new
button group and button is created. No other packages/scripts loaded for
this test:
{Screen shot showing several locked "New toolbar" each with a locked
"New menu" in the "Buttons" view in the editor}
> This happens each time the profile is loaded. Close/reopen and a new
button group appears.
It was happening for him on Windows and I've just discovered it was
affecting the latest PTB on Linux as well.
This was a bug tracked down by **Humera (humera1295)** and myself to two
calls to `addAction(...)` in the `dlgTriggerEditor` class constructor.
Unfortunately these were calling the `dlgTriggerEditor::addAction(...)`
method which is used to construct permanent Toolbars/Menus/Buttons and NOT
the `QMainWindows::addAction(...)` that was expected/wanted in this case.
This seemed to be introduced by Mudlet#8469.
#### Motivation for adding to Mudlet
Prevent bogus items being created. The bug was probably also preventing the
ability to perform undo/redo actions with the <Ctrl>+Z and <Ctrl>+Y
keystrokes - which should be restored by this PR.
#### Other info (issues closed, discussion etc)
An announcement will probably be useful as this PR does not attempt to
remove such bogus entries already created (although since they are disabled
they will not show in the application window and they will just clog up the
"Buttons" view in the editor.
A second defect was also noted in that the `tempButton(...)` and
`tempButtonToolbar(...)` Lua API functions do NOT mark the items they
create as being **temporary** so they persist longer than intended (the end
of the session). We do not currently provide either `permButton(...)` or
`permButtonToolbar(...)` functions.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
According to Greptile's analysis more work is needed to avoid the persistence of temporary Buttons/Menus/Toolbars besides just setting the "Temporary" flag. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…lbars - Rename dlgTriggerEditor::addAction(bool) to addNewAction(bool) so it no longer hides the QMainWindow::addAction(QAction*) overload (root cause of Mudlet#9194). - Add BogusActionScanner and an editor banner that offers to remove leftover "New toolbar"/"New menu" pairs from profiles affected by the bug.
- Don't clobber existing error/warning banners when offering cleanup. - Warn and report partial results if the tree widget row is gone but the TAction is still in the unit. - Tell the user when the cleanup click finds nothing instead of silently hiding the banner. - Add translator context on the user-visible strings.
- Snapshot IDs and re-lookup each TAction in the cleanup loop so signal reentrancy during delete/updateToolbar can't invalidate raw pointers - Log drift instead of silently swallowing when a tree item somehow has no parent (the previous fallback was unreachable; now tracked and warned) - Match both current-locale tr() names and untranslated English literals so users who switched UI language after being hit by the bug still see the cleanup offer - Reword %n tr() strings to "entry/entries" so they read correctly for any count, not just n=1 - Tighten mBogusActionsNotified header comment to describe lifecycle - Add tests: isTemporary discriminator, child-side script mutation, multi-pair detection+cleanup, showEvent->timer wiring
ec519d6 to
717087d
Compare
Drops the static-only class wrapper; moves the two scanner functions as private static methods on dlgTriggerEditor. Helpers live in an anonymous namespace. Test class + file renamed to BogusActionCleanupTest.
- fix misleading comments (no signals; Qt %n plurals) - always dedupe English+translated scans to catch historical locales - warn on unknown mudlet: URLs instead of routing to showIntro - cover updateToolbar() and cmActionView branch in tests - bound modal poller deadline; use QTRY_VERIFY_WITH_TIMEOUT
Member
|
💭 Although this is cleaning up a mess we made - is the added code/complexity worth the cost/effort? It is something that is only going to affect recent users (since around November 25 IIRC) and is something that can be cleaned up by the end-user if they are made aware of it - perhaps with the announcement of the next release... |
Member
Author
|
I think we'd all like Mudlet to be an application that cleans up after itself and keep things tidy (barring uninstalls, as that's a separate case) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brief overview of PR changes/additions
Renames the shadowing
addAction(bool)to prevent the root cause from returning, and adds an editor banner that offers to clean up the leftover "New toolbar"/"New menu" pairs already in affected profiles.Motivation for adding to Mudlet
Users affected by #9194 have bogus disabled entries piled up in their Buttons tree that they can't easily remove; this gives them a one-click fix and prevents the original bug from returning.
Other info (issues closed, discussion etc)
Follow-up to #9194.
Test case: Open a profile that still has the stray "New toolbar" > "New menu" pair, open the script editor, click the banner link, confirm - entries should disappear and not come back after restart.