Modify search bar in the contextDrawer
Needs ReviewPublic

Authored by ognarb on Feb 2 2019, 12:03 AM.

Details

Reviewers
ngraham
aacid
Group Reviewers
Okular
Summary

Use a copy of Discover searchBar component, modified. The biggest change between discover version and this version is that this version doesn't support a ToolTip, because I didn't find a way to make the tooltip displayed under the search bar.

Screenshots:

New:



Old:


Test Plan

Search still works
Icon is only displayed when text is present.

Diff Detail

Repository
R223 Okular
Branch
arcpatch-D18658
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9503
Build 9521: arc lint + arc unit
ognarb created this revision.Feb 2 2019, 12:03 AM
Restricted Application added a project: Okular. · View Herald TranscriptFeb 2 2019, 12:03 AM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
ognarb requested review of this revision.Feb 2 2019, 12:03 AM
ognarb edited the summary of this revision. (Show Details)Feb 2 2019, 12:08 AM
ognarb added a reviewer: Okular.
ognarb edited the summary of this revision. (Show Details)Feb 2 2019, 12:14 AM
ngraham requested changes to this revision.Feb 2 2019, 3:22 AM
ngraham added a subscriber: ngraham.

Thanks for all this great work on Okular's mobile interface!

The search field shouldn't be touching the line below it; it should have as much padding on top as on bottom. Just needs to be vertically centered in its parent, probably.

Also, instead of manufacturing your own search field, how about contributing it upstream to Kirigami? There are a number of other places where a QML-yet-non-PlasmaComponents-using desktop app (e.g. System Settings) needs a search field with a clear button.

This revision now requires changes to proceed.Feb 2 2019, 3:22 AM

The search field shouldn't be touching the line below it; it should have as much padding on top as on bottom. Just needs to be vertically centered in its parent, probably.

Ok I will change that :)

Also, instead of manufacturing your own search field, how about contributing it upstream to Kirigami? There are a number of other places where a QML-yet-non-PlasmaComponents-using desktop app (e.g. System Settings) needs a search field with a clear button.

Just submitted a diff to review: D18716. Thanks for the suggestion.

ognarb updated this revision to Diff 52407.Feb 23 2019, 9:18 PM
ognarb edited the summary of this revision. (Show Details)

Rebase this diff on latest state of D18716

ognarb updated this revision to Diff 52415.Feb 23 2019, 10:46 PM
  • Add bottomMargin to search field
aacid added a subscriber: aacid.Feb 26 2019, 11:25 PM
aacid added inline comments.
CMakeLists.txt
12

I don't like this.

If you need a new kirigami or something make a vesion check and then enable kirigami only if X verison is available, but i don't like the general requirement for okular to be increased at this point

ognarb added inline comments.Mar 1 2019, 1:32 PM
CMakeLists.txt
12

I'm not very familiar with cmake. That about creating a separate patch with the placeholder text and the bottom margin improvement and wait until Okular depends on 5.56 to land the rest (the clear button)?

ngraham accepted this revision.Mar 3 2019, 9:47 PM

Looks great to me. Please wait to land this until @aacid or another Okular developer is okay with bumping the Frameworks dependency to 5.56.

BTW @aacid, what's your objection about doing this, out of curiosity? People on rolling release distros will surely have 5.56 by the time Applications 19.04.0 is released, and once time discrete release distros get around to shipping 19.04.0, I expect that they'll have already shipped a new enough version of Frameworks, if not even newer.

Also @ognarb, now that I see all these patches, I'm wondering if it might make sense to create two new Kirigami components, each based on your very nice ActionTextField:

  • SearchField, which includes all the search field boilerplate that you're adding here (keyboard shortcut, clear button, placeholder text, etc)
  • PasswordField, which has a clear button as well as a "show password" button.

This would be very Kirigami-ish, in the spirit of providing high-level controls so that apps don't need to reinvent the wheel all the time. And then we would have total UI consistency for all instances of these fields.

This revision is now accepted and ready to land.Mar 3 2019, 9:47 PM
aacid added a comment.Mar 4 2019, 7:06 PM

BTW @aacid, what's your objection about doing this, out of curiosity? People on rolling release distros will surely have 5.56 by the time Applications 19.04.0 is released, and once time discrete release distros get around to shipping 19.04.0, I expect that they'll have already shipped a new enough version of Frameworks, if not even newer.

Casual developers like myself that don't want to compile KF5 for no real reason.

aacid requested changes to this revision.Mar 4 2019, 7:06 PM
This revision now requires changes to proceed.Mar 4 2019, 7:06 PM

But there is a real reason: to adopt a new API. And compiling KF5 is trivial with kdesrc-build these days. Not to put too fine a point on it, but why should your personal convenience be more important than adopting an API that was specifically made for this use case?

But there is a real reason: to adopt a new API. And compiling KF5 is trivial with kdesrc-build these days. Not to put too fine a point on it, but why should your personal convenience be more important than adopting an API that was specifically made for this use case?

Still, having a single repository to compile is definitely easier, and more welcoming to newcomers.

yurchor added a subscriber: yurchor.Mar 4 2019, 7:28 PM

But there is a real reason: to adopt a new API. And compiling KF5 is trivial with kdesrc-build these days. Not to put too fine a point on it, but why should your personal convenience be more important than adopting an API that was specifically made for this use case?

If you will raise the dependency level that high then I probably cannot test or commit anything during the next several months. Just my 2 cents.

Y'all need to start using kdesrc-build! It makes compiling the frameworks super duper easy! https://community.kde.org/Get_Involved/development#Frameworks

I'm happy to help.

aacid added a comment.Mar 4 2019, 10:53 PM

But there is a real reason: to adopt a new API. And compiling KF5 is trivial with kdesrc-build these days.

No, it's not

aacid added a comment.Mar 4 2019, 10:54 PM

why should your personal convenience be more important than adopting an API that was specifically made for this use case?

:sigh: This is not about *my* personal convenience, this is about making the program be able to be compiled in systems people actually use.

aacid added a comment.Mar 4 2019, 10:56 PM

Not to put too fine a point on it, but why should your personal convenience be more important than adopting an API that was specifically made for this use case?

ifdefs where invented for a reason. I never said not to use the new API, I'm more than happy if the mobile client only builds with the new API, i just want the client that people actually use not require a version of KF5 it does not need.

aacid added inline comments.Mar 4 2019, 11:28 PM
mobile/app/package/contents/ui/Thumbnails.qml
38

This needs to be !== to be 0.00000004 nanoseconds faster (same in the other file)

ognarb updated this revision to Diff 53233.Mar 5 2019, 8:18 PM

Use text.length > 0 insteaf of text != ""

ognarb updated this revision to Diff 53239.Mar 5 2019, 8:55 PM

Use a cmake option to build okular with kde framework 5.56 and okular mobile or wit 5.44 and not okular mobile

aacid added inline comments.Mar 6 2019, 12:11 AM
CMakeLists.txt
13

I would prefer if you left the required version to be 5.44.0 for everything and then inside the if (BUILD_OKULARKIRIGAMI) below you check ${KF5_VERSION} and if it's smaller than 5.56.0 you give a MESSAGE with FATAL_ERROR saying something "You need KF5 version XXX to compile the trouch friendly frontend, if you're not interested in it pass -DSTUFF=OFF to disable it to cmake" or something like that.

Because with your current version it's kind of impossible to know one can disable the kirigami frontend to lower the KF5 requirement (one basically has to read the CMakeLists.txt file)

Do I make sense?

README.md
1 ↗(On Diff #53239)

This file doesn't belong in this commit

ognarb updated this revision to Diff 53268.Mar 6 2019, 11:18 AM
ognarb marked 2 inline comments as done.

Change cmake

aacid added inline comments.Mar 6 2019, 6:55 PM
CMakeLists.txt
129–133

Shouldn't this be 5.55?

ognarb updated this revision to Diff 53338.Mar 7 2019, 8:53 AM
ognarb marked an inline comment as done.

Fix off by on error

@aacid It is ready to land now?

You didn't convert the != to !== as i asked.

Would you mind doing that?

ognarb updated this revision to Diff 53738.Mar 12 2019, 4:11 PM

Use string.length > 0 instead of string == "". It's faster

aacid resigned from this revision.Mar 26 2019, 11:40 PM

I haven't tried this, but if Nathan has, i guess that's good enough.

the sick brain inside me says that

placeholderText: documentItem && documentItem.supportsSearching ?

could be shortened to

placeholderText: enabled ?

but not sure that's better code so feel free to ignore me