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

Searchbox: Enter jumps to tracks if search query was transmitted #4844

Merged
merged 2 commits into from
Jul 11, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jul 6, 2022

1st Enter press: emit query (or debounce timeout expired)
2nd Enter press: jump to tracks table

This is mainly supposed to be a shortcut for when working with the keyboard. Currently it requires 3 x Tab to get from searchbox to tracks, while it's just one press of the Browse/Trax encoder, often mapped to [Library], GoToItem.

I'm planning to add some visual feedback to signal that the query was submitted, it's not clear what that'll be though.

  • could be a another pushbutton next to Clear, e.g. ➜, that's greyed out as soon as the search is triggered, though that's cumbersome to integrate (layout-wise, considering we also expect right-to-left locales)
  • could be a 3rd state of the Clear icon (normal | focused | query submitted)
  • ...

@ronso0 ronso0 added the library label Jul 6, 2022
@github-actions github-actions bot added the ui label Jul 6, 2022
@daschuer
Copy link
Member

daschuer commented Jul 6, 2022

This sounds like a useful feature.

I have not tested yet, but I assume that the current search is replaced by the saved search in tracks after the switch. Is this the case?

What do you think about the status widget idea below the search bar, with a button that applies the same search for all tracks?

@ronso0
Copy link
Member Author

ronso0 commented Jul 6, 2022

Oh, looks like I need to clarify

2st Enter: jump to tracks table

not to "Tracks" feature. Selected feature is unchanged, this is just an alternative to 3 x Tab (> Clear button > sidebar > tracks table)

@daschuer
Copy link
Member

daschuer commented Jul 7, 2022

Ah, I see, I have misread the PR. Just tested and it works. I don't think that this feature requires a visual feedback.

However my personal main issue is the missing workflow to search in the Tracks feature after browsing the library tree.

Can we merge this as it is?

@ronso0
Copy link
Member Author

ronso0 commented Jul 7, 2022

Can we merge this as it is?

I adjusted the tooltip, so: yes!

@@ -207,6 +208,8 @@ void WSearchLineEdit::setup(const QDomNode& node, const SkinContext& context) {
tr("Shortcuts") + ": \n" +
tr("Ctrl+F") + " " +
tr("Focus", "Give search bar input focus") + "\n" +
tr("Return") + " " +
tr("Trigger search, next press jumps to tracks view") + "\n" +
Copy link
Member

Choose a reason for hiding this comment

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

Is this still correct? In my test the search is instantly applied.
And return immediately moves to the Tracks table.

Copy link
Member Author

Choose a reason for hiding this comment

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

When did that happen?
I tested with a long debounce timeout, 1500ms, and with that it worked as expected in all situations (typing query or selecting a saved query with Up/Down keys). Exception: picking a query from the history drop-down would apply that immediately.

Copy link
Member

@daschuer daschuer Jul 7, 2022

Choose a reason for hiding this comment

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

Ah, so it is the long debounce time that eats the first return.
How about this:

Suggested change
tr("Trigger search, next press jumps to tracks view") + "\n" +
tr("Triggers search before search-as-you-type timeout or jumps to results afterwards") + "\n" +

Copy link
Member

Choose a reason for hiding this comment

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

Did you consider this comment?

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, added it now. Thanks!

@daschuer
Copy link
Member

daschuer commented Jul 7, 2022

Please remove the draft state when done.

@ronso0 ronso0 marked this pull request as ready for review July 7, 2022 08:28
Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>
@ronso0 ronso0 force-pushed the search-jump-to-tracks branch from 6911d1f to 0603fad Compare July 10, 2022 19:42
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.

LGTM, Thank you.

@daschuer daschuer merged commit d53e0b0 into mixxxdj:main Jul 11, 2022
@ronso0 ronso0 deleted the search-jump-to-tracks branch July 11, 2022 07:10
@ronso0 ronso0 added this to the 2.4.0 milestone Aug 5, 2022
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