Add Search Bar to Track Library

GaryCraft commented 3 weeks ago

Allows for searching specific tracks based on title, album, author Most of it is already done If any changes need to be made to merge please leave them here

Features I didn't have time to do yet

open search bar with hotkey

tranxuanthang commented 3 weeks ago

Thank you so much for your effort! The search feature for songs is one of the most requested features on my to-do list, but I haven't had time to implement it yet. Your help is greatly appreciated!

I will review this PR more thoroughly in the next few days when I have some free time. In the meantime, I have a few initial suggestions:

Tabs and Spaces

There seem to be conflicts between tabs and spaces in this PR, resulting in many unnecessary diffs that make reviewing harder. Our repository uses 2 spaces for indentation.

Tip: You can find an extension for your favorite text editor to automatically apply the tab settings from our .editorconfig file: https://github.com/tranxuanthang/lrcget/blob/main/.editorconfig

The approach of implementing search for songs

I noticed that your approach to implementing the search feature involves scanning every track ID with invoke('get_track', { trackId: id }) (https://github.com/tranxuanthang/lrcget/pull/91/commits/89a84a2f6dbcbbdd7602c987d145a1701c92e2a2#diff-d2e7ca28e16f5855ca8a670c2ab7234cdf12196e7562bf893fbe3b297197aedfR34).

This method invokes a Rust function:

https://github.com/tranxuanthang/lrcget/blob/dcec6f566096fb0ffbb71e85da882096d1d50415/src-tauri/src/main.rs#L137

And because each call introduces an overhead cost from IPC (inter-process communication), calling this too many times from JavaScript side is a bad idea. People having more than ten thousands of songs might be heavily affected (https://github.com/tranxuanthang/lrcget/issues/19).

I suggest:

Creating a new command in Rust dedicated to searching for songs.
Since LRCGET uses a local SQLite database for managing tracks, we can utilize SQLite's LIKE SQL query or its full-text search feature (https://www.sqlite.org/fts5.html) to improve performance and memory efficiency.

This will require some Rust coding. If you're not experienced with Rust, I’m happy to help implement this function on the Rust side!

GaryCraft commented 3 weeks ago

okie, I implemented this rust side

I probably should've checked a little further into the code of the project.

it is now integrated into the already existent get_track_ids tauri command to also avoid doing a separate query and re-filtering the Ids, as such, a parameter was added to said function to enable/disable filtering directly in the query, calling a different database function to either get the full ids or the query-matching ones.

additionally I added a string to the state, to avoid needing to pass the search query everytime, even when not needed. to set said string another command was added

everything else I had already added was just updated to use this newer implementation

and also used the .editorconfig

FTS

I was reading some documentation of mentioned SQLITE FTS feature, however that's currently something out of my field of expertise, so maybe later?

tranxuanthang commented 3 weeks ago

We are heading in the right direction! Still, there are some things I'd like to modify for us to move forward:

Since we are no longer using fastest-levenshtein package, could you also revert the changes in package-lock.json file?
The src-tauri/src/db.rs, src-tauri/src/library.rs and src-tauri/src/main.rs files are all getting reformatted at 4 spaces, could you keep it as 2 spaces like before?

It seems your editor is set to automatically format the whole document on save, which forces you to commit many unnecessary changes. I suggest turning this feature off first.

Furthermore, one good habit is to check each change in your commit carefully to make sure the diff of a PR is as small as possible. Some things I can detect from the current changes are:

Avoid unnecessary changes such as adding empty new lines or adding semicolons in unrelated places (;) (link and link)
Avoid reordering import or use statements (link)
Avoid reformatting if changes is not necessary for the feature (link)

And check all your staged changes in any git GUI tool (the one integrated into your favorite text editor should work fine) before committing. After pushing to the remote branch, you can also do one more final self-review at the "files changed" section of your created PR: https://github.com/tranxuanthang/lrcget/pull/91/files

tranxuanthang commented 3 weeks ago

I was reading some documentation of mentioned SQLITE FTS feature, however that's currently something out of my field of expertise, so maybe later?

Sure, using LIKE is absolutely fine and we can always do more optimization later, and I think you did a good job implementing the get_search_track_ids function 👍

additionally I added a string to the state, to avoid needing to pass the search query everytime, even when not needed. to set said string another command was added

Could you elaborate more on this situation?

GaryCraft commented 3 weeks ago

sure, I will make the pr as small as possible.

elaborating on the other thing: there was already a use of get_track_ids which did not need the use of a search query, to avoid passing the unneeded parameter every time, (because it seems tauri does not support optional arguments from their docs), I added set_search which in turn modifies the added

pub struct AppState {
  pub db: std::sync::Mutex<Option<Connection>>,
  pub player: std::sync::Mutex<Option<Player>>,
  pub active_search: std::sync::Mutex<Option<String>> // <-
}

allowing it to be used when filtering is requested

GaryCraft commented 3 weeks ago

Apologies for the large commit history, my editor got really annoying in re-indenting automatically every file upon opening it(and even more annoying, did not let me disable that), had to make changes manually (in another editor) very carefully

PR is now as small as I could get it

Everything seems to be working properly

tranxuanthang commented 3 weeks ago

It looks a whole lot better now @GaryCraft! Remember when the commit history is a bit long you can always squash or rebase multiple commits into one. But that is not required in most cases and entirely your choice.

I'll review and test your PR later tonight!

Regarding your concern:

it seems tauri does not support optional arguments from their docs

It's true that Rust functions generally don't support optional arguments. But I think using the type Option<String> for search_query should work (it is similar to using null in other languages):

pub fn get_track_ids(
    search_active: bool,
    search_query: Option<String>,
    conn: &Connection,
) -> Result<Vec<i64>>

If that doesn't, you can always check for an empty search_query string, something like search_query.is_empty() for example.

tranxuanthang commented 3 weeks ago

I've just built your version on my local machine, and it works great! The UI also looks well done (more of my feedbacks about the UI below):

I think this PR is good to be merged. I do have some more preferences though:

Related to my above comment, I don't think we really need search_active argument in get_track_ids function. For better maintainability, we also shouldn't put much UI-related states in the Rust side (you can think of the Rust side as a server code, and should be kept stateless), so I don't prefer having a set_search command either.
I noticed you implemented the debounce logic for the auto-search feature, which is very thoughtful! In my opinion, using some external packages like lodash.debounce is usually better and easier to maintain than the DIY way. Installing the whole lodash package is also fine with me since Vite can do tree shaking, as long as you only import what you need (import debounce from 'lodash/debounce').

About the search bar: While at first I preferred an always visible search input text box more (in the top left corner, for example), your choice of making a dedicated search bar does make sense, and it allows for adding more filtering and sorting options on the search bar in the future 👍. I still have a few suggestions for the search bar to simplify the search bar and make room for more features later:

Remove the Auto checkbox, and make your choice of having the auto-search feature for the user. If you are still concerned about the missing auto-search checkbox, we can add it as an option in the Configuration modal.
Place the clear search input x button inside the text input box.
In my opinion, the submit search button can be safely removed, as most users will understand to press Enter to submit the search, but keeping it is also fine.

But overall, I think your PR can be merged now, and you can make modifications later in another PR to avoid burn-out if you prefer. What do you think about this?

GaryCraft commented 3 weeks ago

I will continue to make the recommended changes in a separate PR then, to, if possible, allow for a release with the feature

tranxuanthang commented 3 weeks ago

Thank you for your big effort!

tranxuanthang / lrcget