-
Notifications
You must be signed in to change notification settings - Fork 71
Firngrod/primary from same half #1393
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
Open
firngrod
wants to merge
6
commits into
UltimateHackingKeyboard:hrm
Choose a base branch
from
firngrod:firngrod/primary_from_same_half
base: hrm
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
44edec9
Added primary role from same half as config variable and as argument …
eb74623
Added some missing documentation
8bab772
Proper argument. Same effect
7cff9b7
Reactions to review pt. 1
7cde843
Reactions pt. 2: Advanced edition
f38fd11
Embarrassing
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
last key in the queue, or the action key keyid? You may get scenarios where there are more keys after the action key, in which case this is a major change to the logic of the entire driver, isn't it?
If you are just trying to get the action key keyid, then I would expect this to do the trick:
Edit: remove the suggestion.
Uh oh!
There was an error while loading. Please reload this page.
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.
PostponerQuery_InfoByQueueIdx(0, &actionPress, &actionRelease);gives me the first key pressed after the secondary-role key under evaluation, not necessarily the one which triggered the evaluation. For trigger on press, that's fine, but it breaks with Trigger on release especially when we care which key triggered the resolution.If I press
Ctrl/O+Alt/Son my right half andDeleteon my left,Ctrl/O,PostponerQuery_InfoByQueueIdx(0, &actionPress, &actionRelease);gives me theAlt/Skey every time theCtrl/Oevaluates, never theDelete, and it doesn't work.Maybe I should pass the action key down all the way from
SecondaryRoles_ResolveState? I'm going to try that.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.
Nope, that was the resolution key.
Uh oh!
There was an error while loading. Please reload this page.
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 elaborate on the above scenario.
What is the exact scenario, what do you expect to happen and why?
So far the way I see it, when you press O+S, with primaryFromTheSameHalf enabled, it should resolve as primary action for O (by definition of primaryFromTheSameHalf), and perform the O primary action. Next, you get S press applied, which triggers a new dual role resolution on S+Delete.
What am I getting wrong?
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.
It's trigger on release and O and S are being held. I didn't make that clear. Neither O or S should trigger before timeout or a key has been released. And since Delete is released first, O should trigger secondary because it was from the other half. That is why I need information on the last postponed key, the Delete key. Realistically, I should loop over the queue to find the first released key. I think I'm going to try that to see if it makes any difference.
Uh oh!
There was an error while loading. Please reload this page.
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 queue lookups are sorted by press times, right?)
So the scenario is:
In what real life scenario are you triggering a secondary role with another intermediate tap in between of the secondary key and the action key?
I mean, I would expect that the S tap is an evidence that you are in the middle of writing and so that the O+whatever would be an undesired O secondary role activation.
Well, you have no guarantee that secondary evaluation takes place on every wake event, so you will indeed have to loop over the queue.
But let's not get ahead of ourselves. I still don't understand the scenario.
Uh oh!
There was an error while loading. Please reload this page.
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.
My conceptual idea of trigger by release is that when an alt role key is being held, the release of ANY key pressed after it should trigger resolution, not just the next key. This allows the very important possibility of combining alt role keys in a multi-mod combo, like the above mentioned
Ctrl+Alt+Del. Press O, press N, press Delete, release Delete, release O and N any order.On release of Del, I want O to trigger Secondary for Ctrl because of that Delete release trigger, then N to trigger secondary to Alt because of that Delete trigger, then Delete with those mods, then the mods release.
What happens in the current main branch logic is that once I release the combos, I either release O or N first. If I release N first, O triggers Ctrl because of N release, then N triggers Alt because of the previous Delete release, then Delete happens and mods are released in the proper order. If I release O first, first O triggers primary because N is still held, then N triggers secondary because Delete was previously released, and then things unroll from there. This is largely mitigated by the Trigger safety margin, and generally it works as I want it to anyway, and it's all acceptable.
However, that logic will never work properly once we start caring which key caused the resolution to happen, in this case whether N or Delete caused the O resolution by releasing first. Without looking through the queue for the first released button, multi-mod combos can only be made by utilizing the timeout to trigger the first mod as secondary.
In general, taking the last key seems to work fine, but I do sometimes see what I feel are false negatives, although I cannot be sure. It may be that I press a secondary Shift, then press the one I want to Capitalize, then press the next key immediately before I release the to-be-capitalized key. I want to see if that is the case.
Uh oh!
There was an error while loading. Please reload this page.
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, I didn't realize this was a multi-mod scenario.
I see now the reasoning and fully agree.
Correct. That's why you can't blindly take the last key.
With a proper implementation things still should work if you press a
postponeKeys delayUntilReleasemacro, then tap your secondary scenario, then tap another key or two, then release the postponeKeys macro.So to sum up, my conclusion is that:
For release triggers, you need to take the first-by-time released key in the queue, that also has the corresponding press in the queue. (This should be implemented in the postponer as a new function that does all the work of finding such a key.)
For the press trigger, you still need to take the first-by-order pressed key in the queue, as there may be no released key in the queue at all. (I.e., according to the old algorithm.)
(Ofc., let me know if there is anything suspicious in my reasoning whatsoever.)
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.
Sounds good. I will get on that when I have the time. Hopefully tonight or tomorrow night.