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.

Sign up for GitHub

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

Jump to bottom

LayeredMenu: allows multiple presses on a single shortcut to cycle #3209

Open
wants to merge 2 commits into
base: community
Choose a base branch
from
Open

LayeredMenu: allows multiple presses on a single shortcut to cycle #3209

wants to merge 2 commits into from
+608 −158

Conversation

nikodemus
Copy link
Collaborator

@nikodemus nikodemus commented Jan 4, 2025 •
edited
Loading

  • replaces the SELECT press changing unison count to unison stereo spread: does the same thing, but works by pressing the same shortcut again.

  • the same mechanism should be usable as-is for eg. providing fast access to new arp parameters.

the second commit adds envelopes 3 & 4 using this mechanism. these commits are best reviewed separately

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 4, 2025 •
edited
Loading

Test Results

106 tests  ±0   106 ✅ ±0   0s ⏱️ -1s
 16 suites ±0     0 💤 ±0 
 16 files   ±0     0 ❌ ±0 

Results for commit 33679ce. ± Comparison against base commit e936bb8.

♻️ This comment has been updated with latest results.

@ok-reza
Copy link
Collaborator

ok-reza commented Jan 4, 2025

nice ! feels good
im noticing that the shortcut's pad doesn't blink when the 1st or 2nd shortcut is in focus. i was also thinking that shortcuts with multiple pages will either:

  1. blink a different color to tell user there's multiple layers (eg. yellow instead of white).
  2. blink white but add another color (eg. yellow) within the sequence of blinking. to leave a breadcrumb that another layer exists, without completely departing from white pad blinks with shortcuts.

not sure how necessary or QoL improving this would be, but maybe entering the 2nd shortcut should occur on release instead of on press.
it's honestly just a bad habit/unnecessary muscle memory of mine, but if i'm already on say LPF freq shortcut, sometimes i still press and hold shift+lpf (despite already being in focus) then add env1 to ensure i'm modulating lpf with env1. it's not necessary as all you need to do is hit shift+env1 to make the modulation, but i guess my logic is that it feels ike im making a connection between two shortcuts in one action.

@nikodemus
Copy link
Collaborator Author

not sure how necessary or QoL improving this would be, but maybe entering the 2nd shortcut should occur on release instead of on press. it's honestly just a bad habit/unnecessary muscle memory of mine, but if i'm already on say LPF freq shortcut, sometimes i still press and hold shift+lpf (despite already being in focus) then add env1 to ensure i'm modulating lpf with env1. it's not necessary as all you need to do is hit shift+env1 to make the modulation, but i guess my logic is that it feels ike im making a connection between two shortcuts in one action.

Hm. relase vs press would be quite tricky right now, esp. if we don't want to enter normal shortcuts on release.

Another option would be to eg. make TRIPLET VIEW change the layer when in shortcut.

@nikodemus
LayeredMenu: allows multiple presses on a single shortcut to cycle
03041c1 
- replaces the SELECT press changing unison count to unison stereo spread:
  does the same thing, but works by pressing the same shortcut again.

- the same mechanism should be usable as-is for eg. providing fast access
  to new arp parameters, and has been tested to work with horizontal menus
  as well. (no combined horizontal and layered menus are part of this PR,
  cominng after)

- LayeredShortcut is a MenuItem class that encapsulates other menu items,
  switching between them via SoundEditor calling MenuItem::nextLayer().

- Moved some Submenu methods to MenuItem, so that we can put both Submenus
  and LayeredShortcuts in the parents tables. Some Submenu items made explicitly
  final.

- SoundEditor navigation stack cleanup.

- Removed const qualifiers from SoundEditor that were only getting cast away,
  making the code harder to follow.

- Replaced NO_NAVIGATION with a proper tombstone object, so it can have methods
  called on it.

- minor cleanup in sound_editor.cpp: we had `const MenuItem*` in a couple
  of places, where we then had to explicitly cast the constness away --
  better not have it in the first place.
@nikodemus
envelopes 3 & 4
33679ce 
- pressing any of the envelope 1 or 2 segments while the same
  segment is already active opens the corresponding env 3 or 4
  segment
@nikodemus
Copy link
Collaborator Author

As, forgot to fix the blink...

@seangoodvibes
Copy link
Collaborator

Need to add some docs for those extra envelopes!

@nikodemus
Copy link
Collaborator Author

Missing bits in addition to the blink missing:

  • env 3 & 4 aren't available as patch source shortcuts yet
  • didn't test how the regular menu access to them works

Additionally, the layering might be confusing on 7-seg. Maybe making it so that holding shift in menu shows the item name on 7-seg (or need to hold shift to see the value & edit?) That could be combined with allowing horiz menu - like access to neighbouring params when the name is shown? @m-m-adams and other 7-seg users?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nikodemus @ok-reza @seangoodvibes