feat: max_width for adaptive_size #1915

ramezgerges · 2023-01-12T14:19:03Z

resolves #1797

P.S> I'm not sure if this is an issue, but the width of NvimTree doesn't seem to be pixel perfect. The width is off by 2 every time. This is not caused by my changes (I tested this using the provided clean room config).

alex-courtis · 2023-01-14T03:40:22Z

P.S> I'm not sure if this is an issue, but the width of NvimTree doesn't seem to be pixel perfect. The width is off by 2 every time. This is not caused by my changes (I tested this using the provided clean room config).

That would be the sign column. We need to remember that the user may disable it.

alex-courtis · 2023-01-14T04:01:26Z

This is a preexisting issue: 2 is "added" to width when setting adaptive size less than max:

This is not strictly part of this PR, however I would be most grateful if you resolved this.

alex-courtis · 2023-01-14T04:32:48Z

Strange behaviour with small sizes:

    view = {
      adaptive_size = true,
      width = 30,
      max_width = 10,

alex-courtis

Thank you for your contribution, this will greatly improve the user experience.

Tested mostly OK:

max_width 30
max_width function
max_width function returning garbage, OK as same as width
max_width percent
max_width -1

Please:

Account for view.signcolumn and remove magic number feat: max_width for adaptive_size #1915 (comment)
Resolve small window size incorrectness feat: max_width for adaptive_size #1915 (comment)

lua/nvim-tree/view.lua

ramezgerges · 2023-01-14T18:32:05Z

Strange behaviour with small sizes:

    view = {
      adaptive_size = true,
      width = 30,
      max_width = 10,

After an unnecessarily long amount of digging, it turns out that the problem is

nvim-tree.lua/lua/nvim-tree/view.lua

Lines 300 to 302 in 3ce0a8e

    
           if not M.View.preserve_window_proportions then 
        
             vim.cmd ":wincmd =" 
        
           end

Moving it to the top solves the problem (still no idea why it was resized to that exact value though), but I don't really understand what view.preserve_window_proportions is supposed to be doing. I understand that it equalizes the other windows, but it does so on any resize invocation (so basically on any redraw) and not specifically when opening a file (as is written in the docs), so is there something wrong, or is that intended?

ramezgerges · 2023-01-14T18:35:14Z

How about width_max, just more pleasing visually. Or make width a table?

I'm not sure how to respond to this :). I prefer max_window, but don't mind it either way. Not sure who has the final call on naming conventions.

alex-courtis · 2023-01-15T00:32:54Z

How about width_max, just more pleasing visually. Or make width a table?

I'm not sure how to respond to this :). I prefer max_window, but don't mind it either way. Not sure who has the final call on naming conventions.

Maybe adaptive_max...

A table would be nice:

    width = {
      adaptive = true,
      min = 30,
      max = 50,
    },

As usual we would need to do a silent refactor.

We could even get rid of the adaptive variable. If the user specifies max > min it implies adaptive. Default both to 30.

alex-courtis · 2023-01-15T00:44:23Z

Moving it to the top solves the problem (still no idea why it was resized to that exact value though), but I don't really understand what view.preserve_window_proportions is supposed to be doing. I understand that it equalizes the other windows, but it does so on any resize invocation (so basically on any redraw) and not specifically when opening a file (as is written in the docs), so is there something wrong, or is that intended?

Here be dragons. I now vaguely recall some vim dramas around winwidth and winfixwidth for small numbers. The live filter overlay is hardcoded to 20, probably to work around such issues.

The default winwidth is 20, which might explain some things.

Let's not deal with that here.

A pragmatic solution may simply be to always specify / percentage calculate a width >=20

alex-courtis · 2023-01-15T01:22:00Z

P.S> I'm not sure if this is an issue, but the width of NvimTree doesn't seem to be pixel perfect. The width is off by 2 every time. This is not caused by my changes (I tested this using the provided clean room config).

That's clearly a bug. The padding should be 2 or 0 based on sign column visibility.

Pushed a fix.

gegoune · 2023-01-15T01:32:09Z

We could even get rid of the adaptive variable. If the user specifies max > min it implies adaptive. Default both to 30.

Still need a way to specify static size. I propose:

width = number -- for static size
width = {min = number, max = number} -- for adaptive

alex-courtis · 2023-01-15T01:38:17Z

yy

We could even get rid of the adaptive variable. If the user specifies max > min it implies adaptive. Default both to 30.

Still need a way to specify static size. I propose:
width = number -- for static size
width = {min = number, max = number} -- for adaptive

Nice. No big refactoring required.

Users with adaptive_size = true can be silently migrated to width = {min = 20, max = -1}. <20 is broken anyway so they get a free fix.

alex-courtis · 2023-01-15T03:48:37Z

Pushed some changes. Please feel free to revert!

fix the sign column silliness
apply a minimum width of 20
use a table
harden the function / string widths from the user

It was hard to resist further refactorings...

Needs review and extensive testing from @ramezgerges

alex-courtis

@ramezgerges review
@ramezgerges implement the "space is the end" indicator for all cases

lua/nvim-tree/view.lua

ramezgerges · 2023-01-15T14:39:24Z

lua/nvim-tree/view.lua

@@ -246,7 +246,7 @@ end
 local function grow()
  local starts_at = M.is_root_folder_visible(require("nvim-tree.core").get_cwd()) and 1 or 0
  local lines = vim.api.nvim_buf_get_lines(M.get_bufnr(), starts_at, -1, false)
-  local padding = 3
+  local padding =  M.View.winopts.signcolumn and 2 or 0


Do we want to add an extra character (I.e. 3 instead of 3) as it was previously? I personally find the extra character useful as it indicates that I haven't reached the maximum width yet (i.e. the filename is fully shown and not clipped)

Ah I think you got it... I didn't realise that the extra space was useful.

Yes, let's put that back and ensure that it works for max = -1 and max = 30

ramezgerges · 2023-01-15T15:27:53Z

Still need a way to specify static size. I propose:
width = number -- for static size
width = {min = number, max = number} -- for adaptive

I like this as well. I also think a useful addition would be to have adaptive_size be toggleable. Sometimes one might want to fix NvimTree to a fixed (wide) size, do some browsing, and then toggle back the adaptiveness. So, I think we might want

A fixed width for users who don't care whatsoever about this.
A (min,max) size pair for adaptiveness
A toggle for adaptiveness

So maybe

width = number
width = {
    adaptive_width = boolean,
    min = number
    max = number
}

with perhaps an NvimTreeToggleAdaptive command or something.

Edit: sorry for closing/reopening. Misclicked. Currently on my phone :) I'm a bit busy with exams but I will try to find the time to thoroughly test the commits

gegoune · 2023-01-15T16:51:30Z

I feel like switching between adaptive and not should be something interested users would do themselves with API. If current API doesn't allow it then it could be expanded.

alex-courtis · 2023-01-16T03:16:48Z

I feel like switching between adaptive and not should be something interested users would do themselves with API. If current API doesn't allow it then it could be expanded.

Having the adaptive_width boolean doesn't quite feel right. It's not a configuration time thing, it's a runtime thing.

API works: allow the user to set width. They can set min=25, max=50 when they want adaptive, min=25,max=25 when they want fixed.

They could even just set it to an integer when they want fixed and a table when they want adaptive.

I'm not quite sure how that API would look; it's a bit different to anything existing.

Proposal: release this now, with the padding put back, then we think about API.

ramezgerges · 2023-01-17T14:06:40Z

Alright, I finally found time for testing.

Some observations:

Firstly, the MIN_WIDTH hack is working fine for now. It does technically break behavior for old configs with width < 20. However, it was already buggy, so I guess that's reasonable. I'm just pointing it out as a reminder.
Not sure if it's the same issue as- Resize event not dispatched when resizing using mouse #1545, but on resizing by mouse, the window snaps back to its previous width on the next redraw. This is not caused by this PR, but it exists.
Side-note: I'm able to reproduce the width < 10 snapping issue even before the PR commits, so this wasn't caused by this PR, which makes sense (I was wondering what I could've possibly changed that caused this).
There seems to be an issue with window splitting. Splitting into a 2x2 setup doesn't seem to align perfectly. Obviously a minor issue, but I will leave it up to you to decide if an issue should be opened to tackle this.
This was reproduced with width = 10 and width = 40. Not caused by this PR.

(Notice how I move bottom left window to the right manually to show the difference.)

These are the ones that I could spot for now. I'm not the most perceptive person in the world, I may have missed a problem or two :).

One more thing about `MIN_WIDTH`. I can look into `vim.cmd ":wincmd ="` to see if we can find a fix for this. Is this something worth spending time on, or should I just forget about it? I assume that would be a separate PR, but I'm just asking if it's worth looking into. I found the `MIN_WIDTH` fix so uncomfortable :).

alex-courtis · 2023-01-21T02:45:52Z

* Firstly, the `MIN_WIDTH` hack is working fine for now. It does technically break behavior for old configs with width < 20. However, it was already buggy, so I guess that's reasonable. I'm just pointing it out as a reminder.
One more thing about MIN_WIDTH. I can look into vim.cmd ":wincmd =" to see if we can find a fix for this. Is this something worth spending time on, or should I just forget about it? I assume that would be a separate PR, but I'm just asking if it's worth looking into. I found the MIN_WIDTH fix so uncomfortable :).

I think you are right here. This is disruptive to the user, and there will be some who are using width < 20 and dealing with any issues.

I'll back that change out; width 20 is just a bad hack. I would be most grateful if you could look into the root issue it in a future PR.

alex-courtis

Thank you for your contribution and all the detailed analysis/testing @ramezgerges

alex-courtis · 2023-01-21T03:56:06Z

Reverted my very broken "fix" for sign column:

local padding = (M.View.winopts.signcolumn and 2 or 0) + 1

…ibility

feat: max_width for adaptive_size

39aadfc

alex-courtis requested changes Jan 14, 2023

View reviewed changes

lua/nvim-tree/view.lua Show resolved Hide resolved

alex-courtis mentioned this pull request Jan 14, 2023

feat: make fs actions async #1863

Closed

This comment was marked as resolved.

Sign in to view

view grow calculates size correctly based on sign column visibility

cdfcf30

alex-courtis added 3 commits January 15, 2023 13:47

limit width to a minimum of 20

f2181e6

adaptive_size -> min/max table

dac0920

harden view size calculations against bad user input

4b06efe

style

5442e10

alex-courtis requested changes Jan 15, 2023

View reviewed changes

lua/nvim-tree/view.lua Show resolved Hide resolved

ramezgerges commented Jan 15, 2023

View reviewed changes

ramezgerges closed this Jan 15, 2023

ramezgerges reopened this Jan 15, 2023

add back an extra column of padding to adaptive resizing

b70abdb

ramezgerges requested a review from alex-courtis January 20, 2023 15:46

alex-courtis added 2 commits January 21, 2023 14:15

back out: limit width to a minimum of 20

ae7d6ec

revert unnecessary change

8e5e019

alex-courtis approved these changes Jan 21, 2023

View reviewed changes

alex-courtis added 3 commits January 21, 2023 14:56

backout: view grow calculates size correctly based on sign column vis…

f9a033d

…ibility

remove adaptive_size from help

c1d1160

backout unnecessary change M.View.config

33e426e

alex-courtis merged commit 96506fe into nvim-tree:master Jan 21, 2023

Uh oh!

feat: max_width for adaptive_size #1915

feat: max_width for adaptive_size #1915

Uh oh!

Conversation

ramezgerges commented Jan 12, 2023

Uh oh!

alex-courtis commented Jan 14, 2023

Uh oh!

alex-courtis commented Jan 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-courtis commented Jan 14, 2023

Uh oh!

alex-courtis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

ramezgerges commented Jan 14, 2023

Uh oh!

ramezgerges commented Jan 14, 2023

Uh oh!

alex-courtis commented Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-courtis commented Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-courtis commented Jan 15, 2023

Uh oh!

gegoune commented Jan 15, 2023

Uh oh!

alex-courtis commented Jan 15, 2023

Uh oh!

alex-courtis commented Jan 15, 2023

Uh oh!

alex-courtis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ramezgerges Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

alex-courtis Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

ramezgerges commented Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gegoune commented Jan 15, 2023

Uh oh!

alex-courtis commented Jan 16, 2023

Uh oh!

ramezgerges commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-courtis commented Jan 21, 2023

Uh oh!

alex-courtis left a comment

Choose a reason for hiding this comment

Uh oh!

alex-courtis commented Jan 21, 2023

Uh oh!

Uh oh!

alex-courtis commented Jan 14, 2023 •

edited

Loading

alex-courtis left a comment •

edited

Loading

alex-courtis commented Jan 15, 2023 •

edited

Loading

alex-courtis commented Jan 15, 2023 •

edited

Loading

alex-courtis left a comment •

edited

Loading

ramezgerges commented Jan 15, 2023 •

edited

Loading

ramezgerges commented Jan 17, 2023 •

edited

Loading