Differential D11906

Fix casesensitive search for quicksearch-bookmark
ClosedPublic
Actions

Authored by rade on Apr 3 2018, 5:56 PM.

Details

Reviewers
nmel
martinkostolny
asensi
Group Reviewers
Krusader
Commits
R167:296b099f6b39: Added support for capitalization when searching bookmarks
Summary

As noted by asensi in D11898 shift is erroneously being filtered out.
This patch fixes that.
I decided that if the user types a capital letter then we should only match entries that have the same case for the entire string. E.g typing 'hamBurglar' will not match 'HamBurglar' but typing 'hamburglar' or 'HamBurglar' will. This allows quicker access too bookmarks with capital letters. Eg with the bookmarks 'HamBurglar' and 'hamhorder' the user can type 'H' and instantly match 'HamBurglar'.

Requiring the user to either match all case or none of it feels both fair and easy for the user to understand. Matching things with a partial case matches feels like it will cause too much confusion. For example should typing 'hamB' match 'HamBurglar' and 'hamBurglar'? What about the case when there is only the 'HamBurglar' bookmark should it match it then? Etc.

Test Plan

Make sure all other modifiers are correctly filtered out.
Test matching strings with different capitalisation.

Diff Detail

Repository
R167 Krusader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rade requested review of this revision.Apr 3 2018, 5:56 PM
rade created this revision.
rade mentioned this in D11898: Implemented bookmark quick search.
rade edited the summary of this revision. (Show Details)Apr 3 2018, 5:59 PM
rade edited the summary of this revision. (Show Details)
martinkostolny accepted this revision.Apr 3 2018, 10:10 PM

Thanks for fixing the shift scenarios. And good idea with the smart case-matching, I like that :).

Just for info to other reviewers: to successfully apply this patch one need to use the dedicated bookmark-quicksearch branch and checkout commit: c118ca1eb5fd6d4cb133d5f644a9004a3c65d4dd

This revision is now accepted and ready to land.Apr 3 2018, 10:10 PM
nmel accepted this revision.Apr 4 2018, 6:51 AM

Thanks for the fix.

Matching things with a partial case matches feels like it will cause too much confusion. For example should typing 'hamB' match 'HamBurglar' and 'hamBurglar'?

I agree it's a bit strange because before typing B the search matched both of them.

What about the case when there is only the 'HamBurglar' bookmark should it match it then? Etc.

In this case 'ham' will already select the bookmark.

Thanks for thinking it through. I accept because the benefit of quicker capital letter matching exceed minor inconsistencies, IMO. We'll probably need this documented to avoid bug submissions later.

Do you mind sending format-patch from the top of the branch? Thanks!

nmel added a reviewer: asensi.Apr 4 2018, 6:51 AM
rade updated this revision to Diff 31266.Apr 4 2018, 9:41 AM

merge with latest commit

rade added a comment.EditedApr 4 2018, 9:41 AM
In D11906#239529, @nmel wrote:

Do you mind sending format-patch from the top of the branch? Thanks!

As requested

0001-Added-support-for-capitalization-when-searching-book.patch2 KBDownload

asensi added a comment.Apr 4 2018, 12:29 PM

Hi! I'm using the "bookmark-quicksearch" branch, and I have applied the present diff, which is:

diff --git a/krusader/BookMan/krbookmarkhandler.cpp b/krusader/BookMan/krbookmarkhandler.cpp
--- a/krusader/BookMan/krbookmarkhandler.cpp
+++ b/krusader/BookMan/krbookmarkhandler.cpp
@@ -592,12 +592,12 @@
            return true;
        }

-        if (kev->modifiers() != Qt::NoModifier ||
-            kev->text().isEmpty()              ||
-            kev->key() == Qt::Key_Delete       ||
-            kev->key() == Qt::Key_Return       ||
+        if ((kev->modifiers() != Qt::ShiftModifier &&
+             kev->modifiers() != Qt::NoModifier) ||
+            kev->text().isEmpty()                ||
+            kev->key() == Qt::Key_Delete         ||
+            kev->key() == Qt::Key_Return         ||
            kev->key() == Qt::Key_Escape) {
-
            return QObject::eventFilter(obj, ev);
        }

@@ -623,6 +623,8 @@
        // match actions
        QAction *matchedAction = nullptr;
        int nMatches = 0;
+        const Qt::CaseSensitivity matchCase =
+            _quickSearchText() == _quickSearchText().toLower() ? Qt::CaseInsensitive : Qt::CaseSensitive;
        for (auto act : acts) {
            if (act->isSeparator() || act->text() == "") {
                continue;
@@ -647,7 +649,7 @@
            }

            // match prefix of the action text to the query
-            if (act->text().left(_quickSearchText().length()).compare(_quickSearchText(), Qt::CaseInsensitive) == 0) {
+            if (act->text().left(_quickSearchText().length()).compare(_quickSearchText(), matchCase) == 0) {
                _highlightAction(act);
                if (!matchedAction || matchedAction->menu()) {
                    // Can't highlight menus (see comment below), hopefully pick something we can

later I have compiled and installed Krusader, but the behavior seems to be the same (e.g. when typing 'H' it doesn't match the bookmark 'HamBurglar'). Is there any other step that must be taken?

rade added a comment.Apr 4 2018, 12:38 PM
In D11906#239745, @asensi wrote:

later I have compiled and installed Krusader, but the behavior seems to be the same (e.g. when typing 'H' it doesn't match the bookmark 'HamBurglar'). Is there any other step that must be taken?

No there shouldn't be. I just tested applying it on top of 89ee1833bedfa9618e2ecdae814b8648e661b821 with

patch -p1 < D11906.patch

and it works for me.

asensi accepted this revision.Apr 4 2018, 12:54 PM

It worked as it is expected, OK!

martinkostolny accepted this revision.Apr 4 2018, 4:26 PM

Works on top of latest branch commit. Thanks! :)

Closed by commit R167:296b099f6b39: Added support for capitalization when searching bookmarks (authored by rade, committed by nmel). · Explain WhyApr 5 2018, 4:45 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 31349

View Options

krusader/BookMan/krbookmarkhandler.cpp

Loading...