-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
add new searchbox styles #4505
add new searchbox styles #4505
Conversation
src/widget/wsearchlineedit.cpp
Outdated
// Show available auto-complete suggestions in a popup. | ||
// Without this (default: QCompleter::InlineCompletion) a suggestion is picked | ||
// automatically, and needs to be removed manually if it doesn't fit | ||
lineEdit()->completer()->setCompletionMode(QCompleter::PopupCompletion); |
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.
This is a valuable change we should implement.
Though I'll revert this for now because currently it can't be styled wit qss.
As soon as I find a way that allows to adequately style the additional drop-down I'll open a follow-up.
aed8ace
to
76cacca
Compare
1344eb5
to
4748a70
Compare
This is now ready for review. |
4748a70
to
8a32cde
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.
can't comment on the qss or any removed code since I don't know how it worked in the first place.
src/widget/wsearchlineedit.cpp
Outdated
<< text; | ||
#endif // ENABLE_TRACE_LOG | ||
|
||
int paddingPx; |
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 variable does not get initialized in both of the if branches below, its uninitialized memory and using it then would result in UB. IMO it would be better to initialize on declaration using this tertiary statement. The resulting double call to isEmpty
can be ignored imo because its cheap.
int paddingPx; | |
const int paddingPx = text.isEmpty() ? 0 : m_innerHeight; |
@Swiftb0y Thanks for your review, I realized some chances for improvement after not having looked at it for some weeks. If the fixup looks okay to you I'll squash it. Re skin testing:
|
Hi, this fixes problem I have on master - auto filing searchbar and jumping to the end of the line. But I think there should be option to disable autocompletion at all. Even if it works better now, I still feel it is slowing me down. |
ece2f6d
to
3f43ed7
Compare
@vlada-dudr I think you tested the intermdeiate version with the auto-complete drop-down. Though I reverted that because I couldn't style the list view, see my comment. |
@Swiftb0y Ready for merge. |
I tested and commented on current version, with inline autocomplete. I checked it out just before commenting now. This branch is bug-free, which is much better, but my opinion holds: autocomplete is annoying feature (for me). I am sorry if I am too moronic. |
Ah okay, I assumed you referred to the auto-complete drop-down. Thanks for testing, appreciated! |
Thanks for testing all skins!
Actually the button sizes are equal in all skins, only the icons are a bit thinner in LateNight (PaleMoon).
Good point. I'll adjust that.
Yes, out of scope. Tab is |
Yeah I figured that either some of the assets are suboptimal or my brain is just straight up tricking me. Still, it just looked "off" in latenight compared to the rest so I figured I'd mention that.
yes, thats not as intuitive as Tab though since going to the arrow key essentially forces me to take my right hand of the typing position. Maybe I'm just biased and prefer tab because I'm used to it though. Still, it would be nice if both ➔ and tab could advance the autocompletion, though I guess that would involve some hacks. |
3f43ed7
to
ba1e124
Compare
@Swiftb0y If you find some time please take a final look. In LateNight the max button is a bit wider now and has some more space, hope that is okay now: |
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.
Works very well and looks good. Thanks.
Skin styles for the new searchbox drop-down menu.