Skip to content

feat: add float autoclose option #1621

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

Merged
merged 1 commit into from
Oct 8, 2022

Conversation

emmanueltouzery
Copy link
Contributor

I like the float mode of the treeview, however I'd like it not to autoclose when the popup loses focus.

So I've added an option to that effect.

@alex-courtis
Copy link
Member

I asked for that to be unconditionally added as I didn't consider the use case that the user desires this functionality.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

view.float.autoclose = true

Tested OK:

  • window focus is changed

Needs work:

  • actions.open_file.quit_on_open = false: float closes when opening file

@emmanueltouzery
Copy link
Contributor Author

Yes, the close_on_open was hardcoded as per the original PR #1462 -- the comment was that it didn't work properly otherwise.

This is also documented:

*nvim-tree.view.float.enable*
        Display nvim-tree window as float (enforces |nvim-tree.actions.open_file.quit_on_open| if set).
          Type: `boolean`, Default: `false`

I didn't have that much testing time with the feature yet, so I'm not sure what I'd prefer yet. Right now I think that I'd like the setup of this PR (don't close on focus loss, close on open).

I understand that the settings as set up by this PR are confusing.

What about if we rename this setting to auto_close_when_unfocused? Or maybe auto_close_on_focus_loss? And we reserve the option to potentially one day respect quit_on_open also for the floating configuration, which I view as an independent thing that also wasn't supported so far.

@alex-courtis
Copy link
Member

This is also documented:

Good catch! That can be changed.

I didn't have that much testing time with the feature yet, so I'm not sure what I'd prefer yet. Right now I think that I'd like the setup of this PR (don't close on focus loss, close on open).

I understand that the settings as set up by this PR are confusing.

What about if we rename this setting to auto_close_when_unfocused? Or maybe auto_close_on_focus_loss? And we reserve the option to potentially one day respect quit_on_open also for the floating configuration, which I view as an independent thing that also wasn't supported so far.

We can keep it simple and do it now: floating window respects both actions.open_file.quit_on_open and view.float.autoclose now. No need for another option.

autoclose -> quit_on_focus_loss might be the clearest, consistent option.

@alex-courtis
Copy link
Member

RE defaults: users will experience disruption if they relied on the implicit quit_on_open, however I think that is a reasonable change for a newly released feature.

@emmanueltouzery
Copy link
Contributor Author

I don't understand the disruption? quit_on_open didn't work for the floating window setup, and still doesn't? So it seems to me there is no change?

I'll update the PR with the setting rename.

@alex-courtis
Copy link
Member

I don't understand the disruption? quit_on_open didn't work for the floating window setup, and still doesn't? So it seems to me there is no change?

The implicit quit_on_open is always firing, regardless of autoclose. My proposal is that we always respect the user's quit_on_open setting for floating windows. This will be a more predictable user experience and offers the full range of behaviour choices.

Making both changes now will be less disruptive and I would be very greatful if you could make this change in this PR.

@emmanueltouzery
Copy link
Contributor Author

ok, i'll research what is the breakage that the quit_on_open locking was supposed to workaround. i think fixing that shouldn't be too hard.

@emmanueltouzery
Copy link
Contributor Author

OK, I've updated the PR. I do not understand what was the issue with quit_on_open. I removed the setting blocking it and.. it worked right away for me. I couldn't find any issue... It seems to work just fine for me.

It does turn out there can be a conflict between both options. If you say quit_on_focus_loss=true but quit_on_open=false... i mean when we open a file, we focus it. Therefore it's a focus loss. But the three other cases are clear.

quit_on_focus_loss=true
quit_on_open=true
=> no problem, clear what should happen (and it's the current behavior)

quit_on_focus_loss=false
quit_on_open=true
=> ok, clear what should happen

quit_on_focus_loss=false
quit_on_open=false
=> clear

quit_on_focus_loss=true
quit_on_open=false
=> ambiguous, quit_on_focus_loss wins and effectively we have quit_on_open=true enforced

quit_on_focus_loss=false
quit_on_open=false
=> ok, clear what should happen

I've tested the four cases, everything looks good to me. I intend to use true+true for now, maybe i'll switch to quit_on_focus_loss=false+quit_on_open=true, I'll see. But so far I can't see any issues.

@emmanueltouzery
Copy link
Contributor Author

Ok, I found an issue. The tree contents are not updated when we switch buffers. I'll check it out.

@emmanueltouzery
Copy link
Contributor Author

false alert: I needed update_focused_file.update_root=true. I could have sworn that I didn't need that in the past. Anyway, nevermind. I know of no bugs right now.

@alex-courtis
Copy link
Member

Thanks for updating. Unfortunately I'm not able to look at this until the weekend.

@emmanueltouzery
Copy link
Contributor Author

no problem, thank you! In the meantime I'll keep using it through the week and be careful for glitches. As I said, I intend to use true+true for now, maybe i'll switch to quit_on_focus_loss=false+quit_on_open=true, I'll see.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested OK.

Defaults match current behaviour.

Thanks for going the extra mile.

@alex-courtis alex-courtis merged commit 79f631b into nvim-tree:master Oct 8, 2022
Almo7aya pushed a commit to Almo7aya/nvim-tree.lua that referenced this pull request Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants