-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Widget highlighting and shortcut system implementation #7488
base: master
Are you sure you want to change the base?
Conversation
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 haven't looked over all the code yet, but here are some of my initial thoughts
@@ -40,6 +42,36 @@ namespace lmms::Clipboard | |||
StringPair, | |||
Default | |||
}; | |||
|
|||
enum StringPairDataType |
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 name could probably be shortened to just DataType
or something.
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.
Which one is better: KeyType
or PairType
or PairDataType
or maybe CarriedDataType
? I think DataType
doesn't say anything about the StringPairDrag
system and it could be confusing because there is also a Default
mime type, but I guess this can be fixed with a comment.
I would like to get a finalized name because this change would effect 30 files.
return shortcuts; | ||
} | ||
|
||
void FloatModelEditorBase::processShortcutPressed(size_t shortcutLocation, QKeyEvent* event) |
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 would be nice if the size_t shortcutLocation
parameter could be const ModelShortcut&
instead.
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 would like to keep the size_t
because I think it is easier to use in this case (with switch
) and it could be more optimized (checking if size_t
== size_t
instead of ModelShortcut
== shortcutArray[index]
).
643cada
to
2012037
Compare
This PR contains multiple changes and overall it is focused on improving lmms functionality communication with the user and on immediate feedback to the user.
These changes are the following:
StringPairDrag
system and theStringPair
system now uses a unifiedenum
as key instead of custom strings. Before this, key names weren't documented and they were scattered across multiple files.StringPair
key while dragging or copying. If the user starts to drag a clip, the other clips will be highlighted to show that they can accept the datatype associated with the drag event.FloatModelEditorBase
) and all clips. I plan to implement it in other widgets in the future (if this gets merged).Problems:
InteractiveModelView
widgets redirectPianoView
'sPianoView::keyPressEvent
to themself. This solution works but it isn't ideal.ClipView::getClipStringPairType
andTrackView::getTrackStringPairType
does not implement Special types of tracks (video, event)ClipView::dropEvent
copying code because I found that this condition is never true:Picture showing highlighting (for knobs and clips separately):