Skip to content
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

Percentage width support #95

Merged
merged 7 commits into from Aug 25, 2018
Merged

Conversation

SidOfc
Copy link
Contributor

@SidOfc SidOfc commented Aug 22, 2018

This is related to #94.

It allows g:codi#width to accept a float and a percent-like string (e.g. '33.54%') aside from the default number type. This is handled in the following way:

  • If it is a number (e.g. 30, 40) then return it
  • If it is a float (e.g. 33.33, 33) or percent-like string (e.g. '33.45%', '33%')
    • then calculate the pane width based on the current pane
      • If the value is larger than 100.0% then clamp it to 100.0%
      • If the value is less than 0.0% then clamp it to 0.0%

Finally, if either pane width is less than Vims :h minwidth setting, the minimum width will be respected for that pane instead. Should this setting not exist a default of 20 columns will be used.
This only happens when a float or percent-like string is used. If an absolute width is used then it will be used as-is without respecting Vims minwidth.

All this adds a little bit of bulk but it is worth the safety-nets that prevent a pane from going out of bounds or barely visible. I've tried to apply any convention I could use, tried to put the function in a decent place and used s:get_opt('width') as my input parameter, maybe other parts need to be changed or improved, let me know :)

I've tested it in Vim, NeoVim and Mvim and they all behave as expected (see #94 for version info).

@SidOfc SidOfc changed the title percentage width support Percentage width support Aug 22, 2018
Fix `s:pane_width` description
Copy link
Owner

@metakirby5 metakirby5 left a comment

My main feedback is that we should drop support for X% syntax to avoid unnecessary string manipulation. Just supporting int and float is fine.

doc/codi.txt Outdated
@@ -246,6 +246,11 @@ g:codi#autocmd
*g:codi#width*
g:codi#width
The width of the Codi split.
It can be a |Number|, a |Float| or a percentage string such as
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

We should specify that in the float case, the option will be treated as a percentage.

Actually, since float and percent do the same thing, it would be more clear (and require less code) to only support float.

Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

Also, please add a line break above here.

doc/codi.txt Outdated
@@ -246,6 +246,11 @@ g:codi#autocmd
*g:codi#width*
g:codi#width
The width of the Codi split.
It can be a |Number|, a |Float| or a percentage string such as
`'33%'` or `'33.33%'`. The width will be clamped between `0.0`
and `100.0`. This plugin respects the |minwidth| variable if it
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

Wording nit: "This setting respects the |minwidth| variable..."

doc/codi.txt Outdated
It can be a |Number|, a |Float| or a percentage string such as
`'33%'` or `'33.33%'`. The width will be clamped between `0.0`
and `100.0`. This plugin respects the |minwidth| variable if it
is present, otherwise the minimum remaining width is 10 columns.
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

Did you mean to say 20? Anyways, I think we should just say that we respect the variable, end of story.

Copy link
Contributor Author

@SidOfc SidOfc Aug 23, 2018

Choose a reason for hiding this comment

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

Aye, am already noticing that the fixed numbers are not handy to put in there. Since you also mentioned that this setting is built in and that it will always be present, might as well refer to the variable and be done with it.

" Else, return absolute width as given
function! s:pane_width()
let width = s:get_opt('width')
let width = type(width) == type('') ? width : string(width)
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

Stringification shouldn't be necessary.

Copy link
Contributor Author

@SidOfc SidOfc Aug 23, 2018

Choose a reason for hiding this comment

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

This was mainly done to support percent-like strings. I will strip it alongside the other percent-like string stuff.

let width = type(width) == type('') ? width : string(width)


if match(width, '[.%]') > -1
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

Related to my comment about only supporting float, we can check for type(width) == type(0) here.


if match(width, '[.%]') > -1
let raw = str2float(substitute(width, '%', '', 'g'))
let clamped = raw > 100.0 ? 100.0 : (raw < 0.0 ? 0.0 : raw)
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

Clamping the upper limit is probably unnecessary, as Vim handles that for us. We still have to clamp the lower limit though, since failure to do so gives us E16: Invalid range.

Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

That said, we can do let clamped = max(0.0, width).

Copy link
Contributor Author

@SidOfc SidOfc Aug 23, 2018

Choose a reason for hiding this comment

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

let clamped = max(0.0, width) will not work. e.g:

image

and second version using a list (let clamped = max([0.0, width])):

image

Hence the ugly ternary. It looks a lot simpler when only clamping one side:

let clamped = raw < 0.0 ? 0.0 : raw

As a matter of fact, I decided to inline it since it is now quite a bit shorter.

let clamped = raw > 100.0 ? 100.0 : (raw < 0.0 ? 0.0 : raw)
let min_width = exists('&winwidth') ? &winwidth : 20
let buffer_width = winwidth(bufwinnr('%'))
let result = ceil((buffer_width / 100.0) * clamped)
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

We can simplify this expression as buffer_width * clamped / 100.

doc/codi.txt Outdated
@@ -246,6 +246,11 @@ g:codi#autocmd
*g:codi#width*
g:codi#width
The width of the Codi split.
It can be a |Number|, a |Float| or a percentage string such as
`'33%'` or `'33.33%'`. The width will be clamped between `0.0`
and `100.0`. This plugin respects the |minwidth| variable if it
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

Also, shouldn't this be winwidth?

if match(width, '[.%]') > -1
let raw = str2float(substitute(width, '%', '', 'g'))
let clamped = raw > 100.0 ? 100.0 : (raw < 0.0 ? 0.0 : raw)
let min_width = exists('&winwidth') ? &winwidth : 20
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

I don't think there will be a case where winwidth is not defined, as it is a built-in option.

let buffer_width = winwidth(bufwinnr('%'))
let result = ceil((buffer_width / 100.0) * clamped)

if (buffer_width - result) < min_width
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

Can be simplified as return max(min_width, min(buffer_width - min_width, result)).

Copy link
Contributor Author

@SidOfc SidOfc Aug 23, 2018

Choose a reason for hiding this comment

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

See previous max comment. P.S. your arguments are wrong according to my :h max:

image

Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

Ah, I didn't realize Float was not a subset of Number, nor that max doesn't even work with Float - too used to Python 🙂 In that case, let's ceil result again and use the max/min functions with the arguments wrapped in a list - I think it's a bit easier to understand what's going on using those instead of the if/elseif/else. This will also allow us to eliminate the float2nr call at the end.

Copy link
Contributor Author

@SidOfc SidOfc Aug 23, 2018

Choose a reason for hiding this comment

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

If I understand you correctly, you want to turn all the floats into integers using ceil so that they can be used in min/max when wrapped in a list. This is something I thought of too but wasn't sure if the lost precision was worth it but if it is cool with you it's cool with me. It will make the code a bit cleaner and easier to understand as well 👍

I am already working on the next commit, all that remains is this part so I'll be back soon. (My internet failed twice while trying to write this comment :/)

@SidOfc
Copy link
Contributor Author

@SidOfc SidOfc commented Aug 23, 2018

@metakirby5 I've updated the examples based on your feedback.
Please see my comments relating to max functionality and let me know if anything else needs to be done of course :)

Copy link
Owner

@metakirby5 metakirby5 left a comment

Minor things, along with the max/min comment I left in the other discussion.

@@ -305,7 +327,7 @@ function! s:codi_hide()
let codi_bufnr = s:get_codi('bufnr')
if s:get_opt('autoclose') && codi_bufnr && !s:updating
" Remember width for when we respawn
call s:let_codi('width', winwidth(bufwinnr(codi_bufnr)))
call s:let_codi('width', s:pane_width())
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

The intent behind this line was to save the current window width for a buffer, allowing persistence of width when hiding/showing Codi on a per-buffer basis. We should revert this change.

doc/codi.txt Outdated
The width of the Codi split.
The width of the Codi split, either a |Number| or a |Float|.

When a |Float| is supplied, it will be treated as a percentage.
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

Let's specify this as "percentage of the current buffer width". Considering the user may already have a vertical split open, it may be alarming if they think you mean percentage of the window.

doc/codi.txt Outdated
This setting will then also respect the |winwidth| builtin which
means that each pane will be at least |winwidth| columns wide.

When a |Number| is supplied, it will be interpreted as the
Copy link
Owner

@metakirby5 metakirby5 Aug 23, 2018

Choose a reason for hiding this comment

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

We should make this the 2nd paragraph instead of the 3rd, since the default value is Number.

@SidOfc
Copy link
Contributor Author

@SidOfc SidOfc commented Aug 23, 2018

@metakirby5, I made the requested changes, the min/max modifications make the code a lot more readable actually! Unfortunately I did have to wrap the ceil in max([0, ceil(width)]) with a float2nrlike this: max([0, float2nr(ceil(width))]). This is because ceil doesn't change it into a number but returns a rounded float instead.

:echo ceil(66.66)
" => 67.0

:echo float2nr(ceil(66.66))
" => 67

Doing just a float2nr actually always floors the result.

:echo float2nr(66.66)
" => 66

Ready for additional feedback :)

Copy link
Owner

@metakirby5 metakirby5 left a comment

Looks excellent! Quite unfortunate that vimscript is so weird with it's max and min functions. Thanks for bearing with my admittedly pedantic feedback :)

endif

let buf_width = winwidth(bufwinnr('%'))
let pane_width = buf_width * max([0, float2nr(ceil(width))]) / 100
Copy link
Owner

@metakirby5 metakirby5 Aug 25, 2018

Choose a reason for hiding this comment

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

I see what you mean by lost precision now...

I think you were correct in using the ternary earlier. Changing the line to the following should allow us to keep precision.

  let pane_width = float2nr(ceil(buf_width * (width < 0.0 ? 0.0 : width) / 100))

Copy link
Owner

@metakirby5 metakirby5 Aug 25, 2018

Choose a reason for hiding this comment

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

Actually, the round function may be even better:

  let pane_width = float2nr(round(buf_width * (width < 0.0 ? 0.0 : width) / 100))

Copy link
Contributor Author

@SidOfc SidOfc Aug 25, 2018

Choose a reason for hiding this comment

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

When I get home I will start on these changes. There is no space on the train to allow for coding at the moment 😅

@SidOfc
Copy link
Contributor Author

@SidOfc SidOfc commented Aug 25, 2018

These kinds of things with min/max, I run into them myself sometimes when I try to outsmart the docs, they always pwn me hard anyways haha.

When I tested the precision loss on a 127 column wide terminal the difference seemed negligible. You seem to have found a more interesting case though, nice!

I restored precision according to your specs. I inverted the ternary since most people will provide a sane hardcoded positive float.

Think it's ready for merging?

@metakirby5
Copy link
Owner

@metakirby5 metakirby5 commented Aug 25, 2018

LGTM, thanks for the hard work!

@metakirby5 metakirby5 merged commit 74ae53a into metakirby5:master Aug 25, 2018
@SidOfc
Copy link
Contributor Author

@SidOfc SidOfc commented Aug 25, 2018

Happy to have been able to contribute to this plugin. It's going to help me out a lot when pairing 👍

It was a pleasure :)

@SidOfc SidOfc deleted the feature/percentage-width branch Aug 25, 2018
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.

None yet

2 participants