Entering CTRL+D at the input prompt breaks hotkey control #184

New Issue

asdf · 2020-07-04T23:49:36Z

asdf commented

2020-07-04 23:49:36 +00:00

Found this while testing this issue in linkulator, reported by @cmccabe. Affects current development versions as well as the previous release.

Steps to reproduce:

Bring up a prompt by pressing space or :
Press CTRL+D

Results:

The error message EOF is displayed.
Hotkeys are printed to the lower left corner of the screen. They can be entered ultiple times without taking action. They will only take action unless you press enter.
The standard control scheme can only be reestablished by restarting the application.

This is pretty minor so no rush to fix. I can take a look at it, but as per Linkulator I'm not sure what should actually happen. Should it just be disregarded?

Found this while testing this issue in linkulator, reported by @cmccabe. Affects current development versions as well as the previous release. Steps to reproduce: 1. Bring up a prompt by pressing space or : 2. Press CTRL+D Results: 1. The error message EOF is displayed. 2. Hotkeys are printed to the lower left corner of the screen. They can be entered ultiple times without taking action. They will only take action unless you press enter. 3. The standard control scheme can only be reestablished by restarting the application. This is pretty minor so no rush to fix. I can take a look at it, but as per Linkulator I'm not sure what should actually happen. Should it just be disregarded?

asdf added the

non-urgent

label 2020-07-04 23:49:36 +00:00

sloum commented

2020-07-06 04:53:39 +00:00

I would like to get it fixed, but I would not call it urgent. It is user recoverable though, without restarting the application.

If you enter a command (rather than a hot key) it will restore normal opperation. Steps:

Press : to enter command mode
Input ^D
- EOF will appear
- All key entry will be printed to the screen
Press enter to go to new line
Input :home followed by enter (any command is fine)
You will now be back in a normal mode, though the display may still be borked. Press j or k to scroll and the screen will be redrawn and normal opperation can resume.

Now, I dont expect a regularl user to do all that, but I also dont expect them to have sent an EOF signal in the first place. :-/

I'm going to mark this as a bug and non-urgent. We can likely catch the signal if that is how it is being interpreted. However, it is doing what it should: ^D should send EOF to the input... so we may have to do a special catch inside the input to check for it.

I would like to get it fixed, but I would not call it urgent. It is user recoverable though, without restarting the application. If you enter a command (rather than a hot key) it will restore normal opperation. Steps: 1. Press `:` to enter command mode 2. Input ^D - EOF will appear - All key entry will be printed to the screen 3. Press `enter` to go to new line 4. Input `:home` followed by `enter` (any command is fine) 5. You will now be back in a normal mode, though the display may still be borked. Press `j` or `k` to scroll and the screen will be redrawn and normal opperation can resume. Now, I dont expect a regularl user to do all that, but I also dont expect them to have sent an EOF signal in the first place. :-/ I'm going to mark this as a bug and non-urgent. We can likely catch the signal if that is how it is being interpreted. However, it is doing what it should: ^D should send EOF to the input... so we may have to do a special catch inside the input to check for it.

sloum added the

bug

label 2020-07-06 04:53:49 +00:00

asdf commented

2020-09-06 06:29:59 +00:00

I thought this was an issue with EOF handling. This does not seem to be the case, it's just that we don't reenable character mode when handling errors in cui.GetLine:

func GetLine(prefix string) (string, error) {
    termios.SetLineMode() // LINE MODE ENABLED
    ...
    text, err := reader.ReadString('\n')
    if err != nil {
        return "", err //RETURN BEFORE CHARACTER MODE IS ENABLED
    }
    termios.SetCharMode() // CHARACTER MODE ENABLED
    ...
}

Adding termios.SetCharMode() within the error handling block, before we return, seems to resolve this issue. I thought using defer termios.SetCharMode() right after we enable line mode might be neater, so I've done this in branch handle_input_errors.

@sloum let me know what you think. If you prefer a PR to review, should I target branch release2.3.2?

I thought this was an issue with EOF handling. This does not seem to be the case, it's just that we don't reenable character mode when handling errors in `cui.GetLine`: ```Go func GetLine(prefix string) (string, error) { termios.SetLineMode() // LINE MODE ENABLED ... text, err := reader.ReadString('\n') if err != nil { return "", err //RETURN BEFORE CHARACTER MODE IS ENABLED } termios.SetCharMode() // CHARACTER MODE ENABLED ... } ``` Adding `termios.SetCharMode()` within the error handling block, before we return, seems to resolve this issue. I thought using `defer termios.SetCharMode()` right after we enable line mode might be neater, so I've done this in branch handle_input_errors. @sloum let me know what you think. If you prefer a PR to review, should I target branch release2.3.2?

sloum commented

2020-09-06 15:19:02 +00:00

Ah, that makes sense! I would love a PR if you dont mind. Can you create the branch release2.3.3 off of develop and push that then create a branch for this change (again off of develop) and PR it into release2.3.3? I can add the release branch if you'd prefer.

I think the defer route is maybe the good one? Though it does mean that we have the overhead of always calling SetCharMode on every call, rather than just on errors... What do you think? I'm also not sure about the code overhead for defer, I think it is likely a jump or a goto under the hood... but I'm not sure.

Either way of handling it (defer or just in the error block) sounds fine to me.

Ah, that makes sense! I would love a PR if you dont mind. Can you create the branch `release2.3.3` off of `develop` and push that then create a branch for this change (again off of develop) and PR it into `release2.3.3`? I can add the release branch if you'd prefer. I think the `defer` route is maybe the good one? Though it does mean that we have the overhead of always calling `SetCharMode` on every call, rather than just on errors... What do you think? I'm also not sure about the code overhead for defer, I think it is likely a jump or a goto under the hood... but I'm not sure. Either way of handling it (defer or just in the error block) sounds fine to me.

asdf referenced a pull request that will close this issue

2020-09-08 03:48:41 +00:00

Handle input errors while returning to correct terminal mode #186

sloum closed this issue

2020-09-08 04:18:08 +00:00

sloum commented

2020-09-08 04:19:22 +00:00

Closed by #186 , seemingly automagically? I had planned on just commenting that it was in a release branch and I'd close it once that gets merged in... but I suppose this works too.