-
Notifications
You must be signed in to change notification settings - Fork 71
Firngrod/passive timeout #1436
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
Firngrod/passive timeout #1436
Conversation
| ``` | ||
| set secondaryRole.defaultStrategy advanced | ||
| set secondaryRole.advanced.timeout 500 | ||
| set secondaryRole.advanced.timeout 200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text below states 200 ms. On second thought, I should update the text instead. 200 ms timeout is aggressive.
| if (actionEvent != NULL) { | ||
| RESOLVED(SecondaryRoleState_Secondary); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was so anticlimactic to write once I got here.
| } | ||
|
|
||
| // otherwise, keep postponing until key action | ||
| return SecondaryRoleState_DontKnowYet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user can fill the queue while holding indefinitely. They will loose keystrokes. I'm okay with that.
|
Btw., a bit of commit message philosophy, since I am doing git archeology right now: it would be nice if I could tell straight away what part of the codebase a commit belongs to just from the message. In the case of your PRs, it might mean just prefixing messages with a short codeword like "hrm". E.g., "hrm: Embarrassing". This is just a general feedback. No need to rebase anything ;-). |
|
(I am guilty too though: "pick c6198af # Reword some code.".) |
|
Makes sense. Where I work, we do the related story prefix, so I always do a double take anyway while I try to remember what the story number was. Might as well harness that. :) |
|
This branch now also addresses #1441 |
|
I have run this for a week with no issues now. |
doc-dev/reference-manual.md
Outdated
| - advanced strategy may trigger secondary role depending on timeout, or depending on key release order. | ||
| - `set secondaryRole.advanced.timeout <timeout in ms, 350 (INT)>` if this timeout is reached, `timeoutAction` (secondary by default) role is activated. | ||
| - `set secondaryRole.advanced.timeoutAction { primary | secondary | none}` defines whether the primary action or the secondary role, or no action at all, should be activated when timeout is reached | ||
| - `set secondaryRole.advanced.timeoutType { active | passive }` defines whether the secondary role should perform it's timeout action when the timeout is reached or when the key is released after having been pressed beyond timeout without triggering secondary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please extend this. I can't wrap my head around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current behavior, which I have called active will perform the timeout action immediately on timeout. The new behavior I have added, called passive will perform the timeout action once the key is released, if the key has timed out. The key difference is that with passive timeout, it can still become secondary after timeout until the key is released. So now, with passive timeout action none, I can hold the key for a long time and still combo the secondary role, or release without activating start or context menus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to other ways of doing this, but this way also allows effectively disabling timeout by setting passive timeout primary action, which is nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, the key difference is that it cannot become primary but can become secondary.
It is fine, just the documentation needs to be polished a bit.
Suggested wording:
- `set secondaryRole.advanced.timeoutType { active | passive }`
- active - the timeout action is activated immediately upon timeout.
- passive - the timeout action is delayed until release. This way, after the timeout, the action cannot become primary anymore, but can still become secondary.
I am wondering if this should be configurable at all, or the only allowed way. But it is definitely fine like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this phrasing:
set secondaryRole.advanced.timeoutType { active | passive } defines how the timeout action is applied. With active timeout, the timeout action is applied the moment the timeout has been reached. With passive timeout, if the key fails to become secondary and was held longer than the timeout time, when it's released, the key will activate the timeout action rather than it's primary function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, reply got stuck with the rest of the review batch.
Will submit the review now and follow up later with more.
doc-dev/reference-manual.md
Outdated
| - advanced strategy may trigger secondary role depending on timeout, or depending on key release order. | ||
| - `set secondaryRole.advanced.timeout <timeout in ms, 350 (INT)>` if this timeout is reached, `timeoutAction` (secondary by default) role is activated. | ||
| - `set secondaryRole.advanced.timeoutAction { primary | secondary | none}` defines whether the primary action or the secondary role, or no action at all, should be activated when timeout is reached | ||
| - `set secondaryRole.advanced.timeoutType { active | passive }` defines whether the secondary role should perform it's timeout action when the timeout is reached or when the key is released after having been pressed beyond timeout without triggering secondary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, the key difference is that it cannot become primary but can become secondary.
It is fine, just the documentation needs to be polished a bit.
Suggested wording:
- `set secondaryRole.advanced.timeoutType { active | passive }`
- active - the timeout action is activated immediately upon timeout.
- passive - the timeout action is delayed until release. This way, after the timeout, the action cannot become primary anymore, but can still become secondary.
I am wondering if this should be configurable at all, or the only allowed way. But it is definitely fine like this.
doc-dev/reference-manual.md
Outdated
| - advanced strategy may trigger secondary role depending on timeout, or depending on key release order. | ||
| - `set secondaryRole.advanced.timeout <timeout in ms, 350 (INT)>` if this timeout is reached, `timeoutAction` (secondary by default) role is activated. | ||
| - `set secondaryRole.advanced.timeoutAction { primary | secondary | none}` defines whether the primary action or the secondary role, or no action at all, should be activated when timeout is reached | ||
| - `set secondaryRole.advanced.timeoutType { active | passive }` defines whether the secondary role should perform it's timeout action when the timeout is reached or when the key is released after having been pressed beyond timeout without triggering secondary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, reply got stuck with the rest of the review batch.
Will submit the review now and follow up later with more.
kareltucek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactor!
Just please rephrase those docs.
|
Also please write a changelog. (List the changes, including all fixes.) |
|
I'm traveling for work until Wednesday evening and didn't bring my own computer, so I will address things once I get home. |
The active timeout should definitely still be an option.
Where do I write that? Just as a comment in this PR? |
Ok!
Ideally edit the leading comment of the PR please. |
I added a new feature to do timeout passively, as I mentioned on the forum, although I had not fully realized yet that it could be implemented this simply. Simply put, instead of applying the timeout action when timeout is reached, it will apply the timeout action once the key is released. If timeout action is primary or secondary, that will be a tap of the key, if timeout action is none, it will do nothing, The big difference here is that the key can be held for an arbitrarily long amount of time and still be used in a combo.
The main motivator is that I often press a secondary key to do a combo, then realize that it's on the same half. Now I can just wait for half a second and release without my screen drowning in menus and without having to time my combos tightly.
It also allows the timeout to be effectively disabled by setting passive timeout to primary.
One downside is that now we have some setting combinations which make little sense in practice. One that I can think of is passive timeout to secondary layer switch. It would effectively be identical to passive timeout to none. Maybe that could be added as a feature if someone wants it.
Also rewrote the main resolver to a fail fast style which removes a lot of ambiguity of where options apply.
EDIT: Oops, I used a word I didn't understand. It's not Fail Fast, it's just that I try to quit the function using as little info as possible. What do I know of programming terms, I never went to programming school.
This PR fixes everything mentioned in #1421 and #1441
Changes:
Secondary Role keys (HRM):
Added option to postpone timeout action until release
Made Trigger by mouse subject to minimum hold time to prevent vibration on key press from triggering secondary role when using a trackball module
Made Trigger by mouse reliably trigger secondary role immediately
Fixed a number of edge cases to do with safety margin where secondary role keys would not trigger in a timely manner