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

add control for showing a deck's track menu #10825

Merged
merged 4 commits into from
Jan 27, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Aug 26, 2022

allows toggling a deck's track menu with controllers via deck, show_track_menu (read/write).
For now, deck is restricted to main decks (group name [ChannelN])

This is an adaption of [Library], show_track_menu.
While the library menu requires a WTrackTableView instance, the deck control requires at least one WTrackProperty for each deck this control should be used with.

The tricky part was to allow only one open track menu per deck at a time. Check the comments for details.

For now, this is just a proposal. If there is interest to merge this I'm happy to receive reviews.

@ronso0 ronso0 force-pushed the track-menu-per-deck branch from 5204422 to c2eb785 Compare August 28, 2022 23:13
@ronso0 ronso0 force-pushed the track-menu-per-deck branch 3 times, most recently from 238b9d8 to 2b0c853 Compare September 10, 2022 20:48
@ronso0 ronso0 removed the library label Sep 10, 2022
@ronso0
Copy link
Member Author

ronso0 commented Sep 11, 2022

This works nicely now.
Mapped it for all decks to Shift+Pfl, and now I can delete obsolete track files and search related tracks from the controller with Pfl and the Trax turn/push encoder : )

@daschuer
Copy link
Member

This is still in draft state, is this still the case?

Maybe the surprising behavior happens because of every WTrackProperty is connected to the new CO.
Did you consider to only connect the first?

I have not debugged it, but I can imagine that Mixxx tries to open many menus at once.

@ronso0
Copy link
Member Author

ronso0 commented Sep 18, 2022

This is a draft because it's still a proposal.

Please take a look at the code first. There are no surprises, it's just working.

  • The first WTrackProperty that is parsed creates the push button and deals with the CO change requests.
  • All other WTrackProperty widgets only connect the WTrackMenu's open/closed signal to a proxy.

This way there can be only one track menu per deck at a time:

  • either opened by right-click on any WTrackProperty
  • or triggered by the CO, then it's the menu of the first WTrackProperty (which doesn't need to be visbile btw, it should only be on the same side of the mixer as the other widgets)

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Dec 18, 2022
@ronso0
Copy link
Member Author

ronso0 commented Dec 18, 2022

works great btw. I have now mapped it to deck Load button (press+hold) + press Trax/Browse.
Since it is possible to open all deck (and library) track menus at once I made the script close all existing menus, then open the deck menu.
Pretty pleasant ux imo.

My main use cases are still the Search Related menu and Remove from disk (5 button presses/turns to delete a track vs fiddling with the trackpad), and deck Properties which I can then easily navigate with the keyboard.

@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Dec 19, 2022
@github-actions
Copy link

github-actions bot commented Jun 5, 2023

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jun 5, 2023
@ronso0 ronso0 removed the stale Stale issues that haven't been updated for a long time. label Jun 5, 2023
@ronso0 ronso0 marked this pull request as ready for review August 17, 2023 22:24
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry that this is now conflicting. I have added some comments.

src/widget/wtrackproperty.cpp Outdated Show resolved Hide resolved
src/widget/wtrackproperty.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Jan 25, 2024

I'm running into the same issue as with stars_up/_down (see #12591):
the ControlPushButtons are not destroyed on skin reload, i.e. they are not connected to the slot WTrackProperty::slotShowTrackMenuChangeRequest.

I noticed this with my personal branch but I didn't notice the warnings.

Therefore: back to the drawing board. I'll try moving the ControlPushButton creation to BaseTrackPlayerImpl.

@ronso0 ronso0 marked this pull request as draft January 25, 2024 17:28
@ronso0 ronso0 force-pushed the track-menu-per-deck branch 2 times, most recently from dd7fb23 to ec637fa Compare January 26, 2024 00:18
@ronso0
Copy link
Member Author

ronso0 commented Jan 26, 2024

Overhauled and rebased to avoid the CO issue I encountered with the stars COs.

Works great now!

As usual, the last commit is for testing purposes only ; )
A MIDI mapping for testing with a virtual device is here https://gist.github.com/ronso0/78d506b0a0462dcdcbceb4f0453d72a8
Also watch [Channel1],show_track_menu in dev tools while testing.

@ronso0 ronso0 force-pushed the track-menu-per-deck branch from ec637fa to ea48c37 Compare January 26, 2024 00:52
[ChannelN],show_track_menu

If show is requested all other track menus are closed.
Since this is not triggered by a mouse click there is no base point for the menu to be displayed.
The center of the first WTrackProperty widget is used instead, even if it's invisible.
@ronso0 ronso0 force-pushed the track-menu-per-deck branch from ea48c37 to de05b71 Compare January 26, 2024 10:44
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works and locks good. Please remove the test commit than I will merge.

@ronso0 ronso0 force-pushed the track-menu-per-deck branch from de05b71 to 67a3a95 Compare January 27, 2024 00:39
@ronso0
Copy link
Member Author

ronso0 commented Jan 27, 2024

Done.

Great, I can skip more and more commits when rebasing my personal branch onto main : )

@daschuer daschuer marked this pull request as ready for review January 27, 2024 10:55
@daschuer
Copy link
Member

Thank you.

@daschuer daschuer merged commit a7af572 into mixxxdj:main Jan 27, 2024
12 checks passed
@ronso0 ronso0 deleted the track-menu-per-deck branch January 27, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants