-
Notifications
You must be signed in to change notification settings - Fork 326
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
Add hidecursorinactive option to indicate status when terminal loses focus #965
Conversation
ui.go
Outdated
@@ -416,7 +416,7 @@ func (win *win) printDir(screen tcell.Screen, dir *dir, selections map[string]in | |||
} | |||
} | |||
|
|||
if i == dir.pos { | |||
if i == dir.pos && gOpts.focus == "on" { |
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.
Minor nit: I'm not sure if "focus" is the right name for this option. At this point, it's an option that chooses whether to draw cursors, and the user decides whether that has anything to do with focus.
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.
Maybe instead add a 'cursorfmt' option that takes a format string like 'tagfmt' so you can apply an arbitrary style.
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 agree, the name is not ideal.
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've changed it back to be a bool since ilyagr does not need it anymore and cursor is not quite right (we already have a text cursor).
I actually stopped using focus events in favor of making the tmux background of the active pane slightly darker. Specifically, I use the terminal background " I may or may not implement the grey cursors on top of this PR. I'm not sure anybody is using it. |
Not a huge fan of this. It adds a lot of commands/options to support an (IMO) pretty minor feature. |
@p-ouellette you got it a bit backwards. The point is not to add this one feature but to make it possible to implement more in user code and to require less code and maintenance in lf. |
What's the usecase for on-ui-enter, on-ui-exit, and echoraw outside of supporting focus events? They seem like workarounds for missing tcell focus event support to me. |
Haven't implemented it yet but I want do use
|
959bf05
to
e58f168
Compare
I removed the |
e58f168
to
df36ad3
Compare
Rebased to current master. |
@ilyagr Since |
Setting If it was necessary, I think I'd go with |
… on-ui-enter/on-ui-exit events.
df36ad3
to
77f1136
Compare
Perfect, I hadn't thought of that. That simplifies this PR. |
@gokcehan @ilyagr @joelim-work this PR has changed a lot as quite a few features are now part of lf and so the implementation is a bit smaller.
I've been maintaining this for almost a year but it would be great if it could be merged. I'd consider it thoroughly tested ;) |
I haven't made up my mind on anything, but here are some quick thoughts. @gokcehan @joelim-work , feel free to chime in. If this PR is about focus events, it could use gdamore/tcell#599 when that's released (if it's not already). Then, instead of Depending on whether the If we do neither, this feature looks cryptic in the documentation. Perhaps it could link to the wiki where you'd have an explanation of how to use this, but I'd prefer the documentation to be self-sufficient. (Again, this is a preference. I'm not saying this is an absolute requirement, I haven't thought about it enough to have a clear opinion). |
@laktak @ilyagr Similar to the above #965 (comment), I am against handling focus events directly in the Regarding the design, perhaps it's fine to enable focus reporting by default and provide |
Thanks for the feedback. Handling this via config would give the user a lot more flexibility but if we reduce this to just focus events, then letting tcell handle it (and maintaining it) is a good option. There is one other use case that I can think of (disabling the tmux paste command when lf has focus) but I don't think that's very important. I'll wait for the tcell release for now. |
@joelim-work Thanks, works great now! I've replaced everything with:
|
At this point, after having added and removed various events in this PR ;), I think adding it as just an option would make the most sense because you don't need to look at the wiki to set it up. I'd even set it as default because that's the standard behavior for a terminal window. |
After thinking about this some more, I think it might be fine to just have it as a bool option. I suspect there aren't many users who care about focus events, so there is no need to expose hooks, and even if we decide to replace I am not a fan of changing the default behavior though. The last time something like this happened was changing Also please update the PR title and description to reflect the new status. |
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.
So the changes now look good to me and I can give my approval. Sorry couple more comments, PR looks much better overall though.
The following is more of a general concern from me and doesn't apply to just this PR:
As a collaborator, I actually do have permission to merge this change, but I'm not sure whether you are in a rush to have this merged or if you want to wait for a second review (I don't know how long this could take). Up until now, as I am not the owner of lf
, I have so far taken the conservative approach and merged only minor changes like bug fixes and Dependabot updates. As much as I want to keep the development of lf
active, I also have reservations about just 'taking over' the project.
Anyway I do intend to take a break until the new year, so this can be discussed later. In the meantime, thanks once again for the PR and have a good break 🙂.
IMO it would be a nice PR to handle all boolean option variations in a centralized manner (instead of the Happy holidays! |
It looks good to me. I'm happy to finally see this feature getting merged with an appropriate implementation. Thank you @laktak for the patch and @joelim-work and others for the reviews and feedbacks. Happy holidays. |
@laktak There actually is such a discussion, see #1552. I doubt you are the only person who is worried about making a typo when copy/pasting the boilerplate code for a new boolean option. |
When
hidecursorinactive
is enabled, focus events from the terminal will hide/show the cursor when the terminal (or tmux pane) loses/gets focus.This is similar to how most terminals and tmux hide the cursor in inactive windows/panes.
OLD VERSION - Enable focus events through configuration
Enable focus events through configuration by using focus (option), on-ui-enter/on-ui-exit (events)
and echoraw (command).This enables the same functionality as #861 but stores all escape sequences (both ways) in the configuration.
For example to enable focus events with tmux (I will add this to the wiki if the PR gets merged):
(updated for the current version)
@ilyagr it's still a on/off option at the moment but since focus is now defined as a string option, you should be able implement something likeset focus dark-grey
on top of this. Please let me know if you need more than that.