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

ShaopengLin · 2024-09-28T21:20:00Z

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 · 2024-10-28T18:50:10Z

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

ShaopengLin · 2024-10-28T18:51:03Z

@kelson42 This is the next change.

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

veloman-yunkan · 2024-10-29T07:16:23Z

@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 · 2024-10-30T07:32:02Z

@veloman-yunkan The code ios ready for review.

veloman-yunkan · 2024-10-31T10:08:32Z

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 · 2024-10-31T10:12:30Z

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 · 2024-10-31T16:32:29Z

@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.

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 · 2024-10-31T17:00:55Z

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 · 2024-10-31T17:04:16Z

@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 · 2024-10-31T17:06:07Z

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

kelson42 · 2024-10-31T18:06:54Z

@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 · 2024-11-01T03:40:02Z

@veloman-yunkan

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

resources/i18n/qqq.json

src/multizimbutton.h

src/searchbar.h

veloman-yunkan · 2024-11-02T13:48:35Z

src/multizimbutton.cpp

+    const auto library = KiwixApp::instance()->getLibrary();
+    for (const auto& bookId : library->getBookIds())
+    {
+        std::shared_ptr<zim::Archive> archive;


Unused variable

Why did you remove the entire try{}catch(){} statement? My understanding was that you wanted to exclude those books that didn't have a valid ZIM archive associated with them (e.g. books that are still being downloaded, or books with ZIM archives on absent removable media).

Done, my bad forgot about the check.

src/multizimbutton.cpp

resources/i18n/qqq.json

src/multizimbutton.cpp

veloman-yunkan · 2024-11-02T14:36:55Z

src/multizimbutton.cpp

+    const int labelWidth = menu->width() - contentWidthExcludeText;
+    textLabel->setFixedWidth(labelWidth);
+
+    QFontMetrics metrics(textLabel->font());
+    const int elideMarkerLength = metrics.tightBoundingRect("(...)").width();
+    const int textLength = labelWidth - elideMarkerLength;
+    QString elidedText = metrics.elidedText(text, Qt::ElideRight, textLength);
+    if (elidedText != text)
+    {
+        /* Remove built-in elide marker */
+        elidedText.chop(1);
+        textLabel->setText(elidedText.trimmed() + "(...)");
+    }
+    else
+        textLabel->setText(text);


Is there code duplication with custom elision of suggestions?

Before the if statement yes. The handling of elide is different since Qt handles LineEdit differently among other text displaying widget (quite a pain)

veloman-yunkan · 2024-11-03T09:02:14Z

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

ShaopengLin · 2024-11-03T17:03:12Z

@veloman-yunkan

Renamed button
Resolved UI for selecting the book to be used for title search #1218 (comment) and UI for selecting the book to be used for title search #1218 (comment)

ShaopengLin · 2024-11-03T17:12:53Z

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

veloman-yunkan

LGTM. Probably it would be better to get rid of the MultiZim (in source code) vs "Search options" (as presented to the user) inconsistency but that's not a sound enough reason to delay the merging of this PR.

veloman-yunkan · 2024-11-03T17:14:16Z

src/kiwixapp.cpp

@@ -438,6 +438,8 @@ void KiwixApp::createActions()
    CREATE_ACTION_SHORTCUT(ToggleTOCAction, gt("table-of-content"), QKeySequence(Qt::CTRL | Qt::SHIFT | Qt::Key_1));
    HIDE_ACTION(ToggleTOCAction);

+    CREATE_ACTION_ICON_SHORTCUT(OpenMultiZimAction, "filter", gt("search-options"), QKeySequence(Qt::CTRL | Qt::Key_M));


@kelson42 Is the CTRL+M shortcut OK for a button named "Search options"?

I was aware of this as well. I will look around to find an appropriate one or what is the usual shortcut for this.

Done, switched to Ctrl + Shift + L which is a common shortcut for opening and closing search options.

User can search zim other than the current tab.

Shortcut and Icon for MultiZim. Since window is popup, toggle is not possible.

Decouple complex item creation from button. Used later for styling.

Missing this reason cause completer to do nothing

Previously only checks when clicking Radiobutton

Set height&padding for list&items and stretch to menu width.

Helper function to retrieve text content that can fit inside a length given a custom "(...)" elide.

Add Custom elide marker

With MultiZim, searching on library, settings, and blank tabs are reasonable.

ShaopengLin requested a review from veloman-yunkan September 28, 2024 21:20

ShaopengLin changed the title ~~Issue#413 multizim~~ Introduce Multi-Zim Search Sep 28, 2024

ShaopengLin mentioned this pull request Sep 28, 2024

Implement Fine-Tuned Zim Search #1189

Merged

kelson42 added this to the 2.4.0 milestone Oct 3, 2024

ShaopengLin force-pushed the Issue#413-search-UI-improvement branch 4 times, most recently from 8246c5c to c3f4a1e Compare October 28, 2024 17:32

Base automatically changed from Issue#413-search-UI-improvement to main October 28, 2024 18:46

ShaopengLin force-pushed the Issue#413-multizim branch 3 times, most recently from cc6d952 to 3f282b4 Compare October 30, 2024 07:31

kelson42 force-pushed the Issue#413-multizim branch from 3f282b4 to c26fd20 Compare October 30, 2024 19:52

ShaopengLin force-pushed the Issue#413-multizim branch from c26fd20 to 9b2c81a Compare October 31, 2024 06:10

ShaopengLin force-pushed the Issue#413-multizim branch 2 times, most recently from 1400fff to 0c08a9e Compare November 1, 2024 03:38

veloman-yunkan requested changes Nov 2, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#413-multizim branch from 0c08a9e to 9e0b37a Compare November 3, 2024 06:09

ShaopengLin requested a review from veloman-yunkan November 3, 2024 06:11

veloman-yunkan changed the title ~~Introduce Multi-Zim Search~~ UI for selecting the book to be used for title search Nov 3, 2024

ShaopengLin force-pushed the Issue#413-multizim branch from 9e0b37a to 7fe06f5 Compare November 3, 2024 16:48

veloman-yunkan approved these changes Nov 3, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#413-multizim branch from 7fe06f5 to 1d533a3 Compare November 3, 2024 17:31

ShaopengLin added 11 commits November 4, 2024 10:16

Introduce MultiZim Search Button

95ccd76

User can search zim other than the current tab.

Introduce KiwixApp::OpenMultiZimAction

e9c7421

Shortcut and Icon for MultiZim. Since window is popup, toggle is not possible.

Introduce ZimItemWidget to Delegate Item Display

e489712

Decouple complex item creation from button. Used later for styling.

LineEdit Should PopupFocus

19c7acc

Missing this reason cause completer to do nothing

MultiZim Auto-Select Current Zim

f5bef3b

Check MultiZim Item When Clicking Anywhere

85d2790

Previously only checks when clicking Radiobutton

Proper MultiZim List Sizing

c5f2f76

Set height&padding for list&items and stretch to menu width.

Proper Styling for MultiZim List

37c1c38

Introduce getElidedText() in suggestionlistdelegate.cpp

4beac33

Helper function to retrieve text content that can fit inside a length given a custom "(...)" elide.

Proper Elide for MultiZim List

14d7cff

Add Custom elide marker

Allow Search On Any Tab.

5922f38

With MultiZim, searching on library, settings, and blank tabs are reasonable.

kelson42 force-pushed the Issue#413-multizim branch from 1d533a3 to 5922f38 Compare November 4, 2024 09:16

kelson42 merged commit 2365249 into main Nov 4, 2024
6 checks passed

kelson42 deleted the Issue#413-multizim branch November 4, 2024 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ShaopengLin commented Sep 28, 2024 •

edited

Loading

kelson42 commented Oct 28, 2024

ShaopengLin commented Oct 28, 2024

veloman-yunkan commented Oct 29, 2024

ShaopengLin commented Oct 30, 2024

veloman-yunkan commented Oct 31, 2024 •

edited

Loading

veloman-yunkan commented Oct 31, 2024

ShaopengLin commented Oct 31, 2024

veloman-yunkan commented Oct 31, 2024

veloman-yunkan commented Oct 31, 2024

ShaopengLin commented Oct 31, 2024 •

edited

Loading

kelson42 commented Oct 31, 2024

ShaopengLin commented Nov 1, 2024

veloman-yunkan Nov 2, 2024

ShaopengLin Nov 3, 2024

veloman-yunkan Nov 3, 2024

ShaopengLin Nov 3, 2024

veloman-yunkan Nov 2, 2024

ShaopengLin Nov 3, 2024 •

edited

Loading

ShaopengLin Nov 3, 2024

veloman-yunkan commented Nov 3, 2024

ShaopengLin commented Nov 3, 2024 •

edited

Loading

ShaopengLin commented Nov 3, 2024

veloman-yunkan left a comment

veloman-yunkan Nov 3, 2024

ShaopengLin Nov 3, 2024

ShaopengLin Nov 3, 2024

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

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

Conversation

ShaopengLin commented Sep 28, 2024 • edited Loading

kelson42 commented Oct 28, 2024

ShaopengLin commented Oct 28, 2024

veloman-yunkan commented Oct 29, 2024

ShaopengLin commented Oct 30, 2024

veloman-yunkan commented Oct 31, 2024 • edited Loading

veloman-yunkan commented Oct 31, 2024

ShaopengLin commented Oct 31, 2024

veloman-yunkan commented Oct 31, 2024

veloman-yunkan commented Oct 31, 2024

ShaopengLin commented Oct 31, 2024 • edited Loading

kelson42 commented Oct 31, 2024

ShaopengLin commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaopengLin Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veloman-yunkan commented Nov 3, 2024

ShaopengLin commented Nov 3, 2024 • edited Loading

ShaopengLin commented Nov 3, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaopengLin commented Sep 28, 2024 •

edited

Loading

veloman-yunkan commented Oct 31, 2024 •

edited

Loading

ShaopengLin commented Oct 31, 2024 •

edited

Loading

ShaopengLin Nov 3, 2024 •

edited

Loading

ShaopengLin commented Nov 3, 2024 •

edited

Loading