Only emulate up/down key presses for mouse scrolls on alternate screen
ClosedPublic

Authored by ahmadsamir on Mar 7 2018, 8:18 PM.

Details

Summary

Konsole sends up/down key press events for mouse scrolls for apps that
aren't interested in mouse events, such as less. Only do this when the
terminal is using the alternate screen.

Now scrolling up/down will be translated to up/down key presses only
when the terminal is using the alternate screen but scrolling in a terminal
using the primary screen will only scroll using the scrollbar, now it does
not cycle through the shell history.
Now the behavior matches xterm and and gnome-terminal.

BUG: 355106
FIXED-IN: 18.04

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
ahmadsamir created this revision.Mar 7 2018, 8:18 PM
Restricted Application added a project: Konsole. · View Herald TranscriptMar 7 2018, 8:18 PM
Restricted Application added a subscriber: Konsole. · View Herald Transcript
ahmadsamir requested review of this revision.Mar 7 2018, 8:18 PM

This fixes the main BR - however, if the profile starts zsh and then you type bash, the old/bad behavior is back. I wonder how other terminals handle that.

ahmadsamir retitled this revision from Only emulate up/down key presses for mouse scrolls for foreground processes to Only emulate up/down key presses for mouse scrolls on alternate screen.Mar 8 2018, 9:21 PM
ahmadsamir edited the summary of this revision. (Show Details)
ahmadsamir updated this revision to Diff 29042.Mar 8 2018, 10:47 PM

Looking at the info in the bug report again and at[1], I've changed the patch to only send the up/down key press events if the terminal is using the alternate screen, which is what less and co. use.

Now the behaviour matches xterm and and gnome-terminal.

[1] http://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h2-Wheel-mice

ahmadsamir edited the summary of this revision. (Show Details)Mar 8 2018, 10:52 PM
ahmadsamir edited the summary of this revision. (Show Details)Mar 8 2018, 10:55 PM

Are you using arc? If not perhaps it will make it easier. I see your name is not attached to the patch.

copy/paste from other ticket:

Can you provide your email address so we can associate your authorship information with this patch?

In the future, if you upload patches using arc, this will happen automatically. See our documentation: https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches

src/TerminalDisplay.cpp
3061

remove return

Is this patch really fixing the BR?

hindenburg added inline comments.Mar 9 2018, 1:53 AM
src/SessionController.cpp
210

I think you could just a lambda and not need the below mthod

210

oh nvm this

What I mean should the BR be changed to WONTFIX? Also, I can't find something easy to test this that shows the difference.

ahmadsamir updated this revision to Diff 29082.Mar 9 2018, 1:27 PM

Remove explicit return statement from void function

ahmadsamir updated this revision to Diff 29083.Mar 9 2018, 1:30 PM

Remove explicit return statement from void function

ahmadsamir marked 3 inline comments as done.Mar 9 2018, 1:39 PM

What I mean should the BR be changed to WONTFIX? Also, I can't find something easy to test this that shows the difference.

No, the bug should be fixed with this patch.

The way I tested with xterm:

  • Run less in xterm, note that mouse wheel scroll events are ignored
  • Launch xterm -xrm XTerm.vt100.alternateScroll:true, now when running less in xterm mouse wheel scrolls are translated to up/down key presses

With konsole:

  • Without the patch, scrolling up/down in a terminal (primary screen) cycles through the shell history; with a program that uses the alternate screen (vim, less ...etc) mouse wheel scrolls also work
  • After applying the patch, scrolling up/down will be translated to up/down key presses only when the terminal is using the alternate screen but scrolling in a terminal using the primary screen will only scroll using the scrollbar, now it doesn't cycle through the shell history

Are you using arc? If not perhaps it will make it easier. I see your name is not attached to the patch.

Using arc diff to submit a new diff is OK, but I find that using it to update a diff isn't that intuitive (git I can try an understand, but arc on top of git is just an extra hoop to jump through for me); I'd rather just persevere through the web UI.

OK thanks for the info - could you try to add your email/name to arc to I don't have to remember to use --author

rkflx added subscribers: ngraham, rkflx.EditedMar 10 2018, 8:07 AM

Are you using arc? If not perhaps it will make it easier. I see your name is not attached to the patch.

Using arc diff to submit a new diff is OK, but I find that using it to update a diff isn't that intuitive (git I can try an understand, but arc on top of git is just an extra hoop to jump through for me); I'd rather just persevere through the web UI.

Sorry to chime in again, but as part of T7116: Streamlined onboarding of new contributors it is incredibly important to not work around (perceived) issues with Arcanist, but to improve our documentation. By all means, please let me know what you miss there, because IMO it really is much easier than using Git and the web uploader:

# setup
git checkout master
arc feature updownkeys

# do initial work
arc diff

# do more work
arc diff

# do more work
arc diff

IOW, instead of git diff, you can just use arc diff. Note that this will still work even if you have multiple local commits, because it diffs against the upstream tracking branch set by arc feature, e.g. master in this case.


@ngraham The Phab wiki page is missing something about how to set the author in Git's config. Do we have a snippet elsewhere which could just be copied? Any help appreciated! ;)

@ngraham Should we change the wiki to my example above, and mention git commit only in a side note? After all, arc diff will prompt if there are unstaged changes, or you could specify arc diff -a.

OK thanks for the info - could you try to add your email/name to arc to I don't have to remember to use --author

I looked at the docs on the kde community wiki and phabricator site and I don't know how to do that; shouldn't using the web ui still work? I am using the same email in git, phabricator.kde.org and when using arc diff (the latter AFAIK).

@rkflx I will try harder next time I submit a diff, and will hopefully note down the steps I used (or used and it still did the wrong thing).

rkflx added a comment.Mar 10 2018, 9:54 AM

@ahmadsamir No problem, we are still smoothing out rough edges…

shouldn't using the web ui still work?

Sadly not. Until T5242 is solved, Phabricator will not take over any authorship information from patches uploaded via the web uploader.

I am using the same email in git, phabricator.kde.org and when using arc diff (the latter AFAIK).

For arc diff, make sure your local Git config is set up correctly:

git config --global user.name <Your Real Name>
git config --global user.email <Your identity.kde.org email>

[....]

shouldn't using the web ui still work?

Sadly not. Until T5242 is solved, Phabricator will not take over any authorship information from patches uploaded via the web uploader.

I am using the same email in git, phabricator.kde.org and when using arc diff (the latter AFAIK).

For arc diff, make sure your local Git config is set up correctly:

git config --global user.name <Your Real Name>
git config --global user.email <Your identity.kde.org email>

git config is set up correctly.

rkflx added a comment.EditedMar 10 2018, 12:22 PM

git config is set up correctly.

In that case, to fix it for this Diff, it is simply a matter of using arc diff again, because the last revision was uploaded via Phabricator and not via Arcanist. I'm not sure what your local setup is, but the following should work in any case:

arc feature updownkeys origin/master
arc patch --nobranch D11146
arc diff

Edit: Made my example handle more edge cases…

ahmadsamir updated this revision to Diff 29155.Mar 10 2018, 1:39 PM

Did it work?

rkflx added a comment.Mar 10 2018, 1:41 PM

Did it work?

Yup, well done 👍 (checked with arc patch).

Did it work?

Yup, well done 👍 (checked with arc patch).

Well done you; I used a variation of the commands you posted (and read/skimmed to docs on them), I didn't know about arc patch.

Thanks.

@ngraham The Phab wiki page is missing something about how to set the author in Git's config. Do we have a snippet elsewhere which could just be copied? Any help appreciated! ;)

Done: https://community.kde.org/Infrastructure/Phabricator#Set_your_git_identity

@ngraham Should we change the wiki to my example above, and mention git commit only in a side note? After all, arc diff will prompt if there are unstaged changes, or you could specify arc diff -a.

Done.

Well it looks I can't update message/diff unless I commandeer this patch - arc isn't making this easier IMHO

You can only update revisions you own. You can 'Commandeer' this revision from the web interface if you want to become the owner.

Huh, I can update the revision's title and summary from the web interface.

I'm sure I could use the web UI - was trying to use arc from command line. It would be nice to not have to keep switching from the web to arc and back

rkflx added a comment.EditedMar 10 2018, 4:50 PM

I'm sure I could use the web UI - was trying to use arc from command line. It would be nice to not have to keep switching from the web to arc and back

I updated the commit message recently in https://phabricator.kde.org/D10529#221590 (hover text: "Via Conduit"). Note that I commandeered the revision manually only afterwards (hover text: "Via Web"). arc diff --edit --verbatim was enough for me.

hindenburg edited the summary of this revision. (Show Details)Mar 10 2018, 5:17 PM
hindenburg edited the summary of this revision. (Show Details)
hindenburg accepted this revision.Mar 10 2018, 5:19 PM

Sorry for all the noise on these last 2 reviews - trying to get a pattern down for handling these patches

This revision is now accepted and ready to land.Mar 10 2018, 5:19 PM
This revision was automatically updated to reflect the committed changes.