D18939: KMenuEdit: add search/filter bar

ngraham accepted this revision. ngraham added a comment. LGTM. @cfeck, are you happy now? REPOSITORY R103 KMenu Editor BRANCH search-bar REVISION DETAIL https://phabricator.kde.org/D18939 To: tuxxi, ngraham, #plasma, cfeck Cc: ognarb, mlaurent, ngraham, plasma-devel, jraleigh, GB_2,

D18939: KMenuEdit: add search/filter bar

tuxxi marked 2 inline comments as done. tuxxi added a comment. Good point about CJK, I had not considered that. REPOSITORY R103 KMenu Editor BRANCH search-bar REVISION DETAIL https://phabricator.kde.org/D18939 To: tuxxi, ngraham, #plasma, cfeck Cc: ognarb, mlaurent, ngraham,

D18939: KMenuEdit: add search/filter bar

tuxxi updated this revision to Diff 52129. tuxxi added a comment. - Use logicalLength for non-latin characters - Fix ptr style REPOSITORY R103 KMenu Editor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18939?vs=51565=52129 BRANCH search-bar REVISION DETAIL

D18939: KMenuEdit: add search/filter bar

cfeck added inline comments. INLINE COMMENTS > kmenuedit.h:60 > QSplitter *m_splitter = nullptr; > +KTreeWidgetSearchLine* m_searchLine = nullptr; > Surrounding code uses `Type *var`. > treeview.cpp:1924 > +// expand all categories if we typed more than a few characters, >

D18939: KMenuEdit: add search/filter bar

ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. 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

D18939: KMenuEdit: add search/filter bar

tuxxi added a comment. In D18939#411450 , @ngraham wrote: > 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

D18939: KMenuEdit: add search/filter bar

ngraham added a comment. If you use a blank QWidget, you won't have to do that `setContentsMargin()` workaround, right? REPOSITORY R103 KMenu Editor REVISION DETAIL https://phabricator.kde.org/D18939 To: tuxxi, ngraham, #plasma, cfeck Cc: mlaurent, ngraham, plasma-devel, jraleigh,

D18939: KMenuEdit: add search/filter bar

mlaurent added inline comments. INLINE COMMENTS > kmenuedit.cpp:137 > +m_searchLine = new KTreeWidgetSearchLine(this, m_tree); > +m_searchLine->setCaseSensitivity(Qt::CaseSensitivity::CaseInsensitive); > +m_searchLine->setKeepParentsVisible(true); you can use directly

D18939: KMenuEdit: add search/filter bar

tuxxi added a comment. 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. REPOSITORY R103 KMenu

D18939: KMenuEdit: add search/filter bar

ngraham added a comment. 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 REPOSITORY R103 KMenu Editor REVISION DETAIL

D18939: KMenuEdit: add search/filter bar

tuxxi updated this revision to Diff 51565. tuxxi added a comment. - Fix padding issues REPOSITORY R103 KMenu Editor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18939?vs=51562=51565 BRANCH search-bar REVISION DETAIL https://phabricator.kde.org/D18939 AFFECTED FILES

D18939: KMenuEdit: add search/filter bar

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. 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

D18939: KMenuEdit: add search/filter bar

ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. 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

D18939: KMenuEdit: add search/filter bar

tuxxi added inline comments. INLINE COMMENTS > ngraham wrote in kmenuedit.cpp: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

D18939: KMenuEdit: add search/filter bar

tuxxi updated this revision to Diff 51562. tuxxi marked 8 inline comments as done. tuxxi added a comment. - resolve (hopefully) final nitpicks REPOSITORY R103 KMenu Editor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18939?vs=51561=51562 BRANCH search-bar REVISION DETAIL

D18939: KMenuEdit: add search/filter bar

ngraham added inline comments. INLINE COMMENTS > tuxxi wrote in kmenuedit.cpp:30 > IMHO it looks cleaner to separate includes into categories, which is what I > did here. 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

D18939: KMenuEdit: add search/filter bar

tuxxi added inline comments. INLINE COMMENTS > ngraham wrote in kmenuedit.cpp:30 > Unrelated change. IMHO it looks cleaner to separate includes into categories, which is what I did here. > ngraham wrote in kmenuedit.cpp:141 > Since the placeholder text says "Search..." we should probably not

D18939: KMenuEdit: add search/filter bar

tuxxi updated this revision to Diff 51561. tuxxi marked 5 inline comments as done. tuxxi added a comment. Fix various nitpicks REPOSITORY R103 KMenu Editor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18939?vs=51545=51561 BRANCH search-bar REVISION DETAIL

D18939: KMenuEdit: add search/filter bar

ngraham added a comment. Thanks! The UI and behavior are now good. Code looks pretty sane too, just a few style nitpicks: INLINE COMMENTS > kmenuedit.cpp:30 > #include > + > #include Unrelated change. > kmenuedit.cpp:141 > +m_searchLine->setPlaceholderText(i18n("Search...")); > +

D18939: KMenuEdit: add search/filter bar

tuxxi updated this revision to Diff 51545. tuxxi marked an inline comment as done. tuxxi added a comment. - search -> search... REPOSITORY R103 KMenu Editor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18939?vs=51540=51545 BRANCH search-bar REVISION DETAIL

D18939: KMenuEdit: add search/filter bar

ngraham added a comment. 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. INLINE COMMENTS > kmenuedit.cpp:140 > +m_searchLine->setKeepParentsVisible(true);

D18939: KMenuEdit: add search/filter bar

tuxxi updated this revision to Diff 51540. tuxxi added a comment. - Removed `.gitignore` changes - Switch label to placeholder text CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18939?vs=51468=51540 REVISION DETAIL https://phabricator.kde.org/D18939 AFFECTED FILES

D18939: KMenuEdit: add search/filter bar

ngraham added a comment. 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

D18939: KMenuEdit: add search/filter bar

tuxxi added a comment. 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? REVISION DETAIL https://phabricator.kde.org/D18939 To: tuxxi, ngraham, #plasma, cfeck Cc: ngraham, plasma-devel,

D18939: KMenuEdit: add search/filter bar

ngraham added inline comments. INLINE COMMENTS > kmenuedit.cpp:143 > +auto searchLayout = new QHBoxLayout; > +searchLayout->addWidget(new QLabel(i18n("Filter:"))); > +searchLayout->addWidget(m_searchLine); In general, we're trying to standardize on a single KDE-wide style for our

D18939: KMenuEdit: add search/filter bar

ngraham added a comment. This revision still has unrelated `.gitignore` changes. REVISION DETAIL https://phabricator.kde.org/D18939 To: tuxxi, ngraham, #plasma, cfeck Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts,

D18939: KMenuEdit: add search/filter bar

tuxxi updated this revision to Diff 51468. tuxxi added a comment. - 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

D18939: KMenuEdit: add search/filter bar

ngraham requested changes to this revision. ngraham added reviewers: Plasma, cfeck. ngraham added a comment. This revision now requires changes to proceed. Thanks! All of those `.gitignore` changes look unintentional and unrelated and will need to be reverted. REPOSITORY R103 KMenu