Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement Fine-Tuned Zim Search #1189

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Implement Fine-Tuned Zim Search #1189

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7b7c949
Introduce suggestiolistmodel.{h, cpp}
ShaopengLin Aug 19, 2024
1f1743b
Move SearchBarLineEdit urlList to SuggestionListModel
ShaopengLin Sep 1, 2024
8611a1c
Enter Library::getZimIcon()
ShaopengLin Sep 1, 2024
6cbc9e8
Add Suggestion Icon Decoration Role
ShaopengLin Sep 1, 2024
c029da2
Refactor SuggestionListWorker
ShaopengLin Sep 1, 2024
d5de7c6
Introduce SuggestionListModel::lastIndex
ShaopengLin Sep 17, 2024
0aac03f
Introduce Endless Suggestions
ShaopengLin Sep 17, 2024
84e29ec
Endless Suggestion Key press compatible
ShaopengLin Sep 17, 2024
2aca115
Endless Suggestion Page_Down Compatible
ShaopengLin Sep 17, 2024
1894d41
Correctly handle Endless w/Fulltext
ShaopengLin Sep 17, 2024
d0a1680
Fetch Only When More Suggesion Exist
ShaopengLin Sep 18, 2024
76bae0d
Make First Row Visible on Suggestion Popup
ShaopengLin Sep 17, 2024
a7d7304
Introduce Kiwix Search Header
ShaopengLin Sep 17, 2024
d713173
Replace m_completer.popup() with m_suggestionView
ShaopengLin Sep 17, 2024
b3d9424
Enter KiwixApp::getSearchBar()
ShaopengLin Sep 17, 2024
ff7117b
BugFix openCompletion Connected Multiple Times
ShaopengLin Sep 28, 2024
ce36366
Make SearchBar Margin Consistent
ShaopengLin Sep 28, 2024
b9f4f94
Align Search Suggestion with Line Edit
ShaopengLin Sep 28, 2024
02e042a
Fix Suggestion Flickering when Typing
ShaopengLin Sep 28, 2024
dac8d04
Proper Size for Suggestion View
ShaopengLin Sep 28, 2024
fcebdd0
Proper Suggestion Icon&Text Spacing
ShaopengLin Sep 28, 2024
64b30c3
Proper Hover Style to Suggestion Items
ShaopengLin Sep 28, 2024
b19ea81
Proper Elide for Suggestion List Text
ShaopengLin Sep 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions resources/css/popup.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,22 @@ QWidget {
border: none;
}

/* data of src/suggestionlistModel.cpp and SearchBarLineEdit of src/searchbar.cpp
references the line-height and padding.
*/
QHeaderView::section {
border: none;
color: #3b3b3b;
padding-top: 10px;
padding-left: 10px;
padding-right: 10px;
padding-bottom: 5px;
background-color: white;
font-size: 16px;
line-height: 24px;
font-weight: 400;
}

QScrollBar {
width: 5px;
border: none;
Expand Down
8 changes: 8 additions & 0 deletions src/searchbar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

connect(&m_suggestionModel, &QAbstractListModel::modelReset, [=](){
/* +1 for header. +10px for 5px top&bottom extra space */
int count = std::min(m_suggestionModel.rowCount(), m_completer.maxVisibleItems());
m_suggestionView->setMinimumHeight(m_suggestionView->sizeHintForRow(0) * (count + 1) + 10);
});

connect(m_suggestionView->verticalScrollBar(), &QScrollBar::valueChanged,
this, &SearchBarLineEdit::onScrollToEnd);

Expand Down
5 changes: 5 additions & 0 deletions src/suggestionlistmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ QVariant SuggestionListModel::data(const QModelIndex &index, int role) const
return library->getZimIcon(zimId, defaultIcon);
}
break;
case Qt::SizeHintRole:
/* See resources/css/popup.css QHeaderView::section.
Take line-height + 5 padding top and bottom.
*/
return QSize(0, 34);
Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

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;
    ...
}

Copy link
Collaborator Author

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.

}
return QVariant();
}
Expand Down