Closed inv2004 closed 5 months ago
@inv2004 I don't like exposing getKeys
, it complicates the programming model from the client's POV.
The model should be you keep calling getKey
until it returns None.
I should have commented on your previous PR about this, getKeys
should never have been made public.
@johnnovak the change is just addition fix to the prev PR
I suppose we can discuss it in separate issue. I think that that I understand your point, because I thought about some kind of buffer during the prev PR implementation, but I was not sure how to work correct with it in event loop with sleep timeout, an I found that it a a bit overcomplicate compared to seq[Key] because you need the buffer/seq anyway and it is not very obvious that you can do timeout only after Key.None. At least it does not fit the example in readme
-- added -- Lets move the discussion to another place
@johnnovak the change is just addition fix to the prev PR
I suppose we can discuss it in separate issue. I think that that I understand your point, because I thought about some kind of buffer during the prev PR implementation, but I was not sure how to work correct with it in event loop with sleep timeout, an I found that it a a bit overcomplicate compared to seq[Key] because you need the buffer/seq anyway and it is not very obvious that you can do timeout only after Key.None. At least it does not fit the example in readme
-- added -- Lets move the discussion to another place
Well, it's my repo @inv2004, so we can discuss it here, I don't mind at all ๐
The guy in the original issue ticket described how this should work:
If two (or more) keys are pressed between getKey calls, for example pressing "A" and then "B" and then calling getKey, then Key.None is returned. My expectation was that the first call to getKey would return Key.A and the next subsequent call to getKey would return Key.B.
Read the rest of his comment for extra details. Basically, don't overthink it; the client just needs to keep calling getKey
in the event loop, then it will eventually get all the keys. Yeah, with a pause between the keypresses (sleep duration per event loop interation), but who cares. That's OK for terminal apps, we're not writing an FPS game here ๐
The fact that we're internally buffering is 100% an implementation detailโall the client should care about is calling getKey once per the event loop to get all key presses, eventually (or multiple times, really up to them; from the point of view of the library we don't care).
Feel free to make these changes in this PR. So getKeys
should go aways, or become internal only, and we should only have getKey
, as before (but it should work now correctly, without swallowing keypresses).
No reasons to hold the PR (because the getKey
is broken at the moment).
I am not 100% sure that just getKey is better at the moment.
Lets continue here: https://github.com/johnnovak/illwill/issues/48
@johnnovak Please do not forget to lift version again after the merge
@inv2004 This works well on macOS, haven't tested on Windows yet. However, that super messy parseStdin
needs to be optimised for maintainability and readability.
@johnnovak Probably the dance around parseStdin
could be another task if someone (maybe me) would like to invest more time in it, because it is just aesthetic requirement and I agree with it. If we move the aesthetic a bit aside then the change tested, fixes a lot of issues and improve functionality of the lib, not only linux, but win also
For the moment it is up to you to merge or not. I am ok to use the branch from my repo
@johnnovak Probably the dance around
parseStdin
could be another task if someone (maybe me) would like to invest more time in it, because it is just aesthetic requirement and I agree with it. If we move the aesthetic a bit aside then the change tested, fixes a lot of issues and improve functionality of the lib, not only linux, but win alsoFor the moment it is up to you to merge or not. I am ok to use the branch from my repo
Ok @inv2004 can you address my further minor comments and clean up the history? This whole thing should go in a single commit, please squash & rebase off main. Then in it goes... Could be useful for some, and I'm not really maintaining this anyway ๐
I suppose that squash and & merge or rebase is just one github button on your side
I suppose that squash and & merge or rebase is just one github button on your side
Indeed, I can squash & merge. I'm just used to a rebase workflow and that's greyed out because of your merge commits. All good now, thanks ๐
Looks like old
getKey
was broken here: https://github.com/johnnovak/illwill/pull/45