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

Improve interval check method #2

Closed
wants to merge 1 commit into from

Conversation

conao3
Copy link

@conao3 conao3 commented Sep 13, 2020

Those commit are commit in @zk-phi's fork (repository)

Accoding his blog, this idea is like below.

So what I thought was to reduce the number of false inputs by
invoking when "one tempo delay from other keystrokes", instead of
just checking for "a short enough interval between two keystrokes".

before

 \ カタカタ /      \ バン /     \ カタカタ /
h o g e h o g e      hj       h o g e h o g e . . .
----------------------------------------------------
                  |< 0.1s|

after

 \ カタカタ /      \ バン /     \ カタカタ /
h o g e h o g e      hj       h o g e h o g e . . .
----------------------------------------------------
                 |< 0.15s|
              |> 0.1s||> 0.25s|

@irigone
Copy link

irigone commented Dec 17, 2020

Why don't they accept the patch? Also, is the package unmaintained anymore?

@conao3
Copy link
Author

conao3 commented Dec 17, 2020

What do you think about this PR, @tarsius? And are there any potential maintainers?

@tarsius
Copy link
Member

tarsius commented Dec 18, 2020

What do you think about this PR

Please put some more work into it before asking me to review. It appears that you are making some tweaks and then tweak the same things in the next commit or so. It is not at all obvious to me why you are doing that. Maybe there are reasons for doing that; if so, then please document them.

key-chord.el Outdated
@@ -183,6 +172,8 @@

;; ######## History ########################################
;;
;; 0.7 (2017-06-29) zk-phi
;; Add option `key-chord-safety-interval'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes little sense to release in the middle of a patch series. Also you should add the note about this new option in the commit that adds this new option. You can add a new history entry without releasing at the same time. Just use something like Unreleased instead of 0.7 (2017-06-29) zk-phi.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I don't make a bump version in this PR.

key-chord.el Outdated
"Max time delay between two press of the same key to be considered a key chord.
This should normally be a little longer than `key-chord-two-keys-delay'.")

(defvar key-chord-safety-interval 0.08
(defvar key-chord-safety-interval 0.15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain why you are making these changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there is a typo in the commit message.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why he makes this change but this commit is disappeared after fixup commits.

key-chord.el Outdated
@@ -173,7 +173,7 @@
;; ######## History ########################################
;;
;; 0.7 (2017-06-29) zk-phi
;; Add option `key-chord-safety-interval'
;; Add option `key-chord-safety-interval-backward/forward'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are adding two options and neither one is named key-chord-safety-interval-backward/forward.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't make a bump version, so this History entry was the whole disapear.

@conao3
Copy link
Author

conao3 commented Dec 18, 2020

Fix commit history and force pushed.

This PR was originally a separate fork by zk-phi, which he continued to develop as a fork, not to create a such PR.
It was my mistake to make it into a PR as it is. I apologize for the hassle and thanks for the good review.

@tarsius
Copy link
Member

tarsius commented Dec 22, 2020

I actually wanted to try this package for a long time and now I finally have.

I also cleaned it. Please rebase this pr on top of that cleanup. Sorry for the additional work this causes.

This PR was originally a separate fork by zk-phi, which he continued to develop as a fork, not to create a such PR.
It was my mistake to make it into a PR as it is. I apologize for the hassle and thanks for the good review.

No problem. I didn't realize what was going on.

So I assume you tried this for a while. In your experience, how (much) doee it improve the user experience? I don't have enough experience using this package even without this change, so I intend to test both approaches for a while before making a decision.

@conao3
Copy link
Author

conao3 commented Dec 23, 2020

rebase done.

To be honest, I'm not an enthusiastic fan of this package, but I thought zk-phi's suggestion made sense, so I suggested this PR.
(When he wrote the blog, upstream was already inactive and he was wondering where to send the patch)

It is reasonable to assume that the "key-chord decision" will be a little slower than the surrounding input speed, and I feel that this change has reduced the number of unintended command executions.

@tarsius
Copy link
Member

tarsius commented May 22, 2023

Finally merged.

@tarsius tarsius closed this May 22, 2023
This was referenced May 22, 2023
@conao3
Copy link
Author

conao3 commented May 22, 2023

thanks!

@BlueDrink9
Copy link

BlueDrink9 commented Jan 8, 2024

@tarsius would you consider reverting this merge?

New issues created by merge

This has introduced #6, #7 and #8. ghost-of-freedom makes what I think is a good point, albeit brusquely:
Neither author nor merger uses this package much from what we can read here (correct me if this is wrong), whereas at least 11 people that do use it have been significantly enough disrupted by this PR to come here and mention it.

No old issues solved by merge

This repo had sat for 4 years without accruing many issue reports. Two issues are referenced as being fixed by this PR, of which neither are actually solved by it. #5 had an existing correct solution found by the author (but they never closed the issue). #4 seems unrelated, presumably closed by mistake.

tarsius added a commit to emacsmirror/key-chord that referenced this pull request Jan 9, 2024
This reverts commit e724def.

This (emacsorphanage#2) should not have been merged.  Maybe as an opt-in, but the
default, which worked for many people for years, should have stayed
the same.

Closes emacsorphanage#6, emacsorphanage#7 and emacsorphanage#8.
@tarsius
Copy link
Member

tarsius commented Jan 9, 2024

I agree. Sorry about that.

@tarsius
Copy link
Member

tarsius commented Mar 19, 2025

A similar change has been proposed in #12. Please help us test and review that, because otherwise my only options are to simply reject or to merge and risk a repeat of what happened in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants