Fix browse mode not disabled after Ctrl is released
ClosedPublic

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 11763
Build 11781: 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.

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).

Removing the Ctrl key handling is the feature I mean you have removed.

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.

That explanation is quite thorough, please fix the code then instead of removing the Ctrl key handling. Most notably, it sounds like all we need to do is special-case the release with something like key == 0 && modifier == modifierForKey(m_browsingBykey)

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.

simgunz updated this revision to Diff 60096.Jun 20 2019, 7:50 AM

I reverted the commit were the Ctrl feature was disabled, going back to the first fix I proposed

  • Revert "Disable Ctrl and keep only Alt"
simgunz updated this revision to Diff 60099.Jun 20 2019, 8:06 AM
  • Document feature
Restricted Application added a project: Documentation. · View Herald TranscriptJun 20 2019, 8:06 AM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript
mwolff accepted this revision.Jun 25 2019, 11:22 AM

lgtm, thanks - do you have commit rights? if so, please push to the 5.3 branch

This revision is now accepted and ready to land.Jun 25 2019, 11:22 AM

I have pushed to 5.3 and then merged to master manually, as specified in the Phabricator page guide. How do I close the revision now?

aacid closed this revision.Jun 25 2019, 9:57 PM
aacid added a subscriber: aacid.

If you're not using arc you should have included the "Differential Revision" line in the commit so it would close automatically as stated in https://community.kde.org/Policies/Commit_Policy#Special_keywords_in_GIT_and_SVN_log_messages

I'll close this manually for you :)

If you're not using arc you should have included the "Differential Revision" line in the commit so it would close automatically as stated in https://community.kde.org/Policies/Commit_Policy#Special_keywords_in_GIT_and_SVN_log_messages

I'll close this manually for you :)

In the phabricator guide it is suggested to push manually when the target branch in not master:

https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

Wouldn't make more sense to do arc land --onto 5.3 (and then merge 5.3 into master)? In this way the Differential Revision would be closed automatically.

Final question: for discussing things like this, regarding the https://community.kde.org guides, what is the most appropriate place? Opening a discussion on the Wiki page? Or is it therea bugs.kde.org section for this (I could not find an obvious one).

kfunk added a subscriber: kfunk.Jun 26 2019, 8:33 AM

If you're not using arc you should have included the "Differential Revision" line in the commit so it would close automatically as stated in https://community.kde.org/Policies/Commit_Policy#Special_keywords_in_GIT_and_SVN_log_messages

I'll close this manually for you :)

In the phabricator guide it is suggested to push manually when the target branch in not master:

https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

Wouldn't make more sense to do arc land --onto 5.3 (and then merge 5.3 into master)? In this way the Differential Revision would be closed automatically.

I suggest to do a arc amend (to basically update the commit message with current reviewers, "Differentiatl Revision" line, etc.) and then git push your change manually to the right branch. Let's you use your normal git command-line to actually push changes, which to me is a much more thrust-worthy approach than to rely on arc to do that for me...

Final question: for discussing things like this, regarding the https://community.kde.org guides, what is the most appropriate place? Opening a discussion on the Wiki page? Or is it therea bugs.kde.org section for this (I could not find an obvious one).

kde-community@kde.org probably.

In the phabricator guide it is suggested to push manually when the target branch in not master:

https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

That seems to have been edited by people who have not fully got what arc does ("By default, arc will land the patch onto whatever branch was the parent." seems a bit wrong to me, arc does not remember any here called parent branch, it just knows about a default branch to land to, by the config key 'arc.land.onto.default').

Wouldn't make more sense to do arc land --onto 5.3 (and then merge 5.3 into master)? In this way the Differential Revision would be closed automatically.

IMHO yes, it would. I use it all the time :) Never failed to me. Only failed myself when forgetting to use --onto :)

Wouldn't make more sense to do arc land --onto 5.3 (and then merge 5.3 into master)? In this way the Differential Revision would be closed automatically.

I suggest to do a arc amend (to basically update the commit message with current reviewers, "Differentiatl Revision" line, etc.) and then git push your change manually to the right branch. Let's you use your normal git command-line to actually push changes, which to me is a much more thrust-worthy approach than to rely on arc to do that for me...

Probably this should be added to the guide. And probably it should also be added that the commits should be squashed (thing that arc land does automatically).

Wouldn't make more sense to do arc land --onto 5.3 (and then merge 5.3 into master)? In this way the Differential Revision would be closed automatically.

IMHO yes, it would. I use it all the time :) Never failed to me. Only failed myself when forgetting to use --onto :)

Good to know. I prefer this approach, seems way easier and less error prone than cherry pick, squash, arc amend and push.

Huh? It's easier than that:

  • If you do not have the commit yet locally:
arc patch --nobranch <ID>
git push
  • If you do have the commit and it is the latest one in your history:
arc amend
git push

Obviously you should be on the correct branch before doing so.

Anyhow, I guess my point is clear: I do not like the arc land magic :)

> I suggest to do a `arc amend` (to basically update the commit message with current reviewers, "Differentiatl Revision" line, etc.) and then `git push` your change manually to the right branch. Let's you use your normal git command-line to actually push changes, which to me is a much more thrust-worthy approach than to rely on arc to do that for me...

Probably this should be added to the guide. And probably it should also be added that the commits should be squashed (thing that `arc land` does automatically).

The problem with that is that you already pushed, so you'd have to force-push, and everyone who has already pulled your original commit will have to force-pull. A git amend will in fact rewrite the history and change the commit hash (but the original hash continues to exist). Unless I'm doing something wrong, which isn't impossible at all.

Good to know. I prefer this approach, seems way easier and less error prone than cherry pick, squash, arc amend and push.

I only use arc to upload to phab, and sometimes to fetch a patch file. For the rest I prefer to do things manually. Git is voodoo enough on its own, no need to add additional complexity written in some obscure-to-me scripting language :)

  • If you do have the commit and it is the latest one in your history: ` arc amend git push `

Sorry but it is still not clear if this would work. In this diff I have three commits (modification_1, modification_2, revert modifications_2) in a feature branch forked from master. I need to push to 5.3. If I run those two commands I expect that three commits are pushed (not a single one with the total modification) and I would expect that those are pushed to origin/my-feature-branch not in 5.3. Isn't it?

What I would do is: rebase my-feature-branch on top of 5.3, squash the three commits in one, arc amend to updated the commit message, and finally git push.

The problem with that is that you already pushed, so you'd have to force-push, and everyone who has already pulled your original commit will have to force-pull. A git amend will in fact rewrite the history and change the commit hash (but the original hash continues to exist). Unless I'm doing something wrong, which isn't impossible at all.

I think that he meant to do arc amend before anything was pushed, just to pull the commit text from the Phabricator review and add it to the latest commit, before pushing.

I only use arc to upload to phab, and sometimes to fetch a patch file. For the rest I prefer to do things manually. Git is voodoo enough on its own, no need to add additional complexity written in some obscure-to-me scripting language :)

I agree that arc is another layer of complexity, thus all my questions. :-)

Huh? It's easier than that:

  • If you do not have the commit yet locally:

    ` arc patch --nobranch <ID> git push `

Or probably a simpler approach is to run these two commands irregarding the fact that I have the commits locally or not. So I just pull the 'clean' modification with the correct commit message and then push to a target branch. E.g: git push origin 5.3

Oh, I usually squash my changes into the original commit before uploading the change (i.e. via arc diff). I do not use an extra branch either.

Phew. Anyhow. Let me sum this all up with: Please make your life easier and rather start using KDE's GitLab instance: It's much easier to use since it shares the same workflow as all other major Git hosting platforms out there: https://invent.kde.org/kde/kdevelop

Phabricator is just horrible to adapt to and integrates badly with the usual Git workflow. For KDE development, Phabricator will likely be displaced by GitLab anyway in near future.

Oh, I usually squash my changes into the original commit before uploading the change (i.e. via arc diff). I do not use an extra branch either.

Phew. Anyhow. Let me sum this all up with: Please make your life easier and rather start using KDE's GitLab instance: It's much easier to use since it shares the same workflow as all other major Git hosting platforms out there: https://invent.kde.org/kde/kdevelop

Phabricator is just horrible to adapt to and integrates badly with the usual Git workflow. For KDE development, Phabricator will likely be displaced by GitLab anyway in near future.

Indeed Gitlab should simplify things a lot. Thanks again.

rjvbb added a comment.Jun 26 2019, 4:28 PM
Indeed Gitlab should simplify things a lot.

Not for everyone it won't; not if your workflow doesn't conform to the one that's the most common currently. And no, that doesn't necessarily mean it's "less good" :)
(Well, I guess that contributing less is a form of simplification too... :-/)