Closed felixfung closed 1 year ago
@dreamcat4 please review
@felixfung it was necessary for me to go back and review my original commits. (of 5 years previous). when those were introduced...
for remembering properly what this keyboard code is about: which is still remains all around complex matters... which is utterly not time efficient to explain fully. to walk through so many usecase testing scenarios (which is part forgotten, was never documented).
now for this matter:
reversedirection
totab
means that reversing the direction (with the user configured modifier keys)... only applies to the tab
key (successive press)ok so with that established the corner use case is such:
ok so you can take that situation, and it applies more generally as a user optimization strategy. in order to let the user acheive a maximum effeciency to press the minimum number of keypress on the keyboard.
why it matters?
it's significantly important if you are a user who:
final 2 points:
one of the main motiviating reasons for why i picked up skippy (to begin with). was in the knowledge that the most frequently used function of the computer is repeated over and over. perhaps a million times (or more) in the short human lifetime. so saving 1-2 keystrokes is multiplied so much to then add up. and return a significant mathematical result. for hours saved. that made these inclusions deemed 'worth it'.
(however admittedly skippy is tied to xorg platform, and cannot outlast so many years, to switch everybody to wayland, but i was not thinking for those considerations of 5 years previous, when doing these coding)...
anyhow. back to the story:
in original versions of skippy code 100% of this logic... whilst admittedly somewhat convoluted and difficult to understand. it all worked 100% and with 0 bugs. under all of the tested and real world user usecase. and sensible usage.... it was indeed thoroughly tested (by myself). and with the assumption that it would never need to be returned back to ever again. on what was at the time, arguably quite some dead / defunkt project...
however now since your many commits and changes, then maybe? might have broke somehow certain parts of these keyboard handling? --> i was recently forced to looked again, and maybe have now partial fixed (it may be incorrectly, my recent single commit) on those logic with ||
on the conditional... is in fact also wrong.
ok so then wrong of me. at least in terms of attempting to be restoring the original functionality here. and so on.
therefore your desire to rip out something around keybinding feature again, which was not clearly documented. but which also you clearly have not good enough understanding of (again?)...
but i hope you had a good example and humor / laughing over it. but seriously speaking i do have long term rsi. to please be a bit more considerate here (around this specific areas of the code).
but i hope we can agree that:
1) ripping stuff out in a pr/mr simply because we do not use it or understand it is... perhaps lead to unforseen breakage or regressions in a pre-existing functionality? that somebody else already (5 years ago) thoroughly tested. is perhaps not best use of the collective project time?
although it did finally remind me. so it's not a complete waste of both our time:
but you could have instead asked in an issue and reference the original commit? --> because reviewing original commit was finally what got me to remember better / more fully. those previous work (of 5 years before).
2) that of all of the current open items, maybe instead the multi-monitor support is probably the most valued, amongst other general user base? (for me to work on, than to keep on explaining those difficult use cases around the keyboard handling).
3) that sending me such to review and explain why (that i would prefer to deny it's merge) isn't going to help me to work on higher priority, other item (2) above
then i think we can return to saying:
well this still best left for my area (for now...) if you don't mind too much (for now). but it is not considered to be 'high enough' matter, for the overall priorities list to actually bother with. until i can complete those other more needed fixings for multi-monitor. and just participate in more enjoyable ways. that isn't as much a forced-drudgery.
and then come back, to restore this originally (and retest it as per original ways). then to move forwards from that point. if after that any real removal is in fact actually still required.
for example previously you made such claims as (for example with reversemodifierkeysdirection):
that those code is never executed... (or similar level claim). however i didn't actually understand how is that possible, because the only way it could be the truth is if (you yourself) had already broken this pre-existing feature. (perhaps not intentionally, simply by mistake).
and then determined that such broken state was then further grounds for complete removal... having never properly understood it's actual or original function? - which is somewhat understandable! ... since it is indeed convoluted code / difficult feature.
anyhow my decision (for this / today's mr) is simply do not merge this. until a later time... that i can be sure to first restore and re-test the original program behaviour. (which is a low priority task, on myself). and then go from there. rather than to keep on arguing about these keyboard matters... until i can myself come back. to return to it on a voluntary basis. and let you know it's restored (to the prior behaviour). and then it makes a better place to reconsider these keyboard matters.
... but what is even more funny about this:
Is that I myself was thinking exact same things as you were here. With very confused face. As to why is put there a TAB
key. And very angry about, thinking: its clearly an error!. (Or maybe that you do not recognize, that somebody else came and added after you. but setting value is mistake). Makes not any sense! but...
Then you look in the commit history. And find out well actually.... ah goddamnt it. There is indeed some valid reasons for why we need this thing(s). But they were entirely undocumented.
I believe my thinking at the time was: well, the work to document all of the use cases fully. (to know fully the regression test case). Is maybe about 3 times the work, than the code itself!
So i have some last-resort to: make the variable names something complex enough to remind what the variable unambiguously does. (not to hide those reasons behind some simpler names as such...)
And well: if i don't document a bunch of difficult regression test cases. Then it's quite the matter to determine by the code alone! (that is 3x less work to read it back, to remind). But clearly there most likely are also other corner case regression test cases... that i might forget! And so on. It's not like i wanted to be proud of this code, given such necessary level of complexities added. I didn't want to actually be so worse from the code perspective. But the overall decision was to balance and prioritize the actual user use case functionality. And lets say... "robustness level" to always work no matter the specific situation. That those logic just holds up against whatever any user could potentially try to throw at it.
But (if you have any form of a sense of humor, at all):
if that's not some sort-of an unexpected "code trolling".... then it surely must instead be a **reverse**
code trolling.. . since because the functionality is for to reverse
the keys direction (or not, as per the user wishes 😀)
And that's all i'm going to say about it. Since all the humor is exhausted now, it is too much assumed to make fun out of such things.
for this other configuration key here, this is to specify which subset of the direction key(s) skippy is applying that reversedirection to
Is that I myself was thinking exact same things as you were here. With very confused face. As to why is put there a TAB key
So 1 final comment here;
However from a code perspective, it's just a lot easier to delay such (purely renaming) improvement. Until after my own retest, refixings on master. Around these feature set. And thorough regression / remind. For how it all indeed affects those variety of testing use cases...
So it's better aproach, just to leave open some issue, to discuss rename these config key names. (Until happy about it). And then can be applied after my own return back to those code / rework / refixings.
For the sake of letting me remember better the original code (as i had coded it, from my memory). That the code is more easily recognized (by myself).
But indeed - no issue to future approve (or prior approve now) a future config name change to be due to improve. That (at minimum) makes a good sense here. Since was indeed such high degree of confusion around the config actual purpose / key names.
This (again), the doing is then considered as some lower priority task. After the other task(s). But is to remain something valued, and an important matter to fix (just not now, for later on).
Compared to modifierKeyMasksReverseDirection, which is a holding button (e.g. Shift/Ctrl) to reverse window selection direction, keysReverseDirection is a strange concept.
The name suggests striking a key, not holding a key, to change direction.
For one, the implementation actually is holding a key.
Second, there is no storing of this directional change, so that it is not a key stroke to toggle direction; who would want that on Alt-Tab anyway?
Third, the current default key for keysReverseDirection is
Tab
. That would mean if a user sets upkeysNext = Tab
, and forgets to removemodifierKeyMasksReverseDirection = Tab
, then hittingTab
key goes in reverse direction. I have verified that with testing.Unless, the user configures hot key similar to this:
Then the hot key (be it from window manger or xbindkeys or similar) would intercept the Tab strokes, and neither keysReverseDirection nor modifierKeyMasksReverseDirection key events would ever register, and they are effectively removed from the code.
In this PR, we remove keysReverseDirection. While the real world use case for modifierKeyMasksReverseDirection is limited, it is functional and let's keep it for now.