-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Turn Undo/redo history dialog into History panel #25389
base: master
Are you sure you want to change the base?
Conversation
{ | ||
} | ||
|
||
size_t UndoRedoHistoryModel::undoRedoActionCount() const | ||
void UndoRedoHistoryModel::classBegin() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity - what's the reason for doing this (and all the QQmlParserStatus
stuff) instead of calling an init
function from a Component.onCompleted
signal as we do for most models in the app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to try out this approach once. It might be a bit more idiomatic. If it is successful, we can roll it out to other places too.
@@ -225,7 +226,7 @@ const muse::TranslatableString NotationUndoStack::undoRedoActionNameAtIdx(size_t | |||
return {}; | |||
} | |||
|
|||
if (auto action = undoStack()->atIndex(idx)) { | |||
if (auto action = undoStack()->lastAtIndex(idx)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels a little odd to call a method with the name undoRedoActionNameAtIdx
and have it return the name of the last action. To me it would make more sense for this to do what it says on the tin, and call it with idx - 1
if needs be - what do you reckon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is: UndoStack
indexes are state indexes, and not action indexes in m_macroList
. Because there is an initial state with index 0, the UndoStack
state index of a given state is always one higher than the index of the action in m_macroList
that led to this state. It's a bit like this:
I could rename this method to NotationUndoStack::lastActionNameAtIndex
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok got you - thanks.
I could rename...
Yeah I think this would be clearer - and/or a little comment detailing the above.
if (!masterNotation->hasParts()) { | ||
return code == UNDO_ACTION_CODE || code == REDO_ACTION_CODE || code == UNDO_HISTORY_CODE; | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the comment is no longer relevant with this change.
I was concerned that this change would prevent Undo as well as Redo, so I went to try to test it. But I was thwarted by #25395. Anyway, it's a concern I have that may not be valid.
fe43025
to
16d5bae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cbjeukendrup! A few things:
1. Menu placement:
Since this is now a panel and not a dialog, I think we should add it to the 'View' menu as well, perhaps between 'Selection filter' and 'Navigator'. That would group it with other items without a shortcut, while keeping the end of the section mostly options pertaining to sound.
We can keep it in the 'Edit' menu too, but let's get rid of the '...' at the end (since we mostly use that for dialogs, not panels).
2. Wording:
- I think we should replace "Initial state" as the first thing in the list with the slightly friendlier "File opened"
- When I change a selected note's pitch with arrow keys, the title of the action is "Change pitch", but when I drag it to change the pitch, the title is "Drag element". If it's a small job, could we make the latter scenario say "Change pitch" too?
3. UI:
My bad for not checking before, but let's make the italic text on the "undone" rows 60% opacity instead of 50% so that it passes contrast guidelines for accessibility.
It was just a logic error; correct logic would have been ```c++ if (code != UNDO_ACTION_CODE && code != REDO_ACTION_CODE) { if (!masterNotation->hasParts()) { return false; } } ``` I.e. all actions, except undo/redo, can only be handled when the current notation contains parts (instruments). However, to make the intention even more clear, and to prevent lookup in m_isEnabledMap while we already know which action code it is because we have just *checked* which action code it is, I opted for the solution of this commit.
16d5bae
to
1a60f65
Compare
@avvvvve Thanks, I implemented most changes. A couple of things:
|
|
One more thing - can we rename the shortcut action from "Undo/redo history" to "Show/hide history" to match other action names? |
I'm a bit worried about loosening the relation to Undo/redo too much, by putting it in the View menu. If we do that, people who are looking for it will probably not find it, because just below undo/redo would a priori be the only place where one might expect such functionality; and people who are not looking for it but stumble across it, won't understand what it is about, because "History" is a very general term. |
@cbjeukendrup I support this plan! For the shortcut, I was going to say "Show/hide edit history" might be cleaner, but I suppose then we lose the advantage of seeing it when you search for "undo" in shortcuts. Maybe just "Show/hide undo history" for brevity? I think the redo is implied enough. |
Resolves: #25327