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

Search related tracks in collection #3181

Merged
merged 21 commits into from
Oct 24, 2020
Merged

Search related tracks in collection #3181

merged 21 commits into from
Oct 24, 2020

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Oct 17, 2020

A simple yet effective extension of the track context menu for exploring and managing the library:

Screenshot from 2020-10-20 00-11-24

Searching for tracks with a similar tempo would be another option.

I have created this feature for personal usage and will not extend it myself, i.e. only provided as a starting point.

@uklotzde uklotzde added this to the 2.4.0 milestone Oct 17, 2020
@uklotzde
Copy link
Contributor Author

The CI test failures on Windows are unrelated. @Holzhaus Maybe caused by recent float fixes? These failures seem to become more frequent.

@uklotzde
Copy link
Contributor Author

I must have created a Launchpad issue a long time ago: https://bugs.launchpad.net/mixxx/+bug/1455901

@uklotzde
Copy link
Contributor Author

Update and extended. BPM and year can be added by someone else who needs it.

@ronso0
Copy link
Member

ronso0 commented Oct 17, 2020

Update and extended. BPM and year can be added by someone else who needs it.

So this could be extended to 'fuzzy BPM search' like +- 5 BPM?

@uklotzde
Copy link
Contributor Author

Update and extended. BPM and year can be added by someone else who needs it.

So this could be extended to 'fuzzy BPM search' like +- 5 BPM?

Sure. Just a few lines of code. But I am not keen on discussions about the actual range and configuration options, therefore left out in this PR.

The year is also tricky, because this is a string field. Parsing the string to a date is part of the aoide PR, I already had to do that. Too lazy to backport it.

@ronso0
Copy link
Member

ronso0 commented Oct 17, 2020

Update and extended. BPM and year can be added by someone else who needs it.

So this could be extended to 'fuzzy BPM search' like +- 5 BPM?

Sure. Just a few lines of code. But I am not keen on discussions about the actual range and configuration options, therefore left out in this PR.

Sure.

The year is also tricky, because this is a string field. Parsing the string to a date is part of the aoide PR, I already had to do that. Too lazy to backport it.

Could you please share link to the code that does it?

@uklotzde
Copy link
Contributor Author

Update and extended. BPM and year can be added by someone else who needs it.

So this could be extended to 'fuzzy BPM search' like +- 5 BPM?

Sure. Just a few lines of code. But I am not keen on discussions about the actual range and configuration options, therefore left out in this PR.

Sure.

The year is also tricky, because this is a string field. Parsing the string to a date is part of the aoide PR, I already had to do that. Too lazy to backport it.

Could you please share link to the code that does it?

TEST_F(AoideTrackExporterTest, exportDateTimeOrYear) {

The tests are both the starting point and crucial!!

@poelzi
Copy link
Contributor

poelzi commented Oct 18, 2020

Lastfm has a query similar api that returns artists with similar style. If we could form a or query, that might be interesting. Or we implement this as some fancy filter for the search query. lastfm_similar("Bakermat") would be replaced with some large artist in ['...', '..', '...', ] query. It seems you can't combine like with in.

@uklotzde
Copy link
Contributor Author

Lastfm has a query similar api that returns artists with similar style. If we could form a or query, that might be interesting. Or we implement this as some fancy filter for the search query. lastfm_similar("Bakermat") would be replaced with some large artist in ['...', '..', '...', ] query. It seems you can't combine like with in.

Extending the query capabilities is out of scope of this PR.

@uklotzde
Copy link
Contributor Author

If this PR is not reviewed in-time I will delete it and keep it as a commit in my private fork on top of all my other pending changes. The changes introduced here cause a lot of merge conflicts for me and I am not willing to resolve them for an extended period of time.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Code looks good. I'll give this a try tomorrow.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 18, 2020

I think there's room for a more innovative design that doesn't rely on navigating menus and right clicking (which isn't available on touch screens), but that can be added later as another interface for the same features. For now, adding this to WTrackMenu is an easy and obvious design for a big usability improvement, especially now that WTrackMenu is available in the decks.

src/widget/wtrackmenu.cpp Outdated Show resolved Hide resolved
src/widget/wtrackmenu.cpp Outdated Show resolved Hide resolved
src/widget/wtrackmenu.cpp Outdated Show resolved Hide resolved
src/widget/wtrackmenu.cpp Outdated Show resolved Hide resolved
@uklotzde
Copy link
Contributor Author

I need to force push to fix a preceding PR that contains unintended changes caused by conflict resolution while cherry-picking.

@uklotzde
Copy link
Contributor Author

I have also reordered and separated the menu items:

  • Musical properties
  • Artists
  • Anything else

@Holzhaus
Copy link
Member

Unfortunately the key search depends on the notation chosen in the preferences and won't work in all cases. For "Lancelot" it works, but for "Lancelot/Traditional" it doesn't.

The key text parsing is done by KeyUtils::guessKeyFromText. I haven't checked if this works as expected, out of scope. The text is provided by the Track object and should match the user's preferences.

TL;DR: I will not fix the key search, provided as is.

I think it's just a quoting issue. ~key:7A works, ~key:7A (Dm) does not. Adding quotes fixes it: ~key:"7A (Dm)".

@ronso0
Copy link
Member

ronso0 commented Oct 20, 2020

@ALL Please provide feedback on the ordering and formatting of the menu and its items. This is what matters most in the first place.

The first 2 and the last 3 are probably the most useful, and I find elements at the top or end of a list easier to find quickly than in the middle of a list.

I'd use BPM and Artist the most, and I think searching for Title is used rarely.
So I'd swap the Artist/Album Artist and Title/ Album Title rows:

Key
BPM
-----
Artist
Album Artist
Title
Album
-----
Year
Genre
Folder

@uklotzde
Copy link
Contributor Author

Unfortunately the key search depends on the notation chosen in the preferences and won't work in all cases. For "Lancelot" it works, but for "Lancelot/Traditional" it doesn't.

The key text parsing is done by KeyUtils::guessKeyFromText. I haven't checked if this works as expected, out of scope. The text is provided by the Track object and should match the user's preferences.
TL;DR: I will not fix the key search, provided as is.

I think it's just a quoting issue. ~key:7A works, ~key:7A (Dm) does not. Adding quotes fixes it: ~key:"7A (Dm)".

Thanks for the hint! I have recently removed the quotes because I thought they were not needed 🙈 Will re-add them.

@uklotzde
Copy link
Contributor Author

Unfortunately the key search depends on the notation chosen in the preferences and won't work in all cases. For "Lancelot" it works, but for "Lancelot/Traditional" it doesn't.

The key text parsing is done by KeyUtils::guessKeyFromText. I haven't checked if this works as expected, out of scope. The text is provided by the Track object and should match the user's preferences.
TL;DR: I will not fix the key search, provided as is.

I think it's just a quoting issue. ~key:7A works, ~key:7A (Dm) does not. Adding quotes fixes it: ~key:"7A (Dm)".

Fixed.

@uklotzde
Copy link
Contributor Author

@ALL Please provide feedback on the ordering and formatting of the menu and its items. This is what matters most in the first place.

The first 2 and the last 3 are probably the most useful, and I find elements at the top or end of a list easier to find quickly than in the middle of a list.

I'd use BPM and Artist the most, and I think searching for Title is used rarely.
So I'd swap the Artist/Album Artist and Title/ Album Title rows:

Key
BPM
-----
Artist
Album Artist
Title
Album
-----
Year
Genre
Folder

Done.

@uklotzde uklotzde changed the base branch from master to main October 22, 2020 14:28
Copy link
Member

@Holzhaus Holzhaus 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!

@Holzhaus Holzhaus requested a review from Be-ing October 22, 2020 19:42
// Title and grouping actions
addSeparatorBeforeNextAction = !isEmpty();
{
const auto title = track.getTitle();
Copy link
Contributor

Choose a reason for hiding this comment

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

How about searching for trailing text in parentheses and adding another menu entry without that? I think that would work for the remix/original searches in most cases. For example, if the title is "Foo (Bar Remix)", add items to search both title:"Foo (Bar Remix)" and title:"Foo"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are too many variations. I leave this (and the following discussion) to a subsequent PR after we collected some feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to start working on a class TitleParser that will be needed for this purpose ;)

@Be-ing Be-ing merged commit ac38a04 into mixxxdj:main Oct 24, 2020
@Be-ing
Copy link
Contributor

Be-ing commented Oct 24, 2020

Thanks for working on this!

@uklotzde uklotzde deleted the wtrackmenu_search_related branch October 24, 2020 20:32
@ronso0
Copy link
Member

ronso0 commented Oct 25, 2020

👍 in conjunction with Remove from Disk #3212 this is a killer tool for cleaning up my library!

only the location string shown in the Folder: : is that needed? it expands the menu a lot.

@uklotzde
Copy link
Contributor Author

+1 in conjunction with Remove from Disk #3212 this is a killer tool for cleaning up my library!

only the location string shown in the Folder: : is that needed? it expands the menu a lot.

That's exactly the reason why we need an MVP of every new feature for collecting early feedback instead of getting lost in detailed discussions about potential edge cases ;) Work continues in #3215

@ronso0
Copy link
Member

ronso0 commented Oct 25, 2020

for everyone who like me has to lookup MVP: minimum viable product

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build changelog This PR should be included in the changelog library ui
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants