Add search/filter bar
ClosedPublic

Authored by tuxxi on Feb 12 2019, 5:40 AM.

Details

Summary

This diff contains a much-requested feature: adding a search or filter bar to KMenuEdit's application tree view. The search bar automatically expands categories as you search, then un-expands when search is cleared.

FEATURE: 57314
FIXED-IN: 5.16.0

Before:

After:

Test Plan

KMenuEdit's codebase is a mess; I noticed that there was only one unit test run by ctest! I'd love to write more tests in the future, but for this feature we don't need much.

Diff Detail

Repository
R103 KMenu Editor
Branch
search-bar
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8220
Build 8238: arc lint + arc unit
tuxxi created this revision.Feb 12 2019, 5:40 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 12 2019, 5:40 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
tuxxi requested review of this revision.Feb 12 2019, 5:40 AM
tuxxi edited the summary of this revision. (Show Details)
tuxxi edited the summary of this revision. (Show Details)Feb 12 2019, 5:46 AM
tuxxi edited the summary of this revision. (Show Details)
tuxxi retitled this revision from KMenuEdit search/filter to KMenuEdit: add search/filter bar.
ngraham requested changes to this revision.Feb 12 2019, 5:47 AM
ngraham added reviewers: Plasma, cfeck.
ngraham added a subscriber: ngraham.

Thanks!

All of those .gitignore changes look unintentional and unrelated and will need to be reverted.

This revision now requires changes to proceed.Feb 12 2019, 5:47 AM
tuxxi edited the summary of this revision. (Show Details)Feb 12 2019, 6:20 AM
tuxxi updated this revision to Diff 51468.Feb 12 2019, 7:05 AM
  • Switch to using QTreeView's methods for expand/collapse
  • Filter threshold is 2 chars like KOpenWithDialog to avoid cluttering
  • Reverted unnecessary changes (will do a different revision for refactor, error fixing, and clang-tidy style)

This revision still has unrelated .gitignore changes.

ngraham added inline comments.Feb 12 2019, 5:52 PM
kmenuedit.cpp
143

In general, we're trying to standardize on a single KDE-wide style for our search boxes:

  • No explanatory label to the left
  • Placeholder text that says "Search..."
tuxxi added a comment.Feb 12 2019, 7:59 PM

Hi ngraham:

OK, can do on the label / placeholder.
RE: .gitignore, those files are generated by my IDE (CLion). What do you suggest I do?

I suggest not adding them to the patch. :)

Since they've already been added to this patch, you can do git checkout .gitignore to recover the original version of that file, and then arc diff to update the patch. But if your IDE adds the file again automatically, then I can't help you since I'm not familiar with that IDE.

tuxxi updated this revision to Diff 51540.Feb 12 2019, 8:10 PM
  • Removed .gitignore changes
  • Switch label to placeholder text

Thanks. :)

You can also click on the "Resolved" checkboxes for inline comments and then click the Submit button on the bottom of the page once those issues are taken care of.

kmenuedit.cpp
140

Close! It needs ellipses ("Search...")

tuxxi updated this revision to Diff 51545.Feb 12 2019, 8:47 PM
tuxxi marked an inline comment as done.
  • search -> search...
tuxxi marked an inline comment as done.Feb 12 2019, 8:57 PM

Thanks! The UI and behavior are now good. Code looks pretty sane too, just a few style nitpicks:

kmenuedit.cpp
30

Unrelated change.

141

Since the placeholder text says "Search..." we should probably not use the word "Filter" in the tooltip. Also "Type to" isa not really necessary since that's the only thing you can do with a search field. :)

How about something like "Search through the list of applications below"?

143

No need to use auto here

146

No need to use auto here

157

Unrelated change

treeview.cpp
1922

I'm not sure this is actually working around anything; it's just a nice UI behavior! :)

treeview.h
208

Unrelated change

tuxxi updated this revision to Diff 51561.Feb 12 2019, 11:38 PM
tuxxi marked 5 inline comments as done.

Fix various nitpicks

tuxxi added inline comments.Feb 12 2019, 11:40 PM
kmenuedit.cpp
30

IMHO it looks cleaner to separate includes into categories, which is what I did here.

141

I copied the tooltip text from KOpenWithDialog, but I'll change this one. Perhaps we can change it as well

143

I'm using auto to avoid duplicating the type name when using new since the type is right there; its a habit I got into since clang-tidy recommends it. I couldn't find anything in the style guides prohibiting this. If you _really_ want, I can stop using auto.

ngraham added inline comments.Feb 13 2019, 12:53 AM
kmenuedit.cpp
30

It surely is, but that's not related to this patch. In KDE, we try to make our commits as atomic as possible, and do clean-up like this separately.

141

Yep, sounds like we should change it it there too! A good second patch. :)

143

If there's nothing specifically in the style guidelines about it, it's best to follow the existing coding style. Nothing else here uses auto with new constructors, so we should follow the same style for new code.

KDE software is multi-generational and it's important that each individual developer not use their own personal preferred style instead of following the existing style because that leads to the whole codebase becoming an inconsistent mess over time. Cleanup can be desirable, but that should happen separately, in its own patch, so it can be discussed on its own merits.

tuxxi updated this revision to Diff 51562.Feb 13 2019, 1:07 AM
tuxxi marked 8 inline comments as done.
  • resolve (hopefully) final nitpicks
tuxxi added inline comments.Feb 13 2019, 1:08 AM
kmenuedit.cpp
143

Okay, makes sense. I'll try to be more atomic :)

tuxxi edited the summary of this revision. (Show Details)Feb 13 2019, 1:09 AM
ngraham accepted this revision.Feb 13 2019, 2:51 AM

Excellent. UX and code look good to me now. @cfeck, or Plasma folks, any comments or shall we land this?

BTW @tuxxi you'll need to fix your git authorship information to use your full name. If I try to land this now, the commit hookscript will complain that tuxxi <aidan.sojourner@gmail.com> isn't valis because the name part doesn't have a last name. Assuming your email address reveals your real name, I can fix this up for you before it lands, but it would be nice not to have to in the future, ya know? :-)

See https://community.kde.org/Get_Involved/development#Configure_Git

This revision is now accepted and ready to land.Feb 13 2019, 2:51 AM
ngraham requested changes to this revision.Feb 13 2019, 2:58 AM

Actually looking one more time I see a visual issue introduced by this patch: by putting the search field and list inside a frame, they both have bigger margins than the tabbed view to the right, so they don't line up anymore:


Let's revert the treeFrame part, which should fix this.

This revision now requires changes to proceed.Feb 13 2019, 2:58 AM
tuxxi updated this revision to Diff 51565.EditedFeb 13 2019, 3:21 AM
  • Fix padding issues

Since we're revamping the UI a bit I added 5px of padding to the left and right of the splitter, IMHO it looks a lot better this way. See updated screenshots

tuxxi edited the summary of this revision. (Show Details)Feb 13 2019, 3:22 AM

If the reason why you're creating a QFrame is because QSplitter doesn't accept a layout, you can just give it a blank QWidget and assign the widget your layout. See https://stackoverflow.com/a/53384177/2934226

tuxxi added a comment.Feb 13 2019, 4:08 AM

Yes, that's exactly what I'm doing. QFrame works fine for doing the same thing with the added benefit of easier styling around the edges. But if you *really want* I can switch it to a blank QWidget... it makes absolutely no difference though.

mlaurent added inline comments.
kmenuedit.cpp
138

you can use directly Qt::CaseInsensitive

If you use a blank QWidget, you won't have to do that setContentsMargin() workaround, right?

tuxxi added a comment.Feb 13 2019, 6:02 PM

If you use a blank QWidget, you won't have to do that setContentsMargin() workaround, right?

I actually tried this first, I was surprised when the plain QWidget still had margins; I realized the margins were part of the QVBoxLayout rather than in the widget containing it. I switched back to a QFrame since I've used it for this use in the past -- but it really makes no difference in this case, so I can switch it to a QWidget if you think that's better style :)

ngraham accepted this revision.Feb 13 2019, 6:06 PM

Weird. Well, this look good to me from a UX and basic code sanity check perspective, so I'll hand it over to other folks to do the more in-depth code review now.

Thanks for the patch! Should be a nice improvement. And you may end up becoming the person who's fixed the oldest bug ever. :)

This revision is now accepted and ready to land.Feb 13 2019, 6:06 PM
ognarb added a subscriber: ognarb.Feb 18 2019, 10:59 AM
cfeck added inline comments.Feb 18 2019, 8:11 PM
kmenuedit.h
60

Surrounding code uses Type *var.

treeview.cpp
1924

Maybe use KStringHandler::logicalLength() for CJK users.

tuxxi updated this revision to Diff 52129.Feb 20 2019, 2:47 AM
  • Use logicalLength for non-latin characters
  • Fix ptr style
tuxxi marked 2 inline comments as done.Feb 20 2019, 2:48 AM

Good point about CJK, I had not considered that.

ngraham accepted this revision.Feb 21 2019, 5:46 PM

LGTM. @cfeck, are you happy now?

ngraham retitled this revision from KMenuEdit: add search/filter bar to Add search/filter bar.Feb 26 2019, 4:37 AM
ngraham edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.

Thanks again for the contribution, @tuxxi! If you'd like to submit more patches to clean up the codebase a bit and add unit tests, nobody's gonna complain. :-)