Implement Endless Zim Search Suggestions #1224

ShaopengLin · 2024-10-05T05:39:25Z

Fixes (part of) #413

Changes:

Added a custom model to enable endless suggestions
Added basic UI elements such as the search header and icon of the zim for convenience.
Endless can be triggered by up/down/page-down keypresses and mouse scrolls.

Known Issues to be fixed in #1189: The first suggestion is hidden by the header due to QCompleter's incorrect size management.

src/suggestionlistmodel.h

src/searchbar.cpp

veloman-yunkan · 2024-10-07T19:40:42Z

Also there is the following bug:

Open the Ray Charles ZIM file
Start typing beatles in the search box

As soon as you have typed the first three letters (bea) the text in the search box automatically changes to bea (Fulltext search).

ShaopengLin · 2024-10-09T06:51:01Z

@veloman-yunkan The bug is caused by the absence of the commits that prevent suggestion completer flickering. I added those and the bug should be solved.

src/searchbar.h

src/searchbar.cpp

src/searchbar.h

src/searchbar.cpp

src/suggestionlistmodel.cpp

veloman-yunkan

You can guess from the changed nature of most comments that we are now very close to merging this PR. The next iteration can be the final one.

src/suggestionlistworker.h

src/searchbar.cpp

src/searchbar.h

src/searchbar.cpp

src/searchbar.h

src/searchbar.cpp

veloman-yunkan

There are still a couple of bugs (one old and one new) so let's brush up the PR a bit more.

src/searchbar.cpp

veloman-yunkan

An issue was still left because of hasty conflict resolution. Since you have to fix it, please address a few other comments as well.

src/searchbar.cpp

veloman-yunkan · 2024-10-18T08:05:53Z

src/searchbar.cpp

+    if (!m_aboutToScrollPastEnd)
+        return false;
+
+    if (auto e = dynamic_cast<QKeyEvent *>(event))


This time I checked the documentation of QObject::eventFilter() as I was curious about the need for its first parameter). I saw in the example that a dynamic_cast is not used because there is a cheaper way - QEvent::type().

This won't work, as many key press events are not based on exactly keypress, such as QEvent::ShortcutOverride. This also explains some the same issue: https://forum.qt.io/topic/101176/casts-for-events

veloman-yunkan · 2024-10-18T08:09:31Z

src/searchbar.cpp

+
+    if (auto e = dynamic_cast<QKeyEvent *>(event))
+    {
+        if (e->key() == Qt::Key_PageDown && e->modifiers().testFlag(Qt::NoModifier))


Now I wonder if it's better to handle key down here too (instead of your previous fix for it). Won't it eliminate the need to undo the handling of wrapping from bottom to top? If yes then the final code will be simpler, and so let's do it.

So it seems the scrolling event from Key_down either reaches the popup first or isn't blocked from reaching the popup. I simply removed onScrollPastEnd and put the scrolling only inside the onScroll !m_completer.popup()->currentIndex().isValid() case.

src/searchbar.cpp

veloman-yunkan · 2024-10-18T08:31:37Z

src/searchbar.cpp

+void SearchBarLineEdit::fetchMoreSuggestions()
+{
+    fetchSuggestions(&SearchBarLineEdit::onAdditionalSuggestions);
+}


This encapsulates a performance bug. fetchSuggestions() creates a temporary SuggestionListWorker which creates zim::SuggestionSearcher that is discarded after a single use. In a more optimal implementation the zim::SuggestionSearcher should be reused for loading additional suggestions (since those incremental calls to zim::SuggestionSearcher::getResults() should be much cheaper compared to the first call). I am not implying that that optimization be implemented in this PR. I propose to do it myself after this PR is merged, but if you feel like becoming married to this suggestions feature and would like to take care of it to the end I will humbly stay away :)

Let's do this separately.

Definitely. But please add a TODO comment in the body of SearchBarLineEdit::fetchMoreSuggestions() in this PR.

Please don't start working on this until the rest of your kiwix-desktop PRs are merged.

Allow future customizing of suggestions.

More cohesive design to utilize new model's index data.

Data required can be retrieved from model.

Remove unecessary checks and prevent misuse if openCompletion.

Refactor to simplify book icon retrieval

Add offset and fetch size, prepare for endless.

Automatically determine start and allow custom callback. Prepare for endless

Trigger on scroll.

Down/Page_Down Key press correctly fetches.

Fetch offset and model removal requires this.

Before, view scroll to row based on prefix match

Focus event can happen more than once.

setCompleter call setCompletionPrefix on typing which flicker

veloman-yunkan · 2024-10-24T16:52:49Z

This PR broke the handling of pressing ENTER in the search bar - it always opens the first suggestion (even if it doesn't fully match the text in the searchbar).

ShaopengLin · 2024-10-24T17:01:39Z

@veloman-yunkan I believe its mainly from this #1224 (comment) where we discarded the checks for text comparison inside openCompletion.

I will think of a way to avoid the duplication of text check in both openCompletion and getDefaultSuggestionIndex in #1189

veloman-yunkan · 2024-10-24T17:31:14Z

@veloman-yunkan I believe its mainly from this #1224 (comment) where we discarded the checks for text comparison inside openCompletion.

I cannot agree. I don't see any flaw with the design of that approach. getDefaultSuggestionIndex() picks up the suggestion that needs to be opened and openCompletion() just opens it. It looks like there is a bug in getDefaultSuggestionIndex() but it's not immediately clear what's wrong with it.

ShaopengLin · 2024-10-24T17:34:30Z

@veloman-yunkan getDefaultSuggestionIndex is only called during typing. openCompletion is also signaled normalled when user activate(click/enter) on a cell when not typing. openCompletion itself does not use getDefaultSuggestionIndex.

veloman-yunkan · 2024-10-24T18:01:15Z

@veloman-yunkan getDefaultSuggestionIndex is only called during typing. openCompletion is also signaled normalled when user activate(click/enter) on a cell when not typing. openCompletion itself does not use getDefaultSuggestionIndex.

@ShaopengLin Yes. But when an item from the list is activated openCompletion() is called with its exact index. getDefaultSuggestionIndex() is (or, rather, should be) invoked only when the ENTER key is pressed with no item selected.

veloman-yunkan · 2024-10-24T18:18:03Z

The bug is that openCompletion(getDefaulSuggestionIndex()); is called only from onInitialSuggestions(). It works as intended only if ENTER is pressed before suggestions are generated.

kelson42 · 2024-10-24T20:32:27Z

@veloman-yunkan Please open a dedicated issue, not very much cinfirtable with discussion on closed issues/PRa.

veloman-yunkan · 2024-10-25T07:13:42Z

@veloman-yunkan Please open a dedicated issue, not very much cinfirtable with discussion on closed issues/PRa.

Done (#1230)

ShaopengLin requested a review from veloman-yunkan October 5, 2024 05:39

kelson42 added this to the 2.4.0 milestone Oct 5, 2024

ShaopengLin mentioned this pull request Oct 7, 2024

Implement Fine-Tuned Zim Search #1189

Merged

veloman-yunkan requested changes Oct 7, 2024

View reviewed changes

src/suggestionlistmodel.h Outdated Show resolved Hide resolved

src/searchbar.cpp Outdated Show resolved Hide resolved

src/searchbar.cpp Outdated Show resolved Hide resolved

src/searchbar.cpp Outdated Show resolved Hide resolved

src/searchbar.cpp Outdated Show resolved Hide resolved

ShaopengLin force-pushed the Issue#413-endless-search-suggestuon branch 3 times, most recently from cc90069 to 1804f56 Compare October 9, 2024 06:40

ShaopengLin requested a review from veloman-yunkan October 9, 2024 06:42

veloman-yunkan requested changes Oct 9, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#413-endless-search-suggestuon branch 5 times, most recently from 02b297f to b4e5695 Compare October 11, 2024 01:10

ShaopengLin requested a review from veloman-yunkan October 11, 2024 01:11

veloman-yunkan requested changes Oct 11, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#413-endless-search-suggestuon branch 3 times, most recently from aebe4db to 4fa9ab4 Compare October 15, 2024 02:05

ShaopengLin requested a review from veloman-yunkan October 15, 2024 02:06

veloman-yunkan requested changes Oct 15, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#413-endless-search-suggestuon branch from 4fa9ab4 to b6a6c06 Compare October 15, 2024 22:13

ShaopengLin requested a review from veloman-yunkan October 15, 2024 22:14

veloman-yunkan requested changes Oct 16, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#413-endless-search-suggestuon branch from b6a6c06 to 1bc6c82 Compare October 18, 2024 06:19

ShaopengLin requested a review from veloman-yunkan October 18, 2024 06:27

veloman-yunkan requested changes Oct 18, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#413-endless-search-suggestuon branch from 1bc6c82 to 07aa791 Compare October 18, 2024 19:14

ShaopengLin added 17 commits October 21, 2024 14:16

Introduce suggestionlistmodel.{h, cpp}

6e1e0b8

Allow future customizing of suggestions.

Move SearchBarLineEdit urlList to SuggestionListModel

8f3d97f

More cohesive design to utilize new model's index data.

Remove redundant openCompletion(str, int)

8d3d5ec

Data required can be retrieved from model.

Introduce SearchBarLineEdit::getDefaultSuggestionIndex

14bfad6

Remove unecessary checks and prevent misuse if openCompletion.

Enter Library::getBookIcon()

6130dd6

Refactor to simplify book icon retrieval

Add Suggestion Icon Decoration Role

19bc496

Refactor SuggestionListWorker

1ef77f8

Add offset and fetch size, prepare for endless.

Refactor Launching SuggestionListWorker

f1bcd41

Automatically determine start and allow custom callback. Prepare for endless

Introduce Endless Suggestions

d22a922

Trigger on scroll.

Endless Suggestion Key Scrolling compatible

f44ab97

Down/Page_Down Key press correctly fetches.

Correctly handle Endless w/Fulltext

ca2021e

Fetch offset and model removal requires this.

Fetch Only When More Suggesion Exist

a59aacb

Make First Row Visible on Suggestion Popup

951917f

Before, view scroll to row based on prefix match

Introduce Kiwix Search Header

ff50e9d

Replace m_completer.popup() with m_suggestionView

2fabf3b

BugFix openCompletion Connected Multiple Times

ec39c09

Focus event can happen more than once.

Fix Suggestion Flickering when Typing

9982a22

setCompleter call setCompletionPrefix on typing which flicker

ShaopengLin force-pushed the Issue#413-endless-search-suggestuon branch from f62d9ac to 9982a22 Compare October 21, 2024 18:30

kelson42 merged commit c6ea91f into main Oct 21, 2024
6 checks passed

kelson42 deleted the Issue#413-endless-search-suggestuon branch October 21, 2024 18:41

veloman-yunkan mentioned this pull request Oct 25, 2024

Pressing ENTER in the search bar opens the first suggestion #1230

Closed

ShaopengLin mentioned this pull request Oct 30, 2024

BugFix: Should Open Default Suggestion on Search Bar when Nothing is Selected #1232

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Endless Zim Search Suggestions #1224

Implement Endless Zim Search Suggestions #1224

ShaopengLin commented Oct 5, 2024 •

edited

Loading

veloman-yunkan commented Oct 7, 2024

ShaopengLin commented Oct 9, 2024

veloman-yunkan left a comment

veloman-yunkan left a comment

veloman-yunkan left a comment

veloman-yunkan Oct 18, 2024

ShaopengLin Oct 18, 2024

veloman-yunkan Oct 18, 2024

ShaopengLin Oct 18, 2024

veloman-yunkan Oct 18, 2024

ShaopengLin Oct 18, 2024

veloman-yunkan Oct 19, 2024

veloman-yunkan Oct 21, 2024

veloman-yunkan commented Oct 24, 2024

ShaopengLin commented Oct 24, 2024 •

edited

Loading

veloman-yunkan commented Oct 24, 2024

ShaopengLin commented Oct 24, 2024

veloman-yunkan commented Oct 24, 2024

veloman-yunkan commented Oct 24, 2024

kelson42 commented Oct 24, 2024

veloman-yunkan commented Oct 25, 2024

Implement Endless Zim Search Suggestions #1224

Implement Endless Zim Search Suggestions #1224

Conversation

ShaopengLin commented Oct 5, 2024 • edited Loading

veloman-yunkan commented Oct 7, 2024

ShaopengLin commented Oct 9, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

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

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

veloman-yunkan commented Oct 24, 2024

ShaopengLin commented Oct 24, 2024 • edited Loading

veloman-yunkan commented Oct 24, 2024

ShaopengLin commented Oct 24, 2024

veloman-yunkan commented Oct 24, 2024

veloman-yunkan commented Oct 24, 2024

kelson42 commented Oct 24, 2024

veloman-yunkan commented Oct 25, 2024

ShaopengLin commented Oct 5, 2024 •

edited

Loading

ShaopengLin commented Oct 24, 2024 •

edited

Loading