Implement search function
ClosedPublic

Authored by rthomsen on Dec 2 2016, 9:12 PM.

Details

Reviewers
elvisangelaccio
colomar
Group Reviewers
VDG
Summary

An action was added to Archive menu. The search is mediated through KRecursiveFilterProxyModel instead of QSortFilterProxyModel because the latter does not recurse nested models. This adds a new dependency on the KItemModels framework.

The search bar is displayed above the QTreeView and contains a close button. An eventfilter was installed on Part to catch the escape keypress to close the search bar.

Things to consider:

  1. The filter is currently always case insensitive. Do we want to add a button to switch between case insensitive/sensitive?
  2. Should the Find action be present on the default toolbar?

Fixes bug 188197.



Test Plan

Open any archive, press CTRL+F and search!

Diff Detail

Repository
R36 Ark
Lint
Lint Skipped
Unit
Unit Tests Skipped
rthomsen updated this revision to Diff 8706.Dec 2 2016, 9:12 PM
rthomsen retitled this revision from to Implement search function.
rthomsen updated this object.
rthomsen edited the test plan for this revision. (Show Details)
rthomsen added a reviewer: elvisangelaccio.
rthomsen set the repository for this revision to R36 Ark.
rthomsen added a project: Ark.
Restricted Application added a subscriber: kde-utils-devel. · View Herald TranscriptDec 2 2016, 9:12 PM
rthomsen updated this revision to Diff 8709.Dec 2 2016, 9:19 PM

Change behavior so activating the search action a second time doesn't close the search bar. It's now consistent with other apps.

rthomsen updated this object.Dec 2 2016, 9:21 PM

Let's also involve the VDG in the review, please add screenshots for them.

Comments from my side:

  • A case-sensitive button like in Kate would be good, but can be done in the next iteration. Same for the green/red color that Kate uses for the search line edit.
  • Given that the line edit has the "Type to search..." hint (which is good, neither dolphin nor kate has it!), I think that the Find: label is redundant and can be dropped.
  • I never noticed that both Dolphin and Kate have the close button on the left of the search bar, I find it odd. What does the VDG think? Should we put the close button on the left or on the right?
  • I don't think the ESC key is a big deal, but if it's easy to replicate the Kate behavior then we could do it.
  • I never noticed that both Dolphin and Kate have the close button on the left of the search bar, I find it odd. What does the VDG think? Should we put the close button on the left or on the right?

Actually almost all kde apps put it on the left (okular, konversation, ...). So does the HIG guideline: https://community.kde.org/KDE_Visual_Design_Group/HIG/SearchPattern

Still, I'm not fully convinced but I guess we should stick to the "de-facto" standard...

rthomsen updated this object.Dec 3 2016, 11:45 AM
rthomsen edited edge metadata.
rthomsen updated this object.
rthomsen updated this revision to Diff 8715.Dec 3 2016, 12:29 PM

Disable Find action when Part is busy or when model is empty.

rthomsen updated this revision to Diff 8793.EditedDec 5 2016, 7:40 PM
rthomsen updated this object.

Make the Escape key also work when the QLineEdit is not in focus.

colomar edited edge metadata.Dec 10 2016, 5:35 PM

I'm sorry that I have not commented earlier, I blame it on an overflowing inbox :-/

First of all: Great that you're doing this, it's definitely a useful feature!

Now for the questions:

  • Yes, please keep the close button on the left. There is no clear advantage or disadvantage to either side, so just keeping consistent with other applications (and the HIG) is the way to go. Also, on the right side it could be confused with the similar-looking clear button
  • The HIG recommends to put a label next to the field, but actually this is not really standard anymore, so we meant to change that in the HIG at some point. Please remove the label next to the field, as the text in the field is sufficient to indicate what it does
  • Yes, put a find button int he toolbar by default, for consistency with Dolphin
  • I think case-sensitive search is overkill for a file name search. Not even Dolphin has it.

Further comment:
Please change the label in the menu (and the tooltip for the button) to "Find Files" (because it lives in the "Archive" menu, so people might mistake it as "Find Archives".

rthomsen updated this revision to Diff 8922.Dec 11 2016, 10:37 AM
rthomsen edited edge metadata.

Implement Thomas' suggestions. The toolbar action is placed last, together with the Add Files and Delete actions.

rthomsen updated this object.Dec 11 2016, 11:26 AM
rthomsen updated this object.Dec 11 2016, 11:36 AM
elvisangelaccio accepted this revision.Dec 16 2016, 12:03 PM
elvisangelaccio edited edge metadata.
elvisangelaccio added inline comments.
part/part.h
184

Make it public (this is public in QObject) and add Q_DECL_OVERRIDE

This revision is now accepted and ready to land.Dec 16 2016, 12:03 PM
rthomsen closed this revision.Dec 17 2016, 3:08 PM

Committed to master with commit 7781d6ef794a.