Fix browse mode not disabled after Ctrl is released
Needs RevisionPublic

Authored by simgunz on May 12 2019, 2:07 PM.

Details

Reviewers
rjvbb
mwolff
Group Reviewers
KDevelop
Summary

FIxed by removing the Ctrl modifier. Alt modifier achieves the same effect.

TODO:

  • Document the feature

BUG: 402760

Diff Detail

Repository
R32 KDevelop
Branch
fix-browse-by-key
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11851
Build 11869: arc lint + arc unit
simgunz created this revision.May 12 2019, 2:07 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptMay 12 2019, 2:07 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
simgunz requested review of this revision.May 12 2019, 2:07 PM
simgunz retitled this revision from Fix browse mode not disabled after Ctrl is released BUG: 402760 to Fix browse mode not disabled after Ctrl is released.May 12 2019, 2:09 PM
simgunz edited the summary of this revision. (Show Details)
simgunz added a reviewer: KDevelop.
simgunz updated this revision to Diff 58120.May 15 2019, 11:25 AM
  • Disable Ctrl and keep only Alt
simgunz edited the summary of this revision. (Show Details)May 15 2019, 11:26 AM
simgunz added a subscriber: rjvbb.
rjvbb added a comment.EditedMay 15 2019, 11:55 AM

LGTM.

You identified an event chain which leads to the browsing mode not being restored correctly. This should happen less often with the Alt modifier but it can still happen (hit Alt to display a tooltip or open a menu and then move the mouse over that tooltip or select a menu item?).

In short, it'd be good to look into fixing the browse mode restore while you're at it; if I understood correctly you already identified the proper or additional event to use for that?

With the Alt key the bug does not appear, because the existing code disables correctly the browse mode when Alt is released. For some reasons key press and release events are different for Ctrl key but are the same for Alt. Probably the method avoidMenuAltFocus manages this behavior correctly for the Alt key (I have not analyzed in detail what it does) and then we enter correctly in the if at line 230 where source browse mode is disabled. So removing the redundant Ctrl key solves the bug completely (at least I haven't manage to reproduce it).

rjvbb accepted this revision.May 15 2019, 3:05 PM

Sounds good enough for me then!

This revision is now accepted and ready to land.May 15 2019, 3:05 PM
mwolff requested changes to this revision.May 15 2019, 9:25 PM
mwolff added a subscriber: mwolff.

you are removing a feature, but only partially - a lot of code would become superfluous by this change and should be cleaned up accordingly

that said, how do you trigger the bug? for me ctrl + click works as intended, and I want to keep that functionality. please describe how to trigger the bug, then we can think about fixing it properly instead of removing the feature.

This revision now requires changes to proceed.May 15 2019, 9:25 PM
rjvbb added a comment.EditedMay 16 2019, 12:32 AM
you are removing a feature

What feature? This just removes one of the two keys with which one can trigger the feature.

There's an analysis of the bug in the linked bug report on BKO. I keep getting bitten by it (sometimes it really slows me down) but I've never been able to figure out how I get it. According to the analysis it might be as simple as moving the mouse after using the Ctrl key in shortcut (^s or ^c), and that's something I can definitely see myself doing.

EDIT: if by feature you mean "use Ctrl-Click to XXXX" then there's an additional flaw; on Mac you'd have to use Command-Click (or use conditional code). And if you use conditional code you'll run into a long-standing system shortcut for opening the context menu (i.e. the one-button mouse right-click).

you are removing a feature, but only partially - a lot of code would become superfluous by this change and should be cleaned up accordingly

As rjvbb said, no feature have been removed. The feature is still there through the Alt key (as it was before), while not accessible anymore with the Ctrl key. There is no reason to have the same (undocumented) feature on two different keys. Moreover there is no code to cleanup, what is there is needed to have the feature working on the Alt key (at least for my understanding of the code, I have not noticed unuseful blocks).

that said, how do you trigger the bug? for me ctrl + click works as intended, and I want to keep that functionality. please describe how to trigger the bug, then we can think about fixing it properly instead of removing the feature.

The bug appears when the python plugin is installed and active.
Steps to reproduce:

  • Select the editor
  • Click and release Ctrl
  • The cursor becomes and remains a hand

Beside the python plugin, I believe that also without it (when everything is working correctly) the code was wrong. See comment 15 on the bug report for a detailed explanation.

you are removing a feature, but only partially - a lot of code would become superfluous by this change and should be cleaned up accordingly

that said, how do you trigger the bug? for me ctrl + click works as intended, and I want to keep that functionality. please describe how to trigger the bug, then we can think about fixing it properly instead of removing the feature.