lmms icon indicating copy to clipboard operation
lmms copied to clipboard

Widget highlighting

Open szeli1 opened this issue 1 year ago • 15 comments

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

How to test:

  1. Hover over a knob, hold ctrl and start dragging.
  2. Other knobs and automation clips should get highlighted meaning that they accept the data type that is being dragged.
  3. Drop the knob over an other knob. They should now be linked. The highlighting should stop.
  4. Repeat this with automation clips, midi clips, sample clips and pattern clips.
  5. After dropping a clip on an other clip, one should be the copy of the other.

Problems:

  • Linking sometimes causes a crash (this is probably a bug in the linking code), spam the ctrl + c and ctrl + v and unlink shortcuts on knobs to trigger this.
  • 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

szeli1 avatar Sep 05 '24 21:09 szeli1

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.

qnebra avatar Nov 21 '24 17:11 qnebra

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.

szeli1 avatar Jan 27 '25 20:01 szeli1

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.

Spacemagehq avatar Jan 27 '25 20:01 Spacemagehq

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?

szeli1 avatar Jan 27 '25 20:01 szeli1

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.

Spacemagehq avatar Jan 27 '25 20:01 Spacemagehq

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 avatar Jan 27 '25 22:01 szeli1

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.

szeli1 avatar Jan 27 '25 22:01 szeli1

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.

regulus79 avatar Jan 27 '25 22:01 regulus79

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 avatar Jan 28 '25 01:01 Spacemagehq

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 avatar Jan 28 '25 01:01 Spacemagehq

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.

Spacemagehq avatar Jan 28 '25 05:01 Spacemagehq

I just briefly tested this; maybe the build I'm using wasn't from the best commit, but it seems that the drag and drop connections/copying clips into other clips isn't working.

https://github.com/user-attachments/assets/0f250705-c1da-46de-a29d-722d42f53574

Also, this is just my personal opinion, but I feel like it would make more sense for the highlighting to disappear right after you release ctrl-drag. Idk, it just kind of feels like it's just staying there even after you finish the drag, and it takes a while for it to disappear.

Oh and also, this is probably unrelated, but how does linking a widget work with "Ctrl-C (2x)"? When I do Ctrl-C twice, it says "Link Widget" but I'm not sure what I'm supposed to do after that. image

regulus79 avatar Apr 22 '25 21:04 regulus79

I just briefly tested this; maybe the build I'm using wasn't from the best commit, but it seems that the drag and drop connections/copying clips into other clips isn't working.

Maybe I'm doing something wrong but everything is working for me on my build and in the linux build from github. What you showed in the video seems wrong. The highlighting behaves wrongly. Here is how it should work:

  1. you start dragging or pressing ctrl + c over a widget
  2. widgets that accept that datatype will be highlighted
  3. you drop a widget on an other or press ctrl + v over on an other widget
  4. highlighting is stopped, the datatype is pasted into the widget
  5. ctrl + v still works on other widgets

Also, this is just my personal opinion, but I feel like it would make more sense for the highlighting to disappear right after you release ctrl-drag. Idk, it just kind of feels like it's just staying there even after you finish the drag, and it takes a while for it to disappear.

It should disappear if something accepts it. Usually users start dragging something with the intent of dropping it somewhere, so this should not be a problem for 90% use cases. Maybe It would be better if it would disappear after the drag event is destroyed, but it is an edge case. It will disappear automatically after 10 seconds anyways.

Oh and also, this is probably unrelated, but how does linking a widget work with "Ctrl-C (2x)"? When I do Ctrl-C twice, it says "Link Widget" but I'm not sure what I'm supposed to do after that.

It is really simple: everything is highlighted that accepts that datatype. You can press ctrl + v on any highlighted widget such as an other Knob or an automation clip.

szeli1 avatar Apr 23 '25 08:04 szeli1

I did some quick testing... Here are some initial thoughts...

This feature seems to aim to be three things:

  • Refactor string-pair drag
  • Add a keyboard shortcut system to commonly used GUI controls
  • Add a highlighting system (although I'm not entirely sure what the highlighting is intended for, more below)

My initial glance is that we could greatly benefit from the first two items however I'm uncertain as to the specific implementation. I'll elaborate a bit:

  • Copy/Paste should be a small visual indicator of the source and target (perhaps a brief highlight informing the user of the widget that a value was taken from or applied to). This PRs current mechanism highlights a lot of widgets and I'm not sure what value that adds to the user experience.
  • Linking to an automation pattern should be as simple as looking at what type of widget value was copied and setting up the link on paste. This mimics the fact that we re-use Ctrl for copying and for linking (Well, macOS uses Alt since #2890, but I think "Command + C" is still intuitive here, no changes needed).
    • If the item being pasted to isn't an automation pattern, require an additional modifier key (e.g. Ctrl + Shift + V).
    • Why doesn't "Unlink" show/work for Automation Patterns? (Is this a one-to-many problem? Maybe we display an error if there's' more than one?)

This leaves a lot of questions:

  • Did this PR remove the hints about dragging automatable items?
  • What reason were E, Q chosen for increasing/decreasing?
    • e.g. Would it make sense to introduce the shortcut functionality first, then individual PRs that help settle on the best shortcuts so we're not reviewing so much at once?
  • Is there a way to deselect a widget once it's been highlighted? The only way I could figure out was to paste, which doesn't really make sense
  • How come sometimes a linked automation segment highlights too? Is this to display what's linked? If that's the case, why do unrelated things also get highlighted?
  • Why does this PR re-use the help text area when an action is performed? This is especially frustrating during the learning process because the help text disappears the moment a shortcut is pressed, making it harder to learn.
  • Why do all the volume and pan knobs highlight the same color when I hit Ctrl + C?
  • What gets "copied" when my mouse isn't hovering over anything?
  • Why doesn't things like Copy/Paste work on the mixer sliders, track labels, etc?

In general, I think I agree with some of @sakertooth's sentiments on Discord that this PR may need to be broken up into digestible parts so that we're not trying to review too much at once. If this is done, I would recommend that this PR is kept open and converted to draft so that it can be referenced until all desired features have open (or -- if blocking -- merged) PRs.

tresf avatar Apr 29 '25 18:04 tresf

@szeli1 In the future could you please merge master into your branches rather than rebasing on master? Once you open a PR and people start reviewing it, rebasing makes it difficult for reviewers to see what changed.

messmerd avatar May 10 '25 23:05 messmerd