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

Library: add keyboard short to open track properties Ctrl+Enter #4347

Merged
merged 3 commits into from
Oct 4, 2021

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Oct 2, 2021

https://bugs.launchpad.net/mixxx/+bug/1393357

In the tracks table press Ctrl+Return to show the Track Properties dialog for the selected track.
The shortcut is displayed in the track context menu (only when invoked from the tracks table).

@github-actions github-actions bot added the ui label Oct 2, 2021
@ronso0 ronso0 added library and removed ui labels Oct 2, 2021
@daschuer
Copy link
Member

daschuer commented Oct 2, 2021

Why do you pick exactly Ctrl+Enter
https://www.computerhope.com/jargon/c/ctrl-enter.htm

@daschuer
Copy link
Member

daschuer commented Oct 2, 2021

Oh I just.. just tried it here and it is the short cut for the comment button.

@daschuer
Copy link
Member

daschuer commented Oct 2, 2021

The original report suggests CTRL+i which is used in VLC player to show the metadata of the playing track and in iTunes the info of the selected track and takes a screenshot in Windows Media Player.
Firefox show the metadata of the current page.

Did you consider to use Ctrl+i instead?

@ronso0
Copy link
Member Author

ronso0 commented Oct 2, 2021

I picked Ctrl+Enter because the proposal from the report makes sense IMO:

  • Ctrl and Enter are close > can easily be used with one hand (Ctrl+i feels awkward)
  • Enter is a large key (even on Apple keyboards)
  • Enter is often used, thus easy to locate even for lazy typers (compare to i)
  • Enter has very low risk to interfere with deck keyboard shortcuts

I'd rather not look at how unrelated apps use the key.
I'll work out how to show the Shortcut without too much code duplication, so users will discover it easily.

@ronso0
Copy link
Member Author

ronso0 commented Oct 2, 2021

The shortcut is now displayed in the track menu, at least in the tracks table menu which is sufficient.
Though it would be nice to have "Double-Click" shown as shortcut in the track properties' menu to make the double click a little more discoverable.

// when the menu is invoked from the tracks table.
// The actual shortcut is set in WTrackTableView::keyPressEvent
if (m_pTrackModel) {
m_pPropertiesAct->setShortcut(QKeySequence(tr("Ctrl+Return")));
Copy link
Member

Choose a reason for hiding this comment

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

I think QKeySequence has translation under the hood, no need for() tr here.
Is there a common header where we can move it to? This allows to define it only in a single place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, QKeySequence(Qt::CTRL + Qt::Key_Return) works, too.
I'd appreciate to have one place for those shortcuts, sure. defs_urls.h could work, but then we'd need to complicate WTrackTableView::keyPressEvent to look it up, too.
Though I wonder if it's worth the hassle considering it's about one shortcut only (maybe also Del from #4330).

Copy link
Member Author

@ronso0 ronso0 Oct 3, 2021

Choose a reason for hiding this comment

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

I've put it in util/defs.h, okay?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

} else {
QTableView::keyPressEvent(event);
// If Ignore all Enter combos if any cell editor is open
if (event->key() == Qt::Key_Return &&
Copy link
Member

Choose a reason for hiding this comment

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

unlike many other keys Qt::Key_Return does not reach this function it is already used for
[PreviewDeck1],start_stop

Copy link
Member Author

Choose a reason for hiding this comment

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

oh. I had a custom kbd config in place which didn't contain this shortcut. also it does not work when I have no kbd config in .mixxx that contains it and start mixxx from the build directory. strange..
And it needs to be added to the keyboard shortcut PDF/image.

Anyway, then let's not use it. We could argue if Return is maybe better used for the double click action, not for the preview deck, but that doesn't need to happen here.

Copy link
Member

Choose a reason for hiding this comment

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

We could argue if Return is maybe better used for the double click action

I can confirm that.

@ronso0 ronso0 marked this pull request as ready for review October 4, 2021 00:34
@ronso0 ronso0 added this to the 2.4.0 milestone Oct 4, 2021
@ronso0 ronso0 added the changelog This PR should be included in the changelog label Oct 4, 2021
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.

I can confirm it works now as desired on a German Ubuntu Focal.
LGTM. Thank you.

@daschuer daschuer merged commit d7b5ad5 into mixxxdj:main Oct 4, 2021
@ronso0 ronso0 deleted the trackproperties-hotkey branch October 4, 2021 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog library ui
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants