-
-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Fine-Tuned Zim Search #1189
base: main
Are you sure you want to change the base?
Conversation
@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. |
0e7b89c
to
3861988
Compare
@ShaopengLin For me, it does not compile:
|
3861988
to
f0cec36
Compare
@kelson42 Made a quick fix to the compilation warning. Hopefully the libkiwix bug can be resolved. |
f0cec36
to
5887e75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
@kelson42 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.mp4With 4: video2258420655.online-video-cutter.com.1.mp4 |
777c207
to
4a047b7
Compare
Added 2&3 discussed here. Waiting on confirmation for 1. |
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.
I don't understand, seem perfect to me.
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. |
4a047b7
to
30e5520
Compare
@kelson42 The checkbox UI has been added. |
@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? |
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@kelson42 What is the font family used in the mock-up? |
@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...) |
@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. |
@ShaopengLin Any news here so I can make a new review pass? A rebase would be welcome too. |
I was mostly focusing on TOC. Finalizing the UI here is my next step. |
08b9904
to
710f9b4
Compare
Before, view scroll to row based on prefix match
Refactor to reduce getter chaining length
Focus event can happen more than once.
setCompleter call setCompletionPrefix on typing which flicker
Header&Row height as 34px. 5 extra on top&bottom of view.
Aligns with search title and search text
4902dad
to
b19ea81
Compare
@veloman-yunkan I have removed Multizim. |
@ShaopengLin Where it the PR with this part of the code? Please open it immediatly and adapt PR description accordingly. |
Moved Multi-Zim to #1218 |
I have a question about interaction of the following enhancements brought by this PR to the suggestion list:
At this point, there is only one group (Kiwix search) which make dealing with infinite scroll easy. But what's your vision regarding the operation of the infinite scroll when another group of suggestions is added? |
@veloman-yunkan It will largely depend on the scrolling behaviour. If each group gets a scroller, we might need to replace qcompleter with a popup widget that contain two views. The second one is the better option IMO andwhat I think we should be able to do. We seem to be controlling a major portion of the completing behaviour already. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can think of a way to further split this PR, then we can make faster progress on merging it in two steps. Otherwise, I think that the review process will take too long.
But please don't actually split the PR - describe your proposal first and we'll proceed with it if promises to take us to our destination faster.
/* See SearchBar border and margin size in resources/css/style.css */ | ||
int top = searchGeo.height() - 6; /* top&bottom margin + border */ | ||
int width = searchGeo.width() - 6; /* left&right margin + border */ | ||
|
||
#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0) | ||
bool lineEditFlipped = KiwixApp::isRightToLeft(); | ||
#else | ||
bool lineEditFlipped = m_searchbarInput.isRightToLeft(); | ||
#endif | ||
|
||
/* When not flipped, match left of completer to search bar. Popups are | ||
shifted by margin and border so we undo those changes to stay in border. | ||
When flipped, we first match left of completer to right of search bar. | ||
In addition to the popup shift due to border and margin, We move right by | ||
one more border size to match the right border pixel. We then move it | ||
left by width to match left of search bar. | ||
*/ | ||
int left = lineEditFlipped ? -searchLineEditGeo.left() + 4 - width | ||
: -searchLineEditGeo.left() + 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many magic numbers.
@@ -80,6 +80,14 @@ SearchBarLineEdit::SearchBarLineEdit(QWidget *parent) : | |||
m_suggestionView->setRootIsDecorated(false); | |||
m_suggestionView->setStyleSheet(KiwixApp::instance()->parseStyleFromFile(":/css/popup.css")); | |||
|
|||
/* See line-height&padding resources/css/popup.css QHeaderView::section. */ | |||
m_suggestionView->setIconSize(QSize(24, 24)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Qt provide a way of obtaining values from the stylesheet in effect?
Note that this comment applies to other magic number (CSS property values duplicated/hardcoded in C++ code) of this commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think its possible unless we parse the entire CSS to a global object like a JSON object. That will be a whole new project in itself.
/* See resources/css/popup.css QHeaderView::section. | ||
Take line-height + 5 padding top and bottom. | ||
*/ | ||
return QSize(0, 34); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this value cannot be computed from values obtained from CSS, then you can at least minimize the maintenance burden by defining a few constants in a dedicated source file and cross referencing it with the css file so that at most 2 locations have to be updated if/when any of those values changes. For example
css_constants.h
namespace CSS
{
// Need a good explanation of why this file is needed and the logical connection
// of these values to their counterparts in the CSS files must be emphasized.
// A cross reference to each constant from this file must be put beside the respective
// property in the CSS file.
// QHeaderView::section:line-height
const int SUGGESTION_LINE_HEIGHT = 24;
// QHeaderView::section:padding-top
const int SUGGESTION_LINE_PADDING_TOP = 5;
}
and then
return QSize(0, CSS::SUGGESTION_LINE_HEIGHT + CSS::SUGGESTION_LINE_PADDING_TOP + CSS::SUGGESTION_LINE_PADDING_BOTTOM);
Note that the current value of 34 doesn't match the formula from the code comment and the actual CSS values (which rather evaluates to 39).
All other similar magic numbers introduced in C++ code by this PR must be eliminated in a similar way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@veloman-yunkan I furthur propose we can use multi-level namespaces to further structure the CSS. If a file wants to use, for example, CSS::QTabBar::QWidget, they can do an aliasing to get a shorter name.
One question, why does it evaluate it to 39?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question, why does it evaluate it to 39?
Your formula says line-height + 5 padding top and bottom
Relevant CSS is
QHeaderView::section {
...
padding-top: 10px;
...
padding-bottom: 5px;
...
line-height: 24px;
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah.. I forgot to explain the padding top has an extra 5px. Will restructure this in the next PR with better explanations.
src/suggestionlistdelegate.cpp
Outdated
void SuggestionListDelegate::paint(QPainter *painter, | ||
const QStyleOptionViewItem &option, | ||
const QModelIndex &index) const | ||
{ | ||
/* Paint without text and icon */ | ||
QStyleOptionViewItem opt(option); | ||
QStyledItemDelegate::paint(painter, opt, QModelIndex()); | ||
|
||
paintIcon(painter, opt, index); | ||
paintText(painter, opt, index); | ||
} | ||
|
||
void SuggestionListDelegate::paintIcon(QPainter *p, | ||
const QStyleOptionViewItem &opt, | ||
const QModelIndex &index) const | ||
{ | ||
QRect pixmapRect = opt.rect; | ||
|
||
/* See line-height&padding resources/css/popup.css QHeaderView::section. | ||
We need to add 10px of padding to match header. | ||
*/ | ||
QSize mapSize = QSize(24, 24); | ||
auto pixmap = index.data(Qt::DecorationRole).value<QIcon>().pixmap(mapSize); | ||
if (KiwixApp::isRightToLeft()) | ||
{ | ||
int rightEnd = pixmapRect.width() - mapSize.width(); | ||
pixmapRect.setX(pixmapRect.x() + rightEnd - 10); | ||
} | ||
else | ||
pixmapRect.setX(pixmapRect.x() + 10); | ||
|
||
pixmapRect.setY(pixmapRect.y() + (pixmapRect.height() - mapSize.height()) / 2); | ||
pixmapRect.setSize(mapSize); | ||
p->drawPixmap(pixmapRect, pixmap); | ||
} | ||
|
||
void SuggestionListDelegate::paintText(QPainter *p, | ||
const QStyleOptionViewItem &opt, | ||
const QModelIndex &index) const | ||
{ | ||
auto& searchBar = KiwixApp::instance()->getSearchBar(); | ||
auto lineEditGeo = searchBar.getLineEdit().geometry(); | ||
|
||
/* See SearchBar border in resources/css/style.css. Align text with text in | ||
line edit. Remove border from left() as completer is inside border. | ||
*/ | ||
auto left = lineEditGeo.left() - 1; | ||
QRect textRect = opt.rect; | ||
if (KiwixApp::isRightToLeft()) | ||
{ | ||
/* Calculate the distance of right side of search bar to line edit as | ||
text now starts on the right. | ||
*/ | ||
auto searchGeo = searchBar.geometry(); | ||
auto right = searchGeo.width() - left - lineEditGeo.width(); | ||
textRect.setWidth(textRect.width() - right); | ||
} | ||
else | ||
textRect.setX(textRect.x() + left); | ||
|
||
int flag = {Qt::AlignVCenter | Qt::AlignLeading}; | ||
QString text = index.data(Qt::DisplayRole).toString(); | ||
p->drawText(textRect, flag, text); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please elaborate in the commit message why custom painting of suggestions is needed, i.e. why it couldn't be solved with CSS. Hopefully, you'll find a better solution while trying to describe the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I had spent quite a lot of time to figure out how to do it by style and Qt simply does not provide any API to do this. They are drawing the elements manually themselves as well...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please prepare the next version of the PR so that custom drawing is localized in a few commits at the end so that that change can be easily excluded via a single git checkout
. Then I will try to find a better solution.
Another option is to provide a "... load more ... " entry at the bottom of each group (but that's a different type of UI that doesn't - strictly speaking - qualify as infinite scrolling). |
@veloman-yunkan That was my previous approach, which indeed doesn't satisfy kelson's vision of infinite scrolling. |
@veloman-yunkan @ShaopengLin I have created #1194, at this stage I believe we should let it work like it is now. |
@veloman-yunkan A proposal I see now is to have:
This was what I had in mind when structuring this PR, where I do all the functionality changes, and then the UIUX changes. |
@ShaopengLin Go ahead with it! |
What is the status here, now that we have two additional PRs? |
Fixes (pat of) #413
Changes: