-
-
Notifications
You must be signed in to change notification settings - Fork 491
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
Adding option to exit OTP progress bar #2041
Conversation
5eaeb11
to
5d665d8
Compare
5d665d8
to
0f7db70
Compare
Signed-off-by: Yolan Romailler <yolan@romailler.ch>
0f7db70
to
63df0f1
Compare
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 wonder why we need the go-tty lib. Also I think gocycle rightfully complains. If you need to keep the nesting at least move the implementation of these goroutines to explicit functions.
Yeah, this is still a work in progress since the Linux CI isn't passing, but I was trying to code on Windows for once, which made my life a little bit more complicated. And didn't have time today to go back to it yet. Excellent idea @dominikschulz about putting the Done in a defer. For the go-tty, we need it in order to read a keystroke on all platforms, without requiring to press Enter after entering our input... |
@AnomalRoil Hmm, I thought I had replied to this one. But pressing enter after q sounds very un-intuitive to me. So let's go with go-tty instead. |
RELEASE_NOTES=n/a Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
RELEASE_NOTES=n/a Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
028e1e4
to
7206681
Compare
RELEASE_NOTES=n/a Signed-off-by: Yolan Romailler <yolan@romailler.ch>
At long last! I shouldn't try and dev when I cannot run the full test suite and codequality checks locally xD |
return // returning not to leak the goroutine | ||
default: | ||
} | ||
r, err := tty.ReadRune() |
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.
Actually I wonder if this is not waiting until the next keystroke, and thus this goroutine isn't returning when it should... (basically it's always in iteration 1 of the for loop...)
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.
Yeah, the goroutine is leaking when the OTP Bar finishes on line 52, I think.
Related: https://benjamincongdon.me/blog/2020/04/23/Cancelable-Reads-in-Go/
Seems like there isn't an easy way out...
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 think I'll have to refactor to use a cancelable reader, using a io.Reader
on os.Stdin
and test it on Windows hoping it will work fine without all that boilerplate since we are just caring about key presses...
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 change looks fine and seems to work well.
But I was immedeately thinking that we should maybe keep generating new OTP codes, just like the Autenticator apps do.
That seems like a nice idea, and quite easy to implement I guess :D |
Don't feel obliged to add that. I'm not a huge fan of the OTP feature anyway, but since we have it we could try to make it as nice and useful as it can be. Up to you. |
RELEASE_NOTES=[UX] OTP code now runs in loop until canceled or used with -o Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
RELEASE_NOTES=n/a Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
It was actually easier to "fix" the Go routine leak by not fixing it and having a loop so that it's mandatory to cancel the loop and there's no risk of having a character being eaten by the leaked go routine when we're back to gopass prompt view. |
Seems to work fine. Thank you. |
Looks like another major behavior change, change of default, and no global config for this? Or maybe I skimmed the diff too quickly? |
@colemickens I'm not sure how that's a change of behaviour? |
It's a big change for anyone that had scripts that relied on "show --clip" and "totp --clip" being one-shot behavior. AFAICT there's no way for me to have the same keyboard shortcut working before without doing something like chaining Not a huge deal but it is a change in UX and sadly I just hit another (unrelated) change in |
@colemickens I still don't get it. Before this PR we had a progress bar that you couldn't cancel. How would that break We can discuss if we should print the progress bar when |
Unless I'm bad at reading Go lately. the PR seems to add a loop that wasn't there before that causes the OTP generation to loop. I have a keybinding that pops up a terminal, runs It does seem like there's a short-circuit for when it's being piped, so I can probably workaround this. |
The actual loop was already present before this PR. Try it yourself: I'm not sure if we should be printing the progress bar at all when |
Yeah, the fact the progress bar now prints when using |
BTW, any unexpected behaviour change warrants an issue, don't hesitate to open issue whenever you run into something breaking. Don't hesitate to open issues if you run into anything that breaks your workflow after any update. |
* RELEASE_NOTES=[ENHANCEMENT] Adding option to exit OTP progress bar Signed-off-by: Yolan Romailler <yolan@romailler.ch> * applying code review comments RELEASE_NOTES=n/a Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com> * patching CI RELEASE_NOTES=n/a Signed-off-by: Yolan Romailler <yolan@romailler.ch> * Adding a loop for OTP RELEASE_NOTES=[UX] OTP code now runs in loop until canceled or used with -o Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>
As discussed in #2019