Implement Fine-Tuned Zim Search

[ShaopengLin]

Fixes (part of) #413

Changes:

• Proper font, spacing, and sizing for suggestion items.
• Align suggestion list to searchBar.
• Text Elide.

[ShaopengLin]

@kelson42 Could you do a pass on the UI first? I will be re-factoring the code and commits in the mean time but would want some feed-back during this time so I can incorporate those.

[kelson42]

@ShaopengLin For me, it does not compile:

r/include/x86_64-linux-gnu/qt5/QtCore -I. -I. -I/usr/lib/x86_64-linux-gnu/qt5/mkspecs/linux-g++ -o suggestionlistmodel.o src/suggestionlistmodel.cpp
src/suggestionlistmodel.cpp: In member function ‘void SuggestionListModel::resetSuggestions(const QStringList&)’:
src/suggestionlistmodel.cpp:110:21: error: loop variable ‘suggestion’ creates a copy from type ‘const QString’ [-Werror=range-loop-construct]
  110 |     for (const auto suggestion : suggestions)
      |                     ^~~~~~~~~~
src/suggestionlistmodel.cpp:110:21: note: use reference type to prevent copying
  110 |     for (const auto suggestion : suggestions)
      |                     ^~~~~~~~~~
      |                     &
cc1plus: all warnings being treated as errors

[ShaopengLin]

@kelson42 Made a quick fix to the compilation warning. Hopefully the libkiwix bug can be resolved.

[kelson42]

@ShaopengLin To me this is already almost perfect. My request would be:

• Implement the checkbox "Select all" and grey everything
• The suggestion overlay should be as broad as the search box
• The search overlay is one or two pixel two high, it slightly covers the searchbox and it should not (see mockup)
• If suggestions are displayed, we need to click twise on the suggestions settings to display the overlay, only one click should be necessary

At this stage, this is basically the only problems I have seen!

[ShaopengLin]

@kelson42
For 1: To clarify, do you mean grey the 'Select All' checkbox? I am assuming this as we don't have 'true' multi-zim search yet. So having this feature enabled wouldn't make any sense.

For 2 & 3. After some digging, I found a way to circumvent several Qt Bugs. Aligning with Line Edit is now achievable. Link for future technical reference.

For 4: I have tried my best but cannot find a good solution. Qt doesn't naturally provide this and the best attempt at a hack causes the search to flicker. I would recommend have this for a different issue.

Without 4 implemented:

video1258420655.online-video-cutter.com.mp4

With 4:

video2258420655.online-video-cutter.com.1.mp4

[ShaopengLin]

Added 2&3 discussed here. Waiting on confirmation for 1.

[kelson42]

For 1: To clarify, do you mean grey the 'Select All' checkbox? I am assuming this as we don't have 'true' multi-zim search yet. So having this feature enabled wouldn't make any sense.

Yes, please make the whole "All select" working fine (and then grey or hide it). Reason is that we will ASAP implement the multizim selection (in the libzim) and I want to have the UI ready.

For 2 & 3. After some digging, I found a way to circumvent several Qt Bugs. Aligning with Line Edit is now achievable. Link for future technical reference.

I don't understand, seem perfect to me.

For 4: I have tried my best but cannot find a good solution. Qt doesn't naturally provide this and the best attempt at a hack causes the search to flicker. I would recommend have this for a different issue.

Open issues then please and give all the references. That sounds really weird to me that you face difficulty on this. From my perspective (very far away) you should display the overlay when the settings button get the focus, not when you click on it. Maybe unfocusing the searchbar helps as well.

[ShaopengLin]

@kelson42 The checkbox UI has been added.

[kelson42]

@veloman-yunkan I will check a last time rhat everything works fine but so far I'm concerned LGTM. Can you pleaee start with the code review?

[kelson42]

@ShaopengLin Please respect the mockup:

• Background should be white
• To make a big box if there is only two items, the "Select All" should be directly below the list of items
• For each item the margins around the checkboxes are not regular, the space between favicon and test if far too big.. and the ZIM title sticks too much to the favicon.
• Ensure that the text "Select all" is properly aligned with the favicons
• "Kiwix Search" label should not be bold, try to stick to the mockup AFAP
• Actually the whole suggestions item are in the wrong size/fonting, see
  

Please make an overall check that you check AFAP to the mockup, at the pixel level.

[ShaopengLin]

@kelson42 What is the font family used in the mock-up?

[kelson42]

@kelson42 What is the font family used in the mock-up?

All the details are here... but please don't take measure to change the default font. If you see something to change there, please create an issue.

[ShaopengLin]

@veloman-yunkan @kelson42 Code review should hold off. To have the exact spacings like the mock up, will need a somewhat large architectural change to this PR (due to Qt not offering those stylistic changes and I will have to implement/style the icon and text painting myself.)

[kelson42]

@veloman-yunkan @kelson42 Code review should hold off. To have the exact spacings like the mock up, will need a somewhat large architectural change to this PR (due to Qt not offering those stylistic changes and I will have to implement/style the icon and text painting myself.)

I disagree, a lot of things can be fixed without changing the font (like font size, font boldness, etc...)

[ShaopengLin]

@kelson42 I am mostly talking about the spacing between icon and text. I agree with you the fonts and boldness are easy to change.

[kelson42]

@kelson42 I am mostly talking about the spacing between icon and text. I agree with you the fonts and boldness are easy to change.

OK, do your best to fix the most and we will tackle what is left at the end.

[kelson42]

@ShaopengLin Any news here so I can make a new review pass? A rebase would be welcome too.

[ShaopengLin]

I was mostly focusing on TOC. Finalizing the UI here is my next step.

[veloman-yunkan]

Note also that without custom painting there is proper (and nicer) elision of long suggestions that don't fully fit in the popup.

[veloman-yunkan]

@kelson42 What's your opinion about #1189 (comment)?

[kelson42]

@kelson42 What's your opinion about #1189 (comment)?

@veloman-yunkan I see the differences because most of the fixes seem to be directly the consequences of my requets (so tracked on GitHub and visible to you). The second version is correct whereas the first is not.

[veloman-yunkan]

@kelson42 What's your opinion about #1189 (comment)?

@veloman-yunkan I see the differences because most of the fixes seem to be directly the consequences of my requets (so tracked on GitHub and visible to you). The second version is correct whereas the first is not.

Ok, then I will look for a nicer fix for those styling adjustments.

[ShaopengLin]

@veloman-yunkan The alt-tab comment should be focus related as the shortcut tries to switch windows. I will look into that.

The printscreen seems like a necssary consequence of a modal window. I only captured the screen shots using delayed print screen.

[ShaopengLin]

@veloman-yunkan I investigated and found that ALT+TAB is looking more like an auto-complete shortcut for QCompleter. The QCompleter is activated by the key combination which triggers openCompletion. It also is not overridden to Key_Enter when I verified it in the event handler.

I don't see this as a problem as a modal window is supposed to be the top level and dealt with first. Any modal window would not let ALT+TAB (switching between applications/windows) to go through anyways.

[veloman-yunkan]

My solution to controlling the spacing between the suggestion icon and text is in the branch Issue#413-search-UI-improvement-patch. The main problem with this approach is the display of the suggestion group header. Note however, if additional types of suggestions are introduced (history, open tabs, etc) the header of the group will have to be implemented in a different way (e.g. as a special entry in the suggestion list) in which case this problem will disappear (or rather be replaced with a different one, since then the header of the group won't be fixed when scrolling).

[ShaopengLin]

@veloman-yunkan Moving the discussion from #1224 to here:

If we would like to open getDefaultSuggestionIndex() if no item is selected, then we can do something like this:

void openCompletion(const QModelIndex &index)
{
...
QModelIndex indexToOpen = index;
if (!indexToOpen.isValid())
    indexToOpen = getDefaulSuggestionIndex();
if (!indexToOpen.isValid())
{
...
}
}

void onInitialSuggestions()
{
    ...
    if(m_returnPressed)
        /* openCompletion(getDefaulSuggestionIndex()); */ -> openCompletion(QModelIndex());
    ...
    
    /* Default selection is nothing. */
    const auto completerSelModel = m_suggestionView->selectionModel();
    completerSelModel->setCurrentIndex(QModelIndex(), QItemSelectionModel::Current);
}

This makes would sure we open the default suggestion on both activation and onInitialSuggestions.

[ShaopengLin]

@veloman-yunkan I don't believe the approach from Issue#413-search-UI-improvement...Issue#413-search-UI-improvement-patch have the fine-grained controls to achieve the precision Kelson wants. In addition, custom elide '(...)' is also what is requested by the mock-up, so delegating actually makes achieving that less painful.

[veloman-yunkan]

@veloman-yunkan I don't believe the approach from Issue#413-search-UI-improvement...Issue#413-search-UI-improvement-patch have the fine-grained controls to achieve the precision Kelson wants. In addition, custom elide '(...)' is also what is requested by the mock-up, so delegating actually makes achieving that less painful.

Of course that is a matter of taste but I find Qt's elision nicer. Overall I frankly don't think that those subtle adjustments in the look of the suggestion list justify the extra hacks (neither custom painting nor the double-column approach) needed to achieve them. I really frown upon the custom painting solution because it is likely to incur additional maintenance costs. For example,

• Does TTS automatically work with custom painted text?
• Is the spacing calculation correct for high resolution screens?
• Does custom painting take into account system wide accessibility settings (font size, color scheme)?

@kelson42 Do you still insist that those spacing values are important enough that we should pursue them at the cost of increased complexity of implementation?

[veloman-yunkan]

@veloman-yunkan Moving the discussion from #1224 to here:

If we would like to open getDefaultSuggestionIndex() if no item is selected, then we can do something like this:

@ShaopengLin Let's try it

[kelson42]

@kelson42 Do you still insist that those spacing values are important enough that we should pursue them at the cost of increased complexity of implementation?

Yes, I insist.. not really my fault or the fault of the designer is Qt can not cleanly allow proper positioning of the elements. UI has to be clean at the pixel level. We have to deal with the framework we have with all the "pros¨ and the "cons".

[veloman-yunkan]

@kelson42 Do you still insist that those spacing values are important enough that we should pursue them at the cost of increased complexity of implementation?

Yes, I insist.. not really my fault or the fault of the designer is Qt can not cleanly allow proper positioning of the elements. UI has to be clean at the pixel level. We have to deal with the framework we have with all the "pros¨ and the "cons".

I cannot agree. I think that you give too much importance to a minor issue (at least compared to the potential ramifications that may ensue as we try to address it). It reminds me about Gemini's infamous ideologically-fueled stance.

Whatever. You are the boss. I humbly submit. Time will tell.

[ShaopengLin]

@veloman-yunkan No need for an extra review.

this push was for #1189 (comment) where I changed to just use SearchBarLineEdit::isRightToLeft(). I was kinda waiting for #1230 discussion as well as the UI discussions, but it seems this PR is ready at the current stage so I could do this in separate PRs.