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

Multisearch #469

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

Multisearch #469

wants to merge 16 commits into from

Conversation

michalmoc
Copy link
Contributor

Attempt at #461. I'm not sure about looks and the search engine is abysmal, but in general it works. When ready, I think the single searchers could be removed to clean the interface.

@epoupon
Copy link
Owner

epoupon commented May 17, 2024

Thanks for your attempt! Will check this.
Have you tried to look at the demo site? It is still at an older lms version that had what I believe you call a "multisearch" search

@@ -151,4 +159,111 @@

return res;
}

void showReleaseInfoModal(db::ReleaseId releaseId)

Check warning

Code scanning / CodeQL

Poorly documented large function

Poorly documented function: fewer than 2% comments for a function of 106 lines.

namespace
{
AnyMediumId fromString(const std::string &type, const Wt::Dbo::dbo_default_traits::IdType id)

Check notice

Code scanning / CodeQL

Unused static function

Static function fromString is unreachable
@epoupon
Copy link
Owner

epoupon commented May 22, 2024

I managed to test your branch. Wow, looks like there are a lot of changes!

Here are my remarks:

  • You added an album list view, this is interesting and could be also added in the main releases view (list mode / card mode). For some reason, it looks like I get the list of all the track artists (instead of the release artists), this bloats the interface.
  • You changed the artist list view to append a cover, this is a good idea and can be also used in the artist view (possibly also with a switch list mode / card mode)
  • Unfortunately, the database query SELECT type as col0, id as col1, sum(weight) AS v FROM keywords where (value LIKE ? ESCAPE '\') group by type, id order by v DESC limit ? offset ? is really slow (like 0.5s on my 50k tracks test database, it will likely not be acceptable on the 1M tracks test database.
  • I am a bit confused about the artists/releases/tracks being all mixed together in the results. It can take a while to scroll down to eventually find the artist / release we want. I think the approach that was implemented before is more convenient (separate results for each type). Have you seen the search bar of the demo website? https://lms-demo.poupon.dev. It also has the advantage to use the existing queries and only requires some extra UI code (the old search view should be easy to resurrect).

Thanks again for the nice ideas!

@michalmoc
Copy link
Contributor Author

You added an album list view, this is interesting and could be also added in the main releases view (list mode / card mode). For some reason, it looks like I get the list of all the track artists (instead of the release artists), this bloats the interface.

Could be used, but this is work for some other time. Regarding track/album artist, I'll check it later, I'm not sure how to handle both types yet.

You changed the artist list view to append a cover, this is a good idea and can be also used in the artist view (possibly also with a switch list mode / card mode)

There are quite a lot of code duplications added on this branch, I'll try to follow on it later.

Unfortunately, the database query SELECT type as col0, id as col1, sum(weight) AS v FROM keywords where (value LIKE ? ESCAPE '\') group by type, id order by v DESC limit ? offset ? is really slow (like 0.5s on my 50k tracks test database, it will likely not be acceptable on the 1M tracks test database.

This is probably because Views are in no way cached in Sqlite. We'll probably need to make keywords into proper table, but it will come at the cost of memory and potential desyncs. But most definitely will be needed, I'm still working on search engine and the next version will be far more expensive. Am I correct to assume that database changes only during scan and we can use this moment to cache keywords?

I am a bit confused about the artists/releases/tracks being all mixed together in the results. It can take a while to scroll down to eventually find the artist / release we want. I think the approach that was implemented before is more convenient (separate results for each type). Have you seen the search bar of the demo website? https://lms-demo.poupon.dev. It also has the advantage to use the existing queries and only requires some extra UI code (the old search view should be easy to resurrect).

I've seen it, but still comes with annoying click to switch result type, which I'm striving to avoid. Search results are supposed to be weighted, so all tracks of artist should appear below him (unless of course track names contain the same keywords).

Making results look clean and readable is a job for ui designer, which sadly I'm not; I thought about distinguishing entities by icons and colours, but not sure. Maybe coulour frame for cover? or coloured names? Will toy with it later.

Ultimately, there shouldn't be any problem with keeping both search approaches and guarding them with configuration.

@epoupon
Copy link
Owner

epoupon commented May 23, 2024

You added an album list view, this is interesting and could be also added in the main releases view (list mode / card mode). For some reason, it looks like I get the list of all the track artists (instead of the release artists), this bloats the interface.

Could be used, but this is work for some other time. Regarding track/album artist, I'll check it later, I'm not sure how to handle both types yet.

Yes, this is more to better factorize the code.

You changed the artist list view to append a cover, this is a good idea and can be also used in the artist view (possibly also with a switch list mode / card mode)

There are quite a lot of code duplications added on this branch, I'll try to follow on it later.

Indeed :(

Unfortunately, the database query SELECT type as col0, id as col1, sum(weight) AS v FROM keywords where (value LIKE ? ESCAPE '\') group by type, id order by v DESC limit ? offset ? is really slow (like 0.5s on my 50k tracks test database, it will likely not be acceptable on the 1M tracks test database.

This is probably because Views are in no way cached in Sqlite. We'll probably need to make keywords into proper table, but it will come at the cost of memory and potential desyncs. But most definitely will be needed, I'm still working on search engine and the next version will be far more expensive. Am I correct to assume that database changes only during scan and we can use this moment to cache keywords?

What do you mean by cache keywords? You want to extract the words of the titles, names, etc. and search by exact keyword match?

I am a bit confused about the artists/releases/tracks being all mixed together in the results. It can take a while to scroll down to eventually find the artist / release we want. I think the approach that was implemented before is more convenient (separate results for each type). Have you seen the search bar of the demo website? https://lms-demo.poupon.dev. It also has the advantage to use the existing queries and only requires some extra UI code (the old search view should be easy to resurrect).

I've seen it, but still comes with annoying click to switch result type, which I'm striving to avoid. Search results are supposed to be weighted, so all tracks of artist should appear below him (unless of course track names contain the same keywords).

I see. Maybe we could also consider putting some artists first, then some releases, then some tracks? We could add a "show more" button after each section that makes the view focus on the selected type and add more results using an infinite scroller. I guess for most searches we would not even need to click.

Making results look clean and readable is a job for ui designer, which sadly I'm not; I thought about distinguishing entities by icons and colours, but not sure. Maybe coulour frame for cover? or coloured names? Will toy with it later.

Ultimately, there shouldn't be any problem with keeping both search approaches and guarding them with configuration.

I do agree with this, the current "Search..." fields are actually "filters"

@michalmoc
Copy link
Contributor Author

What do you mean by cache keywords? You want to extract the words of the titles, names, etc. and search by exact keyword match?

More or less, I want to create "middle results", which are cached, and then can be searched for cheap. It's already done in the form of view in Session.cpp, but since views are not cached it does nothing for performance. I intend to make it into proper table, but I'm not sure at which points it should be recalculated.

I see. Maybe we could also consider putting some artists first, then some releases, then some tracks? We could add a "show more" button after each section that makes the view focus on the selected type and add more results using an infinite scroller. I guess for most searches we would not even need to click.

The results are sorted by weight, and hit in direct name is scored far higher than matching artist. I hope it's enough for the target result to come on top. Your proposal is more or less what Spotify does, to which I personally am not convinced, I often look into wrong section and wonder where are my results. Still, it's all matter of taste, perhaps adding configuration options would be for the best.

@michalmoc
Copy link
Contributor Author

Fixed the release/track artist issue, though now some releases may have none.

@epoupon
Copy link
Owner

epoupon commented May 25, 2024

What do you mean by cache keywords? You want to extract the words of the titles, names, etc. and search by exact keyword match?

More or less, I want to create "middle results", which are cached, and then can be searched for cheap. It's already done in the form of view in Session.cpp, but since views are not cached it does nothing for performance. I intend to make it into proper table, but I'm not sure at which points it should be recalculated.

I see. Maybe we could also consider putting some artists first, then some releases, then some tracks? We could add a "show more" button after each section that makes the view focus on the selected type and add more results using an infinite scroller. I guess for most searches we would not even need to click.

The results are sorted by weight, and hit in direct name is scored far higher than matching artist. I hope it's enough for the target result to come on top. Your proposal is more or less what Spotify does, to which I personally am not convinced, I often look into wrong section and wonder where are my results. Still, it's all matter of taste, perhaps adding configuration options would be for the best.

I checked also Apple music: it has a first popup with mixed results:
image
And then when you hit enter, you have this:
image
"Top results" seems to show some mixed results, then there are other categories like my proposal
Clicking on Artists/Albums etc. shows a dedicated page with the name filter active.

Spotify :
image

When you type in something, the view is automatically refreshed, the 'All' tab is active and you have 'top results', 'albums' etc. If you select another tab, this is like the current lms demo server.

So yes, a 'top results' (or whatever the name) would be nice, but I have the feeling we also want to filter by artist/albums/songs/playlists.

The search view could be something like this: tabs All, Albums, Artists, Tracks, [Playlists], With All selected by default and be what you try to do.

# Conflicts:
#	src/libs/database/impl/Migration.cpp
#	src/libs/database/include/database/ArtistId.hpp
#	src/libs/database/include/database/ClusterId.hpp
#	src/libs/database/include/database/IdType.hpp
#	src/libs/database/include/database/ListenId.hpp
#	src/libs/database/include/database/User.hpp
#	src/lms/ui/LmsApplication.cpp
#	src/lms/ui/Utils.hpp
#	src/lms/ui/explore/ArtistsView.cpp
#	src/lms/ui/explore/DatabaseCollectorBase.hpp
#	src/lms/ui/explore/Explore.cpp
#	src/lms/ui/explore/ReleaseHelpers.hpp
#	src/lms/ui/explore/ReleaseView.cpp
#	src/lms/ui/explore/ReleasesView.cpp
#	src/lms/ui/explore/TracksView.cpp
#	src/lms/ui/resource/CoverResource.hpp
@michalmoc
Copy link
Contributor Author

@epoupon Could you test it again? I've implemented your remarks as much as I could.

@epoupon
Copy link
Owner

epoupon commented May 29, 2024

@epoupon Could you test it again? I've implemented your remarks as much as I could.

Hello! Thanks for your work. Will try to have a look on it very soon.
Maybe to iterate faster I can push cleans/changes directly on your branch?

@michalmoc
Copy link
Contributor Author

@epoupon Could you test it again? I've implemented your remarks as much as I could.

Hello! Thanks for your work. Will try to have a look on it very soon. Maybe to iterate faster I can push cleans/changes directly on your branch?

Sure, I've added you as a collaborator to my fork, hope it allows you to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants