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 track move commands to controls library #13098

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

nlw0
Copy link

@nlw0 nlw0 commented Apr 14, 2024

Add track move to controls library by emulating alt+up/down. Depends on #13092

@nlw0 nlw0 marked this pull request as draft April 14, 2024 09:29
Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

Please sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

Code LGTM, just a comment

Comment on lines 834 to 835
emitKeyEvent(QKeyEvent{
QEvent::KeyPress, key, Qt::AltModifier, QString(), false, times});
Copy link
Member

Choose a reason for hiding this comment

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

unfortunately emitKeyEvent requires the library to be focused, which would limit the use cases (same with the current navigation controls, see #9294, #11285 and others).

Let's send the key event to m_pLibraryWidget directly as this is the only widget that would accept this event (check if is not null)

Copy link
Author

Choose a reason for hiding this comment

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

All right, I couldn't test yet, does the code look right?

@nlw0
Copy link
Author

nlw0 commented Apr 15, 2024

Sure. I believe I signed it already, though.

@nlw0 nlw0 force-pushed the nic/track-move-controls branch from 9773a31 to cb745eb Compare April 16, 2024 06:33
Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

LGTM, just the names need to be changed.

src/library/librarycontrol.cpp Outdated Show resolved Hide resolved
src/library/librarycontrol.cpp Show resolved Hide resolved
@ronso0
Copy link
Member

ronso0 commented Apr 16, 2024

pre-commit is complaining (didn't take a closer look)

@nlw0 nlw0 marked this pull request as ready for review April 16, 2024 19:45
@nlw0
Copy link
Author

nlw0 commented Apr 16, 2024

So I was finally able to test and QApplication::sendEvent did not work the way this is, although it worked when I moved back to emitKeyEvent. What could I be missing?

@ronso0
Copy link
Member

ronso0 commented May 30, 2024

Sorry, my mistake:
m_pLibraryWidget is the stacked widget that holds all WTrackTableViews, it won't handle the keypress.

I'll prepare a commit that allows to send the keypress to the current view (if it's a WTrackTableView).

src/library/librarycontrol.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member

ronso0 commented May 30, 2024

If you adopt my code suggestion, then rebase (and squash commits) onto main 2.5 then there is WLibrary::getCurrentTrackTableView() available, and you can pick (and also squash) this commit
ab8c1d2
Works nicely!

@ronso0
Copy link
Member

ronso0 commented Jun 17, 2024

@nlw0 Do you have time to rebase, cherry-pick and squash as I suggested above?

Let us know if you don't, then I'd take over soonish in order to get this into 2.5 along with the keyboard improvement.

@nlw0
Copy link
Author

nlw0 commented Jun 18, 2024

Hi, I can look into it again, but I wasn't able to use that commit, how do I check it out? On GitHub it says This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

@nlw0
Copy link
Author

nlw0 commented Jun 18, 2024

The SlotMoveTrack commands don't seem to be on the 2.5 branch yet, should I wait for another branch to be merged first?

@ronso0
Copy link
Member

ronso0 commented Jun 18, 2024

yes, the Alt moves are available in 2.5 https://github.com/mixxxdj/mixxx/blob/2.5/src/widget/wtracktableview.cpp#L867

I think you need to git fetch [mixxx upstream], you'll see that 2.5 has been updated since your last commit.
Just squash your commits so we have only one essential commit with extra the CI fixup commits, then rebase. Since this is a PR commit that should work flawlessly.

@nlw0
Copy link
Author

nlw0 commented Jun 18, 2024

All right I haven''t tested yet but here it is. Hope I didn't mess up anything.

@nlw0 nlw0 changed the base branch from main to 2.5 June 18, 2024 15:48
@ronso0
Copy link
Member

ronso0 commented Jun 18, 2024

Yep, looks good, thank you!

@ronso0 ronso0 merged commit 55458bb into mixxxdj:2.5 Jun 18, 2024
1 check passed
@ronso0
Copy link
Member

ronso0 commented Jun 18, 2024

Could you also take care of documenting the new controls in the manual? : )
https://manual.mixxx.org/2.5/en/chapters/appendix/mixxx_controls.html#the-library-controls

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