geigi / cozy

🎧 Listen to audio books 📚 on Linux
https://cozy.sh
GNU General Public License v3.0
1.09k stars 83 forks source link

Search UI overhaul #807

Closed rdbende closed 6 months ago

rdbende commented 7 months ago

Done:

Remaining issue:

rdbende commented 7 months ago

Remaining issue:

  • Since the space key is a global shortcut for play/pause, it can't be inserted into the search bar

Disabling the action while the search view is open solved the issue.

TODO:

geigi commented 6 months ago

The new search looks really cool! Thanks for implementing it. It seems like there are two small merge conflicts but other than that it looks good to me :)

rdbende commented 6 months ago

After the PRs related to UI are merged, I should really do #801. Resolving conflicts in the XML files is such a mess :(

@naglis could you give an approval on my conflict resolution?

rdbende commented 6 months ago

Codeclimate is complaining about non-existent problems again :/

rdbende commented 6 months ago

A few things I have noticed about the search while testing locally:

  1. self._fs_monitor.get_book_online is called twice for each book on each search (IIUC once when SearchViewModel.authors property is called and once for SearchViewModel.readers property).

  2. "Hide unavailable books" setting is not respected for book results - if I have it enabled, I still get book results from storage that is offline (NOTE: AFAIR, this feature did not work at all prior to this PR). IIUC this is because in SearchViewModel.search() the books set is built directly from self._model.books, without checking if the books are online.

Fixd.

rdbende commented 6 months ago

@naglis, could you give a final approval, so that GH allows me to merge this? It blocks many other things and will cause conflicts with other PRs, so it would be good to have it in master. I can do the rest in separate pull requests if needed.

naglis commented 6 months ago

@naglis, could you give a final approval, so that GH allows me to merge this? It blocks many other things and will cause conflicts with other PRs, so it would be good to have it in master. I can do the rest in separate pull requests if needed.

@rdbende I think you need approval from someone with write/admin permissions (not sure how it is configured), as my approvals are gray.

rdbende commented 6 months ago

Huh, yeah. Never mind. Apparently this approval doesn't work if you don't have write access. I didn't know about it 😕

rdbende commented 6 months ago

Anyway, your reviews were useful, so thanks for that!

geigi commented 6 months ago

Finally merged! Thanks for the additional review @naglis and the fixes @rdbende!