Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make around-next only affect the next button #167

Merged
merged 2 commits into from
Feb 8, 2021
Merged

Make around-next only affect the next button #167

merged 2 commits into from
Feb 8, 2021

Conversation

slotThe
Copy link
Member

@slotThe slotThe commented Jan 2, 2021

Closes #166

I'm not merging this myself as, while it's technically a bug fix, it
possibly introduces a regression; see the discussion in #166 for all
information.

Before this change, around-next was able to change more buttons than just
the very next one, provided the next button was released after some
other buttons were pressed. For example, suppose we had

(defalias
  nsh (around-next sft))

Then the old behaviour would give

T@nsh Ta Tb       ==> Ab
T@nsh Pa Pb Ra Rb ==> AB

while the intended behaviour (and the one that this commit introduces)
would be

T@nsh Ta Tb       ==> Ab
T@nsh Pa Pb Ra Rb ==> Ab

Before this change, aroundNext was able to change more buttons than just
the very next one, provided the next button was released after some
other buttons were pressed.  For example, suppose we had

    (defalias
      nsh (around-next sft))

Then the old behaviour would give

    T@nsh Ta Tb       ==> Ab
    T@nsh Pa Pb Ra Rb ==> AB

while the intended behaviour (and the one that this commit introduces)
would be

    T@nsh Ta Tb       ==> Ab
    T@nsh Pa Pb Ra Rb ==> Ab
@david-janssen
Copy link
Collaborator

Hmmm, I pulled this branch locally and then made some changes and pushed those changes hoping they'd be reflected here, but instead they turned into a branch on the main repo. Not toooo handy with git and GitHub yet. I have 2 options for you:

  • You give me the greenlight and I'll just merge that branch (around-next-167, currently visible as a branch on KMonad) into master. No fuss for you.
  • You pull the changes from that branch into the pull request and then I'll approve it through this avenue. A bit more fiddling, but I think you get credit somewhere on GitHub, and I wouldn't want to rob you of that without checking with you first.

@slotThe
Copy link
Member Author

slotThe commented Feb 6, 2021

Hmmm, I pulled this branch locally and then made some changes and pushed those changes hoping they'd be reflected here, but instead they turned into a branch on the main repo. Not toooo handy with git and GitHub yet.

I once tried this in an xmonad pr: I think what you can do is push directly to my branch on my fork of the repository (i.e. directly to https://github.com/slotThe/kmonad/tree/around-next). I did this with magit, if I recall correctly, but have also since forgotten the exact steps.

* You give me the greenlight and I'll just merge that branch (`around-next-167`, currently visible as a branch on KMonad) into master. No fuss for you.

This is completely fine; as long as the issue is fixed that's enough satisfaction for me, no extra github dopamine needed :)

@david-janssen david-janssen merged commit 2da5d36 into kmonad:master Feb 8, 2021
@david-janssen
Copy link
Collaborator

Oooohh wow, it DID recognize it when I just did the no-github-dopamine version (I couldn't figure out how to push to your copy). Technology, sufficiently advanced, is indistinguishable from magic :-)

@ghost
Copy link

ghost commented Feb 8, 2021

Thank you!

@slotThe
Copy link
Member Author

slotThe commented Feb 8, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Status: Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there a version of tap-next that affects only one subsequent press?
2 participants