Implement focus events #599
Conversation
tscreen.go
Outdated
| @@ -1804,6 +1847,7 @@ func (t *tScreen) engage() error { | |||
| t.stopQ = stopQ | |||
| t.enableMouse(t.mouseFlags) | |||
| t.enablePasting(t.pasteEnabled) | |||
| t.enableFocusReporting() | |||
There was a problem hiding this comment.
Is it ok to do this unconditionally, or do we need a t.focusReportingEnabled flag to opt in?
There was a problem hiding this comment.
I have mixed feelings. I think it's probably ok to be unconditional -- hopefully applications will know to ignore events that they don't understand.
There was a problem hiding this comment.
Sorry to jump in out of the blue, but if this code enables focus reporting unconditionally for all applications, I think it'd be better to make it opt-in. Focus reporting has side-effects if, for example, if you are using a program that enabled focus reporting over ssh and the ssh connection drops. The user is then surprised to have text printed on their screen (probably in their shell) any time they focus or unfocus the window.
This could be considered a bug in ssh, or in the shells that don't know how to interpret the sequence, but it's pervasive. I think it's present in mosh. In any case, tmux does not turning focus reporting on unless explicitly asked to, and I think most applications follow this approach. Having focus reporting be turned off by default should be possible for applications using tcell, IMO.
Apologies if I misunderstood what this code does.
There was a problem hiding this comment.
I understand your concern, but I don't think making focus reporting opt in helps in any way.
I simulated your ssh scenario by simply kill -9 a tcell application (lazygit in my case, but you could also use _demos/mouse.go). If you kill -9 it, the terminal is in a very messed up state anyway, it spews mouse reporting escape codes and other things, and users just have to know that they need to type reset in such a case to get back to a normal terminal. Whether we also keep sending focus events or not doesn't really make a big difference in this case.
There was a problem hiding this comment.
OK, I take that back. 😄 I didn't realize that mouse reporting is also opt-in, so only applications that use it suffer from the problem. It totally makes sense then to treat focus reporting in the same way. See 89aca70.
There was a problem hiding this comment.
Thanks, I appreciate it! I was going to pin down a way to demonstrate where this makes a difference and make a screencast. I don't have to now, but I know it does make a difference with the way I use ssh/mosh. I believe making this opt-in will help many programs and users that don't know about focus reporting.
Also, thanks for implementing this feature in the first place! 🎉
tscreen.go
Outdated
| @@ -1043,6 +1043,14 @@ func (t *tScreen) enablePasting(on bool) { | |||
| } | |||
| } | |||
|
|
|||
| func (t *tScreen) enableFocusReporting() { | |||
| t.TPuts("\x1b[?1004h") | |||
There was a problem hiding this comment.
This is the part of your code I have a problem with.
Not every terminal will understand these, and for those that don't this may be problematic.
At a minimum this probably needs to be limited to only being used for terminals that support mouse reporting. I think probably any terminal that supports mouse reporting will also support this.
Also, some terminals may want to override this value. See the enablePaste and disablePaste terminal settings.
FIxing this to do it the right way might be more effort than you want to undertake. If it is, let me know, and I can probably take this the rest of the way over the finish line -- this does seem like it could well be a useful feature and so I want to have it included. (And thank you very much for taking it this far already, btw.)
There was a problem hiding this comment.
I pushed a simple attempt at doing this (see 7521492), but I'm not sure I understand all of it. For Terminfo.EnablePaste I don't see any code anywhere that actually sets it to something...
There was a problem hiding this comment.
I commented there... have a look at how the prepareCursorStyles() or prepareBracketedPaste works.
There was a problem hiding this comment.
maybe add stuff to prepareExtendedOSC ? This isn't an OSC, but looking at it, neither is the code for setting Window Size.
There was a problem hiding this comment.
Is this getting closer? 0732b36
stefanhaller
force-pushed
the
focus-events
branch
2 times, most recently
from
7521492
to
0732b36
Compare
|
|
||
| func (s *simscreen) DisableFocus() { | ||
| } | ||
|
|
There was a problem hiding this comment.
I didn't bother setting a flag on s here like it's done for mouse and paste; as far as I can tell, s.mouse and s.paste are not used anywhere, so I don't understand why they exist. Let me know if I'm missing something, or whether you'd like me to maintain an unused s.focus flag for consistency.
|
@gdamore Is there anything I can do to help move this forward? This would be ready to go from my side. |
|
@gdamore Friendly ping... It would be great to get this merged so that it can be used in downstream projects. |
We assume that any terminal that supports mouse reporting will also support focus reporting; but we also add an entry to Terminfo to let specific terminals override it if needed.
stefanhaller
force-pushed
the
focus-events
branch
from
89aca70
to
8fc93f2
Compare
|
I pushed a fixup to fix the Windows build, sorry for not realizing this earlier. I see that a similar fix is also needed for |
This is temporary as long as gdamore/tcell#599 is not merged. Once that PR is merged, we can revert this.
This is temporary as long as gdamore/tcell#599 is not merged. Once that PR is merged, we can revert this.
|
We'll probably need to follow up with a fix for wScreen. |
Add a new event that reports when the terminal window gets or loses focus.
I only tested this on the Mac; it works with Terminal.app, iTerm2, and in a VS Code terminal window.