Differential D11443

Fixed various bugs related to quicksearch handling of folders
AbandonedPublic
Actions

Authored by rade on Mar 18 2018, 1:11 AM.

Details

Reviewers
martinkostolny
nmel
Group Reviewers
Krusader
Summary

This patch is supposed to be applied on top of D11383, copy of the specific version it's supposed to be applied on top of can be downloaded here

D11383.diff8 KBDownload

What doesn't work with this patch:
I could not figure out how to spawn a submenu on the correct y coordinates. I would really appreciate some help on how to do that. I.e how do I figure out the y position of the matched QAction?

What this patch fixes:
eventfilter is supposed to return true if it has handled an event and doesn't want others to also handle it, "true value prevents the event from being sent on to other objects". This patch adds those return statements. Doing this fixes several bugs related to the handling of folders, e.g folders no longer trigger when the first letter of their name is typed, folders no longer swallow letters once you leave them etc.

Previously setActiveAction was used to open folders. That this worked appears to be a bug (from 2007!). I have switched to using exec.
Since we can't use setActiveAction to select a folder I have added code to try and find something other than a folder that we can select.

Test Plan

Test folder behaviour

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
rade requested review of this revision.Mar 18 2018, 1:11 AM
rade created this revision.
rade edited the summary of this revision. (Show Details)Mar 18 2018, 1:14 AM
nmel requested changes to this revision.Mar 18 2018, 5:53 AM

Hi Rade,

Thanks for the suggested patch. Folder handling is one of the hardest problems we have with the bookmark search the the branch and I'm glad you take on this thing. I also researched this topic a few days ago and confirm there is no public method that could activate a folder item without opening the submenu (there are even more than one related bug in Qt bug tracker, all are very old).

Regarding the problem that you have. I don't think we should switch to exec as we'll have to include all the complex logic regarding submenu positioning. This is too much. Actually, I think that (nMatches == 1) is fine with setActiveAction that opens submenu. This is kind of a trigger for submenu. IMO, the problem is only with multiple matches when we want to highlight submenu without opening it. The swallowing issue is something else, AFAIU. If you think I'm wrong, please explain the mechanics. For now, debug log (without this patch) looks weird:

22:16:57.914-debug default KrBookmarkHandler::_setQuickSearchText@335 # Bookmark search: query = "n"
22:16:57.914-debug default KrBookmarkHandler::eventFilter@552 # Bookmark search: started
22:16:57.916-debug default KrBookmarkHandler::eventFilter@591 # Bookmark search: first match = "network" , number of matches = 1
22:16:57.916-debug default KrBookmarkHandler::_setQuickSearchText@332 # Bookmark search: reset
22:16:57.919-debug default KrBookmarkHandler::_setQuickSearchText@332 # Bookmark search: reset
22:17:07.322-debug default KrBookmarkHandler::_setQuickSearchText@335 # Bookmark search: query = "\u001B"
22:17:07.322-debug default KrBookmarkHandler::eventFilter@552 # Bookmark search: started
22:17:07.322-debug default KrBookmarkHandler::eventFilter@593 # Bookmark search: no matches
22:17:07.322-debug default KrBookmarkHandler::_setQuickSearchText@332 # Bookmark search: reset
22:17:20.249-debug default KrBookmarkHandler::_setQuickSearchText@335 # Bookmark search: query = "d"
22:17:20.250-debug default KrBookmarkHandler::eventFilter@552 # Bookmark search: started
22:17:20.252-debug default KrBookmarkHandler::eventFilter@591 # Bookmark search: first match = "Data root" , number of matches = 3
22:17:20.253-debug default KrBookmarkHandler::_setQuickSearchText@332 # Bookmark search: reset
22:17:20.547-debug default KrBookmarkHandler::_setQuickSearchText@332 # Bookmark search: reset

Here I pressed:

22:16:57.914 - n (the folder has opened)
22:17:07.322 - Esc
22:17:20.249 - d

Do you understand why there are two resets after d? Why is there a significant interval between them?

I was thinking how to solve the folder highlighting and found that I can highlight the item without opening a submenu when I use keyboard to navigate. I looked into QMenu code and the code is all private to QMenu. It seems we can't call it directly, but if we can send keypress events to QMenu, we may be able to workaround the problem. This is just an idea, I haven't tried this approach yet. If you find it feasible, please look into this.

This revision now requires changes to proceed.Mar 18 2018, 5:53 AM
nmel added a project: Krusader.Mar 18 2018, 6:03 AM

Forgot to add:

eventfilter is supposed to return true if it has handled an event and doesn't want others to also handle it, "true value prevents the event from being sent on to other objects". This patch adds those return statements. Doing this fixes several bugs related to the handling of folders, e.g folders no longer trigger when the first letter of their name is typed, folders no longer swallow letters once you leave them etc.

I don't see how return true fixes the issues. I added

if (nMatches >= 1)
    return true;

so it's similar to your patch. I'm unable to notice any change. Please elaborate.

rade updated this revision to Diff 29837.Mar 18 2018, 6:36 PM

I have updated the submenu placement code so it is now correct.

rade added a comment.EditedMar 18 2018, 7:03 PM
In D11443#228341, @nmel wrote:

Regarding the problem that you have. I don't think we should switch to exec as we'll have to include all the complex logic regarding submenu positioning.

I have updated the placement code now and there is no logic. It's just pure function calls. I feel very strongly that relying on a bug is the wrong move. There are no guarantees for when it will stop working. When it inevitably does break then whom ever has to fix it will have a hard time figuring out what the code was supposed to do before it broke as it relies on undocumented behaviour. Now that you can see the final code do you still feel that it is too complex?

Do you understand why there are two resets after d? Why is there a significant interval between them?

I don't know what was happening, but I did notice in your output that it was searching for escape in the form of "\u001B". I have added code to filter those out. That might have been releated to what was happening, but I doubt it. Is the data root a folder?

we can send keypress events to QMenu, we may be able to workaround the problem. This is just an idea, I haven't tried this approach yet. If you find it feasible, please look into this.

This would be an ugly hack to solve an almost none existent use case. Highlighting is only useful when the user happens to be a slow typist that's monitoring what he types instead of just typing until he gets a match _and_ the first hit happens to be what he wants _ and_ this only causes problems if there are folders which match what he types _and_ he wants one of those folders. Requiring that the user type the shortest unique substring of the folder he is trying to access does not seem overly burdensome in my opinion.

I don't see how return true fixes the issues.

I need to look at the issue further perhaps but returning true is the correct behaviour in any case.
To replicate the issue I'm talking about create a folder e.g 'car' and a bookmark called 'clown'. And run revision 82da5a6d. Without the below patch typing c will trigger car with it it won't (the correct behaviour)

totrueornottotrue.patch456 BDownload

rade updated this revision to Diff 29848.EditedMar 18 2018, 7:14 PM

I accidentally broke the accelerator key functionality by always returning true if we had gotten a match, even on the first run. Fixed that. Incidentally this is a good example of why it's important to return the correct value.

nmel added a comment.Mar 18 2018, 8:52 PM
In D11443#228650, @rade wrote:
In D11443#228341, @nmel wrote:

Regarding the problem that you have. I don't think we should switch to exec as we'll have to include all the complex logic regarding submenu positioning.

I have updated the placement code now and there is no logic. It's just pure function calls. I feel very strongly that relying on a bug is the wrong move. There are no guarantees for when it will stop working. When it inevitably does break then whom ever has to fix it will have a hard time figuring out what the code was supposed to do before it broke as it relies on undocumented behaviour. Now that you can see the final code do you still feel that it is too complex?

It's not complex but it doesn't handle all the cases. For example, move krusader close to the right side of the screen and invoke bookmark menu, then search for a folder. Your submenu is not on the correct position. Left arrow also doesn't close the submenu. Sometimes submenu disappears for some reason (I don't have a consistent repro).

Do you understand why there are two resets after d? Why is there a significant interval between them?

I don't know what was happening, but I did notice in your output that it was searching for escape in the form of "\u001B". I have added code to filter those out. That might have been releated to what was happening, but I doubt it. Is the data root a folder?

Yes, it is. The added code may have hidden a problem. Hard to say. I'll try to debug when I have time.

we can send keypress events to QMenu, we may be able to workaround the problem. This is just an idea, I haven't tried this approach yet. If you find it feasible, please look into this.

This would be an ugly hack to solve an almost none existent use case. Highlighting is only useful when the user happens to be a slow typist that's monitoring what he types instead of just typing until he gets a match _and_ the first hit happens to be what he wants _ and_ this only causes problems if there are folders which match what he types _and_ he wants one of those folders. Requiring that the user type the shortest unique substring of the folder he is trying to access does not seem overly burdensome in my opinion.

I tend to agree. I just want the search to be consistent, i.e. it should not matter if we find a folder or a bookmark. Ideally. Given the bug, your patch seems to be good enough as a workaround.

rade added a comment.EditedMar 18 2018, 9:08 PM

It's not complex but it doesn't handle all the cases. For example, move krusader close to the right side of the screen and invoke bookmark menu, then search for a folder. Your submenu is not on the correct position.

Did you try it after I updated the code? Did it work before this patch? When I read the code for popup() earlier today, in qmenu.cpp, this was one of the cases I noted that it does explicitly handle and it works fine on my computer. I.e if I move the program halfway between my left monitor and my right monitor so that there is not enough room to the right for the complete menu. Then the menu gets positioned further left so that the right most part of the menu is touching the right edge of the left monitor, like would be expected.

I was not aware that the left arrow shortcut existed, I only tested with escape. I'll look into it.

rade updated this revision to Diff 29872.Mar 18 2018, 11:38 PM

I've added support for the left arrow key closing menus. It works in all menus even the main one. I would argue that this is what is expected behaviour though. E.g if I'm three levels into submenus and want to close all of them I want to press leftarrow, leftarrow, leftarrow and _not_ have to press leftarrow, leftarrow, escape.

I have also added code to filter the return key. It was sending the text '\r' so there for getting past the 'kev->text() == ""' check.

nmel added a comment.Mar 19 2018, 6:52 AM
In D11443#228774, @rade wrote:

It's not complex but it doesn't handle all the cases. For example, move krusader close to the right side of the screen and invoke bookmark menu, then search for a folder. Your submenu is not on the correct position.

Did you try it after I updated the code? Did it work before this patch?

I tested the updated patch. It's still a problem with the current patch:
Navigated with mouse:


Navigated with search:

It did work before the patch.

nmel requested changes to this revision.Mar 20 2018, 7:11 AM

I propose we split this into three reviews (don't worry about Summary and Test Plan at this point):

  • Return value fixes
  • Setting active item workaround (ignoring folders)
  • setActiveAction replacement

Logically it's 3 commits, so we need to split this at some point anyway. Since the changes are independent, it's much easier to discuss and review.

Thanks!

krusader/BookMan/krbookmarkhandler.cpp
587

firstMatch no longer corresponds to its name. It's the last match of a folder or the first match of a bookmark. Either rename (what's a good name?) or use two vars firstBookmarkMatch and firstFolderMatch.

627

Why there is no return true in this case?

This revision now requires changes to proceed.Mar 20 2018, 7:11 AM
rade added a comment.EditedMar 20 2018, 10:36 AM

I agree that splitting it in to 3 commits is the right move and I'll try to do this later today if I can find the time.

Regarding the setActiveAction fix. I investigated the layout issue yesterday and came to the conclusion that it was a arguably a bug in qmenu.cpp, at least that's how I interpret the documentation. I have created a patch that I will submit upstream to Qt that fixes it. I have also created a separate patch that makes it is possible to highlight submenu items that I will also submit to them.
We can discuss further how to proceed with those issues once I have split them.

rade added inline comments.Mar 20 2018, 9:12 PM
krusader/BookMan/krbookmarkhandler.cpp
627

because we didn't find a match, so we haven't dealt with the button press and are sending it along to others who might be able to handle it.

rade abandoned this revision.Mar 21 2018, 12:51 PM

I split this into separate reviews per nmels request.

Revision Contents
Changeset List

Diff 29837

View Options

krusader/BookMan/krbookmarkhandler.cpp

Loading...