kiwix / kiwix-desktop

Kiwix for Windows and GNU/Linux desktops

https://download.kiwix.org/release/kiwix-desktop/

GNU General Public License v3.0

778 stars 102 forks source link

UI for selecting the book to be used for title search #1218

Closed ShaopengLin closed 3 weeks ago

ShaopengLin commented 1 month ago

Fixes (part of) #315

Relies on #1189

Changes:

Added MultiZim button to SearchBar. Ctrl + Shift + L to open popup.
Searching is now allowed on library, settings, and blank tabs.
Popup shows a list of Zims that could be searched. The selection is singular as we do not have *true multi-zim search (yet).
Users can now search a different zim than the current tab.
Added placeholder UI for 'select-all' button. Hidden for now until *true multi-zim search is implemented.

kelson42 commented 4 weeks ago

@ShaopengLin I guess this is the next one to complete the work on the new searchbar?

ShaopengLin commented 4 weeks ago

@kelson42 This is the next change.

@veloman-yunkan Depending on how #1230 discussion goes we can combine them here.

veloman-yunkan commented 3 weeks ago

@veloman-yunkan Depending on how #1230 discussion goes we can combine them here.

@ShaopengLin This PR is about Multi-ZIM search which has nothing to do with #1230. Please provide a bugfix for #1230 as a separate PR.

ShaopengLin commented 3 weeks ago

@veloman-yunkan The code ios ready for review.

veloman-yunkan commented 3 weeks ago

Before looking at the code I tested the new feature and here is my feedback:

The most obvious new possibility that could be enabled by the new button is not working - searching while on the library page.
The list of ZIMs doesn't seem to be sorted in any particular order.
The new UI (like probably a lot of our old UI) requires a mouse - it cannot be used via keyboard only
Are there plans to support filtering the list of ZIMs shown in the ZIM selector for search? Currently it shows the full list of ZIMs. Maybe it should be in sync with the library ZIM file filter?

veloman-yunkan commented 3 weeks ago

Fixes (part of) #413

413 is unrelated to multi-zim search (and probably can already be closed). Doesn't this PR rather address part of #315?

ShaopengLin commented 3 weeks ago

@veloman-yunkan Kelson decided to merge the two together back here https://github.com/kiwix/kiwix-desktop/issues/413#issuecomment-2283072087. What we have here isn't true multi-Zim, since we can only search one zim at a time.

That particular problem has been there since the search bar introduction. Searching also doesn't work on blank tabs, which I believe was mentioned in some issues but not sure where. Likely need to do this fix separately.
Sure I can sort it. Currently, it's ordered in the same way it appears in the Local Files library.
I will investigate and re-commit.
One problem I see is there isn't a strong correlation for users to think library tab filters are applied to multi-zim. It can be hard to comprehend and create bad UX. Adding filters will likely need to be done within multi-zim.

veloman-yunkan commented 3 weeks ago

That particular problem has been there since the search bar introduction. Searching also doesn't work on blank tabs, which I believe was mentioned in some issues but not sure where. Likely need to do this fix separately.

Then it made sense because search was supposed to work only for the current book and there was no current book in the library view. Now that you can select the book to search in via dedicated UI such behavior is no longer justified.

veloman-yunkan commented 3 weeks ago

@veloman-yunkan Kelson decided to merge the two together back here #413 (comment). What we have here isn't true multi-Zim, since we can only search one zim at a time.

413 and #315 are about completely different features. This PR is definitely a step toward #315 rather than the final touch of #413.

ShaopengLin commented 3 weeks ago

@kelson42 We should mark this PR for #315 instead if we would like to keep that Issue.

kelson42 commented 3 weeks ago

@kelson42 We should mark this PR for #315 instead if we would like to keep that Issue.

Sorry but I'm really lost now about what fixes what. As long as the issues are implemented at the end, I'm fine with it.

ShaopengLin commented 3 weeks ago

@veloman-yunkan

Keyboard controls should be available for the MultiZim popup now.
searching in library page is now working.
Sorted by alphabetic order.

veloman-yunkan commented 3 weeks ago

I changed the title of this PR. Can we also rename the button from "Multi-Zim search" to "Search Options"?

ShaopengLin commented 3 weeks ago

@veloman-yunkan

Renamed button
Resolved https://github.com/kiwix/kiwix-desktop/pull/1218#discussion_r1826933049 and https://github.com/kiwix/kiwix-desktop/pull/1218#discussion_r1826579991

ShaopengLin commented 3 weeks ago

I just realized the shortcut I gave search options is supposed to be used by #42. Let me change it to another one.