- User Since
- Mar 1 2018, 9:33 PM (72 w, 1 d)
Apr 8 2018
Apr 4 2018
merge with latest commit
Apr 3 2018
Good catch! I have created a fix for that here D11906
Apr 2 2018
First off I'm marking the revision as accepted as either you agree with my points below and apply the patch which obviously I would agree with, or you don't agree and keep it as is which is also fine.
Either way this is a really good patch.
Thanks again for figuring out how to fix this issue!
Apr 1 2018
another QCloseEvent event for the submenu after ~200ms through QTimer
after which the active item is reset.
Can't have been easy figuring out that out!
Have we been extremely unlucky running in to three different Qt5 bugs developing this feature or is Qt5 just this buggy?
Mar 29 2018
Seems you were right about the jump back being reset martin. I feel into the trap of since I 'knew' it wasn't being reset since my code wasn't calling setjumpback and my cursory (and faulty) test passed and seemed to confirm it wasn't being reset then all was fine and dandy.
Accidentally submitted the previous to last revision.
Mar 27 2018
This issue was closed just as I was commenting... Might be too late now but I just wanted to add that it might be a good idea to add a comment explaining the reason why we are removing quickssearchaction from the old menu as that is not really obvious by just reading the code.
Mar 25 2018
Other then the issue martin pointed out it looks good!
Updated the code so that start calls openUrl.
Mar 24 2018
I changed the local path behaviour, even though I don't think this code could be called with a remote path, there is no reason why it shouldn't be able to handle it.
right clicking on them just closes the menu
Right click actually executes the action currently. I think this is standard behavior for menus.
It didn't use to be standard behaviour for this menu, it used to be that right clicking on them you got the same options as you get when right clicking on trash et al. AFIK it's not consistent with other menus either. The only menu I could find where right clicking doesn't give you a pop up is "Useractions" and there right clicking simply does nothing. I could find no menus where right clicking executes the action, nor would a user expect right clicking to trigger the action.
Given Rade's points, I see that it may be useful for some people, so I suggest we add another config option "Search in special items" for this feature and stop arguing. My only ask is to leave it off by default.
Mar 23 2018
Changed return code so that we return true, even if we couldn't find a matching bookmark or folder
Mar 21 2018
When looking through the menu I discovered a bug though but that is unrelated to this code.
I have changed to using git format-patch, and phabricator doesn't like one file with several diffs in it apparently so I made them into one full diff.
Added return statement from D11521 and renamed firstMatch
I split this into separate reviews per nmels request.
removed the extra newline.
You could add back the the shortcut text only using
QString shortcutText = sourceAction->shortcut().toString();
Mar 20 2018
Read through the code and it looks good.
One 'problem' though is that the shortcut 'Ctrl+j' and 'Ctrl+shift+j' used to be displayed next to jump back point and set jump back point the same way they still are in the go menu. That should probably be fixed.
fixed minor error
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.
Mar 18 2018
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.
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.
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.
I have updated the submenu placement code so it is now correct.
Mar 17 2018
Good to hear. However it's definitely not present on the man page that shipped with my distro. Dunno if it's a recent addition to the man page, or if my distro removed it for some reason.
--left <path> start left panel at <path>
This looks good. I would just recommend some minor changes.
Mar 13 2018
I don't agree that having the QLineEdit pop in just so that one extra bookmark can be displayed before typing is worth it. But either way works fine. Having it be an option as you suggested would be nice, but not really all that important. Especially as I think almost nobody will ever discover that there is an option for it.
Mar 12 2018
Thanks for looking into QWidgetAction, and the code!
Mar 7 2018
Thanks for reviewing my code again.
Mar 4 2018
fixed misplaced else.
I have updated the code. Sorry for the large amount of changes, if requested I can submit the small style fixes and the large code revision separately. I have tried to follow the coding standard this time, hopefully I haven't missed anything.