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

Feat 52 search by genre #80

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Feat 52 search by genre #80

wants to merge 6 commits into from

Conversation

xxxserxxx
Copy link
Collaborator

This is forked off issue-76-clarify-search-results!!!

It's branched here because, otherwise, the merge for both will be nasty. It's a separate branch, because it adds a feature.

This implements #52.

  • It works better with the feature that focuses the columns rather than the search field.
  • It's in the search page; this is largely to prevent the proliferation of pages
  • Pressing 'g' in any column toggles between genre search, and a general search3 search
  • Genre searches only return songs (that's an API thing). This means the artist and album fields are unused, which leads to....
  • in genre-search mode, the album field is renamed to "genres" and is filled with a list of genres the server knows, pulled by /getGenres
  • Selecting a genre from the middle column is the same as searching for that genre, and fills the songs column with songs with that genre.
  • Genre searches are case sensitive, on both gonic and Navidrome.
  • An entire genre can be added to the queue with a

I try to keep commits focused, but this commit changes a couple of other (unrelated) things:

  • I added Year to the song info metadata. I originally wanted to include the genre, but most of the API calls returning song info do not include the genre with the song; when included, it's in the album info. A work-around would have been far more invasive, so I've put it off, but I kept the Year change.
  • The Queue page now show the size of the queue in the column title. I originally did this to test that queue population was adding the right number of things, and because
  • I started expanding column counts in the search page, and liked it. I think ultimately I'd like to add it to all lists.

You should be able to merge "clarify" and then merge this branch; or if you decide to merge both, just merge this branch and you'll get both.

@xxxserxxx
Copy link
Collaborator Author

@spezifisch There are conflicts between my PRs, because I keep forking them off main and they often overlap files. When it's too obvious to me that merges are going to be hard this way, I'll branch off another of my unmerged branches, but this has its own problems.

I have a suggestion: when you're next ready, tell me which of the PRs you would like to merge. I'll create a merge branch off main, merge and resolve the conflicts, and you can take a look at it, test run it, and pull them in at once without having to deal with the conflicts.

@spezifisch
Copy link
Owner

I have a suggestion: when you're next ready, tell me which of the PRs you would like to merge. I'll create a merge branch off main, merge and resolve the conflicts, and you can take a look at it, test run it, and pull them in at once without having to deal with the conflicts.

Let's try that. I've just been through PRs and want to weed out the performance bugs on my setup next.
This PR here is the one I would merge next.

@xxxserxxx
Copy link
Collaborator Author

Let's try that. I've just been through PRs and want to weed out the performance bugs on my setup next. This PR here is the one I would merge next.

Yeah, that was an ugly one.

I rebased the branch on the current main; it passes make all and I cleaned up a couple of column content things in 5cac4ac. Then I did a test merge back onto main with no issues.

I have yet to remember to sign a commit :-/

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.

2 participants