Add: undoredo support #7447
Add: undoredo support #7447
Conversation
|
💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe. |
|
@vadi2 I have PR implementing undoredo support in editor. The pr implements qt undo framework which uses the command pattern. |
|
Awesome! |
|
/create links |
There was a problem hiding this comment.
There are too many unrelated whitespace / code layout changes in this PR for me to review it. (The signal-to-noise ratio is too low!) 😀
Also, the custom editor widget (edbee-lib) that we are using for the Lua-script also has undo/redo support as I understand it (but each separate Mudlet item - trigger, timer. key-binding, etc. has to be treated as a separate "document" as it calls them) - so I am not sure how that meshes with this PR.
src/dlgTriggerEditor.cpp
Outdated
| @@ -6221,7 +6412,7 @@ void dlgTriggerEditor::slot_variableSelected(QTreeWidgetItem* pItem) | |||
| } else if (vu->isSaved(var)) { | |||
| pItem->setCheckState(0, Qt::Checked); | |||
| } | |||
| pItem->setData(0, Qt::UserRole, var->getValueType()); | |||
| pItem->setData(0, Qt::UserRole, var->getId()); | |||
There was a problem hiding this comment.
Are you replacing this use of the Qt::UserRole for your own, new purposes? If so have you checked that everywhere the original usage is not needed and can be removed? If not you may have to use a different "key" to store this ID number (e.g. at Qt::UserRole + 1). Of course it could be that the original use of var->getValueType() as the original source of a value to store here was bogus and wrong - I didn't have a hand in this bit of the code-base (variables part of the editor) so I am not sure of the significance of it.
There was a problem hiding this comment.
@SlySven I checked and the getValueType() is only retrieved from TVar type not the actual pItem, the Id is used so when the item is moved from one parent to another, the id is used to determine if the item was move to another parent. Also I checked the other items types i.e triggers, scripts, keys, actions store Id in the UserRole, so the original source of storing valueType in userRole was bogus.
There was a problem hiding this comment.
Thanks for adding this, it's great to have a start on the undo/redo.
At a higher level, it seems that not all actions are added to the queue - for example, adding new named event handlers in a script is not a queue action. In a trigger, editing the trigger (adding new patterns or changing options) does not add anything either.
Could you cover all actions for triggers, aliases, scripts, timers, keys, and buttons like this? Variables can be left out I reckon in the initial implementation, those are too tricky to deal with.
|
@vadi2 For undo functionality, initially I have covered undo support for add, delete and moving items. Each item i.e trigger, key, script has various options i.e editing trigger name, command or pattern. I am in the process of adding the remaining actions for triggers, aliases, scripts, timers, keys, and buttons. I have broken down the todo remaining items, Trigger
Alias
Scripts
Timers
Keys
Buttons
Also would be helpful if the bounty amount is reviewed and slightly increased, given the undo support has turned out to be rather large PR and the remaining todo items also add considerable effort. Also @SlySven I tested the undo/redo functionality for custom editor widget and the Lua-Script is setup to undo using shortcuts ctrl+z, ctrl+shift+z which has no impact on the undo functionality of items. |
|
I agree. Bounty raised to $300. Looking forward to seeing this wrapped up! |
|
@vadi2 I have added the remaining actions for triggers, aliases, scripts, timers, keys, and buttons. Please test the PR. Thanks. |
|
@coder1152 the PR does not seem to build, please check. |
|
@vadi2 I have resolved the build issue. PR is ready. |
|
Looks like Windows builds don't work yet. |
|
@vadi2 Following up on qa feedback, I have resolved the part when the name of group ends up in place of alias name. Also, the paste regression issue is resolved. For the remaining part of the feedback, I need video of the issue. |
|
46d12a5 seems like progress is being made.
Sorry about that. I'm trying different things each run through and didn't make note of whatever item type I clicked on Saturday, but it was the first item type that I clicked. Just figured they all would have similar logic so probably not specific to the one I happened to try. I think it was timers since I have some stray timers in that profile now. I'll specifically mess with timers in a moment. Verify all of the things with each of the types though.
That indicates interest in getting the feature but should not be taken as like desperation to hit a release date with lower standards. Go by what Vadi wrote here and the expectation is to have it good before being presented as ready to merge. Vadi hasn't offered to pay me a bonus on top of my normal tester salary for prioritizing bounty items instead of more interesting items. In fact I'm weirdly helping you race me. Anyway, now for the trying of things... With the 46d12a5 build,
|
…k' into undoredoframework
|
@vadi2 I have resolved the following points from qa feedback.
The following points I was not able to replicate and need more details or video of the issue.
|
|
@vadi2 Please reach out to tester regarding qa feedback. |
|
Changing trigger types is causing some problems, it switches focus to wrong trigger item and also draws the colors wrong for rest of session |
|
as per Tim's comment above the focus is lost after changing trigger types Peek.2025-06-08.09-25.mp4also noticed that using a prompt type, then switching back to a different type looses the trigger entry and undo doesn't restore it Peek.2025-06-08.09-29.mp4 |
|
need to remove debug messages sent to command line to tidy up |
|
@vadi2 The focus issue from switching trigger types is resolved along with some other issues. Thanks for testing ZookaOnGit and atari2600tim. |
Brief overview of PR changes/additions
/claim #707
Resolves issue #707
Motivation for adding to Mudlet
The PR adds support for undoredo in the editor using qt undo redo framework.
Other info (issues closed, discussion etc)