Widget highlighting and shortcut system implementation #7488

szeli1 · 2024-09-05T21:39:23Z

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)
Shortcuts without modifiers (Qt::NoModifier) are not supported because of focusing issues.
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):

messmerd

I haven't looked over all the code yet, but here are some of my initial thoughts

include/Clipboard.h

messmerd · 2024-09-06T01:47:50Z

include/Clipboard.h

@@ -40,6 +42,36 @@ namespace lmms::Clipboard
 		StringPair,
 		Default
 	};
+
+	enum StringPairDataType


The name could probably be shortened to just DataType or something.

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.

Perhaps MimeType::Default could be renamed to MimeType::Plain so that it's contextualized away from further entropy, then you could choose whatever name you'd like, including something as simple as Clipboard::Type, since for all intents and purposes, that's what this enum describes.

Edit: Hmm... on second thought, Type would look ugly next to MimeType, so perhaps @messmerd's proposal of DataType is the shortest possible name that we can get away with?

include/Clipboard.h

src/gui/widgets/FloatModelEditorBase.cpp

include/InteractiveModelView.h

messmerd · 2024-09-06T02:26:06Z

src/gui/widgets/FloatModelEditorBase.cpp

+	return shortcuts;
+}
+
+void FloatModelEditorBase::processShortcutPressed(size_t shortcutLocation, QKeyEvent* event)


It would be nice if the size_t shortcutLocation parameter could be const ModelShortcut& instead.

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

qnebra · 2024-11-21T17:10:50Z

So, overall:

shortcut system is fine, not absolutely perfect and across entirety of lmms, but much better than having nothing
linked controls, when user gets an idea on how to use it, is actually well done and useful. Just don't link a lot of controls, as disconnecting one by one can be tedious.
new automation pattern drag & drop behaviour, while changes few things in workflow, is much less buggy and had properly working undo, comparing to old method

It doesn't cause major issues with lmms in my case of using it daily for around two - three weeks.

…fixing_focus_issues

szeli1 · 2025-01-27T20:08:39Z

the volume of the midi track goes down without an active automation track.

I'm sorry, but I'm unsure if I see where this occurs. I don't see knobs moving and in the clip, both times the volume stays the same.

Spacemagehq · 2025-01-27T20:20:29Z

the volume of the midi track goes down without an active automation track.

I'm sorry, but I'm unsure if I see where this occurs. I don't see knobs moving and in the clip, both times the volume stays the same.

I looked elsewhere and nothing is automating the volume to make it go down.

szeli1 · 2025-01-27T20:22:19Z

I looked elsewhere and nothing is automating the volume to make it go down.

I don't see where this happens, could you point me to the right direction?

Spacemagehq · 2025-01-27T20:33:01Z

I looked elsewhere and nothing is automating the volume to make it go down.

I don't see where this happens, could you point me to the right direction?

The issue is in the envelope tab in the instrument ui. when you turn the amt to the right, the audio bug happens. But if you don't the audio bug is not there.

szeli1 · 2025-01-27T22:16:45Z

The issue is in the envelope tab in the instrument ui. when you turn the amt to the right, the audio bug happens. But if you don't the audio bug is not there.

Please could you show a screenshot of what happens and a description of what happens and a description of how it should behave? And a description of how to recreate it. I'm sorry, but I don't know what you mean by "audio bug", since I didn't touch the audio code. I believe you are trying to show that a knob moves without any automation channel attached, but I can't see this in the video you showed. I tried to reproduce the video and I believe everything worked as expected.

Please could you describe the issue you are experiencing in detail?

szeli1 · 2025-01-27T22:25:29Z

And please could you share your opinion about the actual features this PR brings? @Spacemagehq I would appreciate it if I got feedback on how the shortcut system could be changed and how this PR could be improved, or at least it would be useful if you confirmed that everything else worked as it should.

regulus79 · 2025-01-27T22:32:58Z

With this pr there's an audio bug on Windows 11 when you go to the volume tab in the build in instruments when you move the amt to the right the volume of the midi track goes down without an active automation track. I tested this with Nescaline as well, not just with TripleOscillator.

As far as I can tell, this is not a bug. Your envelope has a bit of hold, with the sustain at about half. So, after the hold time is up, the note will go down to the sustain volume of 0.5. If you don't want you note to decay, turn the sustain knob to 1. I don't believe this is relevant to this PR.

Spacemagehq · 2025-01-28T01:13:46Z

With this pr there's an audio bug on Windows 11 when you go to the volume tab in the build in instruments when you move the amt to the right the volume of the midi track goes down without an active automation track. I tested this with Nescaline as well, not just with TripleOscillator.

As far as I can tell, this is not a bug. Your envelope has a bit of hold, with the sustain at about half. So, after the hold time is up, the note will go down to the sustain volume of 0.5. If you don't want you note to decay, turn the sustain knob to 1. I don't believe this is relevant to this PR.

I looked at it again the nightly and the pr they both set to default values on both the pr and the nightly lmms the nightly doesn't have this audio issue. The issue can be user error and my fault, if it is, I just delete the video from the github.

Spacemagehq · 2025-01-28T01:22:26Z

And please could you share your opinion about the actual features this PR brings? @Spacemagehq I would appreciate it if I got feedback on how the shortcut system could be changed and how this PR could be improved, or at least it would be useful if you confirmed that everything else worked as it should.

The pup up hotkey and the highlights I had no issues with. This might be a useless thing to add, but maybe the ability to change the highlight color in the future ?

Spacemagehq · 2025-01-28T05:07:42Z

Fixed the audio issue turns out it was something on my end didn't have a setting set correctly, both Golden Plunger on discord and regulus give me the solution. Deleting the video that is on here.

tresf · 2025-02-27T17:05:01Z

src/core/Clipboard.cpp

@@ -65,11 +67,62 @@ namespace lmms::Clipboard
 	}


+	QString getStringPairKeyName(StringPairDataType type)


Is it right to assume that this will construct a new QString on each call to getStringPairKeyName? I'm not too familiar with C++ enums, but getting an enum by name is so very common in other languages that I feel that there may be a better strategy to this (seems like something that may be best tucked into the header too). Feel free to resolve this comment if I'm way off base.

I believe it is needed to construct a "String" pair.

Ah yes... Although this is technically true, I believe that in all or nearly all cases the code will effectively construct a new QString by concatenation (e.g. + ":Foo") (discarding this value immediately after constructing it)... Do you think it would make sense to make it more of a utility function that does this task for the developer?

So void getStringPairKeyName(StringPairDataType type, QString& stringPair)?

Kind of spitballing without knowing the code a whole lot so bear with me here...

Assuming that I'm right and that getStringPairKeyName doesn't need to construct a new QString each time it's called (and may be a candidate as a private function). 😅 then...

A utility functioncreateStringPair(StringPairDataType type, QString& value) can construct the new QString for cases where it's needed (e.g. new QString("Foo:Bar"))

copyStringPair will do the concat using createStringPair

StringPairDrag will do it's concat using createStringPair

As a result:

This will ensure that there's a contract between Clipboard and createStringPair to always create a new object.

Comparators such as decodeKey aren't creating new QStrings each time getStringPairKeyName is called.

Does that make any sense? 😅

messmerd reviewed Sep 6, 2024

View reviewed changes

szeli1 mentioned this pull request Sep 19, 2024

(Feature request) Add an option to customize the mouse/keyboard shortcuts #7506

Open

szeli1 requested a review from messmerd November 2, 2024 21:53

szeli1 added 27 commits November 2, 2024 23:09

InteractiveModelView_class_implementation

97b682f

InteractiveModelView_fixing_errors

36f1259

StringPairDrag_implementing_stringpair_enum

884964c

CMakeLists_adding_new_class

a61b4bf

FileBrowser_implementing_stringpair_enum

05aedb8

PluginBrowser_implementing_stringpair_enum

d31683a

AutomationClipView_implementing_stringpair_enum

b8fe07e

Track_adding_comment

c6d0b6b

ClipView_implementing_stringpair_enum

93da665

TrackView_implementing_stringpair_enum

55f87e9

Clipboard_adding_new_enum_items

c2c2e6e

InteractiveModelView_static_list_declaration

a278b5c

SampleClipView_implementing_stringpair_enum

0835867

AutomationEditor_implementing_stringpair_enum

19eb5a0

PatternEditor_implementing_sringpair_enum

83ffcaf

TrackContainerView_implementing_stringpair_enum

460b583

EnvelopeAndLfoView_implementing_stringpair_enum

6bb6576

InsrumentTrackWindow_implementing_stringpair_enum

3884277

PianoView_implementing_stringpair_enum

d5d87b2

AutomationTraclView_implementing_stringpair_enum

31ef8a0

TrackContentWidget_implementing_stringpair_enum

6b6020b

SampleTrackView_implementing_stringpair_enum

72f4edc

TrackOpeartionsWidget_implementing_stingpair_enum

2042f06

AutomatableButton_implemening_stringpair_enum

ad346d7

FloatModelEditorBase_implementing_stringpair_enum

6b05a09

Graph_implementing_stringpair_enum

e1e1718

Patman_implementing_stringpair_enum

05bf3f7

szeli1 added 9 commits November 2, 2024 23:09

MainWindow_adding_InteractiveModelView_focus_helper_functions

957c33d

PianoView_fixing_merge_conflict

ca37609

InteractiveModelView_changing_shortcut_class_to_struct

c0cdcf7

InteractiveModelView_updating_highlight_timer

c409413

FloatModelEditorBase_making_shortcut_array_static

42a099e

InteractiveModelView_changing_method

ee5ac8c

ClipView_making_shortcut_array_static

17fdab5

AutomationClipView_making_shortcut_array_static

5cd61a6

MainWindow_remove_qDebug

2012037

szeli1 force-pushed the widget_highlighting_and_improvements branch from 643cada to 2012037 Compare November 2, 2024 22:20

szeli1 added 3 commits November 3, 2024 07:53

Lv2Instrument_updating_StringPair_usage

c3c9d86

Lv2Instrument_fixing_error

9aad0cf

AutomationClipView_fixing_shortcuts

7ef78cd

szeli1 added 3 commits November 22, 2024 23:26

FloatModelEditorBase_adding_new_shortcuts_to_knobs

4e5fddf

InteractiveModelView_adding_support_for_NoModifier_shortcuts_without_…

979602d

…fixing_focus_issues

FloatModelEditorBase_fixing_capitalisation

1defdf4

szeli1 mentioned this pull request Jan 21, 2025

New context menu automation items #7317

Open

tresf reviewed Feb 27, 2025

View reviewed changes

qnebra mentioned this pull request Mar 1, 2025

Linked knobs does not get saved to project #7439

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Widget highlighting and shortcut system implementation #7488

Widget highlighting and shortcut system implementation #7488

szeli1 commented Sep 5, 2024 •

edited

Loading

messmerd left a comment

messmerd Sep 6, 2024

szeli1 Sep 6, 2024

tresf Feb 27, 2025 •

edited

Loading

messmerd Sep 6, 2024

szeli1 Nov 2, 2024

qnebra commented Nov 21, 2024

szeli1 commented Jan 27, 2025

Spacemagehq commented Jan 27, 2025

szeli1 commented Jan 27, 2025

Spacemagehq commented Jan 27, 2025

szeli1 commented Jan 27, 2025

szeli1 commented Jan 27, 2025

regulus79 commented Jan 27, 2025

Spacemagehq commented Jan 28, 2025

Spacemagehq commented Jan 28, 2025

Spacemagehq commented Jan 28, 2025

tresf Feb 27, 2025 •

edited

Loading

szeli1 Feb 27, 2025

tresf Feb 27, 2025 •

edited

Loading

szeli1 Feb 27, 2025

tresf Feb 27, 2025 •

edited

Loading

		@@ -65,11 +67,62 @@ namespace lmms::Clipboard
		}


		QString getStringPairKeyName(StringPairDataType type)

Widget highlighting and shortcut system implementation #7488

Are you sure you want to change the base?

Widget highlighting and shortcut system implementation #7488

Conversation

szeli1 commented Sep 5, 2024 • edited Loading

messmerd left a comment

Choose a reason for hiding this comment

messmerd Sep 6, 2024

Choose a reason for hiding this comment

szeli1 Sep 6, 2024

Choose a reason for hiding this comment

tresf Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

messmerd Sep 6, 2024

Choose a reason for hiding this comment

szeli1 Nov 2, 2024

Choose a reason for hiding this comment

qnebra commented Nov 21, 2024

szeli1 commented Jan 27, 2025

Spacemagehq commented Jan 27, 2025

szeli1 commented Jan 27, 2025

Spacemagehq commented Jan 27, 2025

szeli1 commented Jan 27, 2025

szeli1 commented Jan 27, 2025

regulus79 commented Jan 27, 2025

Spacemagehq commented Jan 28, 2025

Spacemagehq commented Jan 28, 2025

Spacemagehq commented Jan 28, 2025

tresf Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

szeli1 Feb 27, 2025

Choose a reason for hiding this comment

tresf Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

szeli1 Feb 27, 2025

Choose a reason for hiding this comment

tresf Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

szeli1 commented Sep 5, 2024 •

edited

Loading

tresf Feb 27, 2025 •

edited

Loading

tresf Feb 27, 2025 •

edited

Loading

tresf Feb 27, 2025 •

edited

Loading

tresf Feb 27, 2025 •

edited

Loading