Skip to content
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

Open
wants to merge 81 commits into
base: master
Choose a base branch
from

Conversation

szeli1
Copy link
Contributor

@szeli1 szeli1 commented Sep 5, 2024

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:

  • The StringPairDrag system and the StringPair system now uses a unified enum as key instead of custom strings. Before this, key names weren't documented and they were scattered across multiple files.
  • A new class was added that implements handling shortcuts and highlighting widgets:
    • This class was designed to encourage developers to implement shortcuts and pasting data into the widget.
    • Shortcuts:
      • Shortcuts support all QT keys and modifiers and they can differentiate between how many times a shortcut was pressed. Each shortcut has a description.
      • Shortcuts aim to be easy to work with.
      • If the user moves the mouse over the widget, then the available shortcuts will automatically be displayed.
      • In the future settings could be added that could allow the user to change the shortcuts.
    • Highlighting:
      • The idea behind highlighting is to show users which widget can accept which 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.
      • Highlighting will be active for 10 seconds after the user starts to ctrl + drag a widget or when a widget is copied.
      • Highlighting will stop after a successful paste, but the user can keep pasting into other widgets.
  • The new class is implemented inside Knobs (FloatModelEditorBase) and all clips. I plan to implement it in other widgets in the future (if this gets merged).

Problems:

  • For shortcuts to work the new InteractiveModelView widgets redirect PianoView's PianoView::keyPressEvent to themself. This solution works but it isn't ideal.
  • ClipView::getClipStringPairType and TrackView::getTrackStringPairType does not implement Special types of tracks (video, event)
  • I removed ClipView::dropEvent copying code because I found that this condition is never true:
// Defer to rubberband paste if we're in that mode
if (m_trackView->trackContainerView()->allowRubberband() == true)

Picture showing highlighting (for knobs and clips separately):

widget_highlighting6

Copy link
Member

@messmerd messmerd left a 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

include/Clipboard.h Outdated Show resolved Hide resolved
@@ -40,6 +42,36 @@ namespace lmms::Clipboard
StringPair,
Default
};

enum StringPairDataType
Copy link
Member

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.

Copy link
Contributor Author

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.

include/Clipboard.h Outdated Show resolved Hide resolved
src/gui/widgets/FloatModelEditorBase.cpp Outdated Show resolved Hide resolved
src/gui/widgets/FloatModelEditorBase.cpp Outdated Show resolved Hide resolved
include/InteractiveModelView.h Outdated Show resolved Hide resolved
include/InteractiveModelView.h Outdated Show resolved Hide resolved
return shortcuts;
}

void FloatModelEditorBase::processShortcutPressed(size_t shortcutLocation, QKeyEvent* event)
Copy link
Member

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.

Copy link
Contributor Author

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]).

@szeli1 szeli1 force-pushed the widget_highlighting_and_improvements branch from 643cada to 2012037 Compare November 2, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants