D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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
  https://phabricator.kde.org/D18939

AFFECTED FILES
  kmenuedit.cpp
  kmenuedit.h
  treeview.cpp
  treeview.h

To: tuxxi, ngraham, #plasma, cfeck
Cc: ognarb, mlaurent, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Christoph Feck]

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, 
> otherwise collapse and un-select everything
> +if (searchString.length() > 2) {
> +expandAll();

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

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, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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 patch! Should be a nice improvement. And you may end up 
becoming the person who's fixed the oldest bug ever. :)

REPOSITORY
  R103 KMenu Editor

BRANCH
  search-bar

REVISION DETAIL
  https://phabricator.kde.org/D18939

To: tuxxi, ngraham, #plasma, cfeck
Cc: mlaurent, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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 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 :)

REPOSITORY
  R103 KMenu Editor

REVISION DETAIL
  https://phabricator.kde.org/D18939

To: tuxxi, ngraham, #plasma, cfeck
Cc: mlaurent, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Laurent Montel]

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 Qt::CaseInsensitive

REPOSITORY
  R103 KMenu Editor

REVISION DETAIL
  https://phabricator.kde.org/D18939

To: tuxxi, ngraham, #plasma, cfeck
Cc: mlaurent, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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 Editor

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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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
  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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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
  kmenuedit.cpp
  kmenuedit.h
  treeview.cpp
  treeview.h

To: tuxxi, ngraham, #plasma, cfeck
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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 view to the right, so they don't line up anymore:
  
  F6612255: Screenshot_20190212_195729.png 

  F6612252: Screenshot_20190212_195346.png 

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

REPOSITORY
  R103 KMenu Editor

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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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 your git authorship information to use your 
full name. If I try to land this now, the commit hookscript will complain that 
`tuxxi ` 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

REPOSITORY
  R103 KMenu Editor

BRANCH
  search-bar

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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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

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

REPOSITORY
  R103 KMenu Editor

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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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
  https://phabricator.kde.org/D18939

AFFECTED FILES
  kmenuedit.cpp
  kmenuedit.h
  treeview.cpp
  treeview.h

To: tuxxi, ngraham, #plasma, cfeck
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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 clean-up like this separately.

> tuxxi wrote in kmenuedit.cpp:141
> I copied the tooltip text from KOpenWithDialog, but I'll change this one. 
> Perhaps we can change it as well

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

> tuxxi wrote in kmenuedit.cpp: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`.

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.

REPOSITORY
  R103 KMenu Editor

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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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 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"?

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

> ngraham wrote in kmenuedit.cpp:143
> No need to use `auto` here

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`.

REPOSITORY
  R103 KMenu Editor

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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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
  https://phabricator.kde.org/D18939

AFFECTED FILES
  kmenuedit.cpp
  kmenuedit.h
  treeview.cpp
  treeview.h

To: tuxxi, ngraham, #plasma, cfeck
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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..."));
> +m_searchLine->setToolTip(i18n("Type to filter applications below"));
> +

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"?

> kmenuedit.cpp:143
> +
> +auto treeLayout = new QVBoxLayout;
> +treeLayout->addWidget(m_searchLine);

No need to use `auto` here

> kmenuedit.cpp:146
> +treeLayout->addWidget(m_tree);
> +auto treeFrame = new QFrame;
> +treeFrame->setLayout(treeLayout);

No need to use `auto` here

> kmenuedit.cpp:157
>  
> +
>  connect(m_tree, SIGNAL(entrySelected(MenuFolderInfo*)),

Unrelated change

> treeview.cpp:1922
> +/*
> + * Workaround to set expansion of categories when search updates
> + */

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

> treeview.h:208
>  
> +
>  protected:

Unrelated change

REPOSITORY
  R103 KMenu Editor

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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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
  https://phabricator.kde.org/D18939

AFFECTED FILES
  kmenuedit.cpp
  kmenuedit.h
  treeview.cpp
  treeview.h

To: tuxxi, ngraham, #plasma, cfeck
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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);
> +m_searchLine->setPlaceholderText(i18n("Search"));
> +m_searchLine->setToolTip(i18n("Type to filter applications below"));

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

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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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
  kmenuedit.cpp
  kmenuedit.h
  treeview.cpp
  treeview.h

To: tuxxi, ngraham, #plasma, cfeck
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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 automatically, then I 
can't help you since I'm not familiar with that IDE.

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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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 
search boxes:

- No explanatory label to the left
- Placeholder text that says "Search..."

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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Aidan Sojourner]

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 clang-tidy style)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18939?vs=51457=51468

REVISION DETAIL
  https://phabricator.kde.org/D18939

AFFECTED FILES
  .gitignore
  kmenuedit.cpp
  kmenuedit.h
  treeview.cpp
  treeview.h

To: tuxxi, ngraham, #plasma, cfeck
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18939: KMenuEdit: add search/filter bar

[Nathaniel Graham]

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 Editor

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, sebas, apol, mart