feat: add smart component truncation #110

famiu · 2021-09-26T04:06:58Z

Closes #96

Seems to be working fine. Though I'd like @ram02z to review the code (and try to make it simpler if possible). I'd also like it to be well-tested before merge.

famiu · 2021-09-26T04:51:05Z

After doing some benchmarks I can see that it makes the statusline generation time around 15% higher than before

famiu · 2021-09-26T08:43:41Z

I wonder what happens when a window is resized causing a component in an inactive window to require truncation or allowing a component in an inactive window to not be truncated. There's no WinResized autocommand so it's impossible to update inactive statuslines in those cases. The next best thing is VimResized but the windows can still be resized without triggering VimResized

ram02z · 2021-09-26T13:15:31Z

I wonder what happens when a window is resized causing a component in an inactive window to require truncation or allowing a component in an inactive window to not be truncated. There's no WinResized autocommand so it's impossible to update inactive statuslines in those cases. The next best thing is VimResized but the windows can still be resized without triggering VimResized

You are right, this is a limitation of neovim since there is no WinResized autocommand and the only linked PR has been stale.

I quickly tried the file_info example you added to the README and the switching between the normal provider and short provider works well, but it doesn't seem to hide the short provider if it is still too big (took a quick look at the changes and I don't think it was implemented). I don't think it's that big of a deal though, since the user could use the enabled key for that.

I currently don't have much time to thoroughly test and review this properly, however, I will do as soon as I can.

famiu · 2021-09-26T13:35:14Z

I quickly tried the file_info example you added to the README and the switching between the normal provider and short provider works well, but it doesn't seem to hide the short provider if it is still too big (took a quick look at the changes and I don't think it was implemented). I don't think it's that big of a deal though, since the user could use the enabled key for that.

Question, should it start removing components only after all components with short_provider are using their short_provider regardless of whether the component with short_provider has higher priority, or should it remove the component if it doesn't have a short_provider when truncating components in order of priority?

ram02z · 2021-09-26T13:47:10Z

Question, should it start removing components only after all components with short_provider are using their short_provider regardless of whether the component with short_provider has higher priority, or should it remove the component if it doesn't have a short_provider when truncating components in order of priority?

Not necessarily, we should only care about the length and priority of the providers. Same reason why I think it is up to the user to ensure that the short provider is actually shorter than the normal provider. I think that components that don't have a short provider should be hidden when truncated in order of priority and when they do have a short provider, feline should first attempt to fallback to the short provider, but if the short provider's length still surpasses the section limit, it should be hidden. Perhaps, hiding the components should be an opt-in feature though by introducing a key to feline's setup or maybe to each component; whatever you think is better. I am suggesting that it should be opt-in because some users may not want their component's hidden unless they explicitly define that behaviour themselves.

famiu · 2021-09-26T13:54:58Z

Question, should it start removing components only after all components with short_provider are using their short_provider regardless of whether the component with short_provider has higher priority, or should it remove the component if it doesn't have a short_provider when truncating components in order of priority?

Not necessarily, we should only care about the length and priority of the providers. Same reason why I think it is up to the user to ensure that the short provider is actually shorter than the normal provider. I think that components that don't have a short provider should be hidden when truncated in order of priority and when they do have a short provider, feline should first attempt to fallback to the short provider, but if the short provider's length still surpasses the section limit, it should be hidden. Perhaps, hiding the components should be an opt-in feature though by introducing a key to feline's setup or maybe to each component; whatever you think is better. I am suggesting that it should be opt-in because some users may not want their component's hidden unless they explicitly define that behaviour themselves.

I'm going to add an no_hide option then

famiu · 2021-09-26T13:59:12Z

Question, should it start removing components only after all components with short_provider are using their short_provider regardless of whether the component with short_provider has higher priority, or should it remove the component if it doesn't have a short_provider when truncating components in order of priority?

Not necessarily, we should only care about the length and priority of the providers. Same reason why I think it is up to the user to ensure that the short provider is actually shorter than the normal provider. I think that components that don't have a short provider should be hidden when truncated in order of priority and when they do have a short provider, feline should first attempt to fallback to the short provider, but if the short provider's length still surpasses the section limit, it should be hidden. Perhaps, hiding the components should be an opt-in feature though by introducing a key to feline's setup or maybe to each component; whatever you think is better. I am suggesting that it should be opt-in because some users may not want their component's hidden unless they explicitly define that behaviour themselves.

I'm going to add an no_hide option then

Actually, thinking more about it, I think it'd just be better if components aren't hidden at all, best to leave that to the user

ram02z · 2021-09-26T14:05:25Z

Actually, thinking more about it, I think it'd just be better if components aren't hidden at all, best to leave that to the user

I agree with that as well, which I suggested it should be opt-in behaviour (disabled by default). It might as well get implemented since it will make use of feline's priority system, so it would be redundant if the user tried to replicate that behaviour in their own config.

famiu · 2021-09-26T14:24:28Z

Actually, thinking more about it, I think it'd just be better if components aren't hidden at all, best to leave that to the user

I agree with that as well, which I suggested it should be opt-in behaviour (disabled by default). It might as well get implemented since it will make use of feline's priority system, so it would be redundant if the user tried to replicate that behaviour in their own config.

In theory, setting short_provider = '' should have the same effect, though separators and icons with always_visible set to true can still appear in that case.

But if we do make it an opt-in feature either way, I wonder if it'd be better to have one setting for the entire statusline through setup or have it individually for each component.

ram02z · 2021-09-26T14:28:22Z

But if we do make it an opt-in feature either way, I wonder if it'd be better to have one setting for the entire statusline through setup or have it individually for each component.

If the implementation isn't complicated, per component customisation is preferred imo.

famiu · 2021-09-26T15:11:48Z

So I finally added the feature to hide components entirely. Again, this seems to be working fine.

But even though it's a cool feature and all, I can't say I'm a fan of the performance degrade this feature brings, not to mention the increased complexity of the code.

famiu · 2021-09-26T15:18:42Z

So I finally added the feature to hide components entirely. Again, this seems to be working fine.

But even though it's a cool feature and all, I can't say I'm a fan of the performance degrade this feature brings, not to mention the increased complexity of the code.

After doing some benchmarks again though it seems that the performance downgrade is not as bad as I had first seen, which is relieving. The code could still use some simplifying though, except I don't really have much of an idea on how to approach that

famiu · 2021-09-26T17:13:25Z

Simplified the code a little bit, not sure how readable it is now though. @ram02z when you have time I'd like to know if you have trouble understanding the code

ram02z

LGTM. Once neovim releases a WinResized autocommand most of the truncation logic in generate_statusline can be moved to a function triggered by it. I think you should definitely CC the feline users that expressed interest in the truncation to also test this branch, but from what I have tested so far, it seems to working as intended.

lua/feline/generator.lua

ram02z · 2021-09-26T21:44:58Z

lua/feline/generator.lua

+                elseif component.truncate_hide then
+                    statusline_length = statusline_length - parsed_component.len
+                    parsed_sections[indices[1]][indices[2]] = {str = '', len = 0}


Suggested change

elseif component.truncate_hide then

statusline_length = statusline_length - parsed_component.len

parsed_sections[indices[1]][indices[2]] = {str = '', len = 0}

This isn't needed since the check happens after anyways. Also causes an issue when you have a component with no short provider, high priority and truncate_hide set to true. It will still get hidden even though it has a priority higher than a component with a short provider.

That isn't supposed to happen because components are always hidden in order of priority. Also the check that happens afterwards hides all components with truncate_hide set to true, even if they have a short provider, which should only happen if all other components are using their short_provider

Well, for one there is no point of checking the truncate_hide value for components without a short_provider because they would have been hidden by the code I suggested to be removed anyway. Anyway, you should be able to reproduce the bug with this config. Just keep making the active smaller and you will see that the higher priority component is still hidden first.

Well, for one there is no point of checking the truncate_hide value for components without a short_provider because they would have been hidden by the code I suggested to be removed anyway. Anyway, you should be able to reproduce the bug with this config. Just keep making the active smaller and you will see that the higher priority component is still hidden first.

Ah, I see what you mean now. That behavior was intended actually, but thinking more about it, it makes more sense to remove it

USAGE.md

ram02z · 2021-09-26T22:16:45Z

Simplified the code a little bit, not sure how readable it is now though. @ram02z when you have time I'd like to know if you have trouble understanding the code

The code itself is not difficult to understand and the comments are useful too. The only issue I think may that debugging truncation issues may be difficult due to how tables like parsed_sections can get abit convoluted.

famiu · 2021-09-27T02:20:00Z

LGTM. Once neovim releases a WinResized autocommand most of the truncation logic in generate_statusline can be moved to a function triggered by it.

Actually, I don't think so, since it needs to modify the generated statusline string directly.

famiu · 2021-09-27T02:21:07Z

The code itself is not difficult to understand and the comments are useful too. The only issue I think may that debugging truncation issues may be difficult due to how tables like parsed_sections can get abit convoluted.

Any suggestions to fix that?

ram02z · 2021-09-27T08:37:18Z

LGTM. Once neovim releases a WinResized autocommand most of the truncation logic in generate_statusline can be moved to a function triggered by it.

Actually, I don't think so, since it needs to modify the generated statusline string directly.

True, but that autocommand at the bare minmum will allow for truncation to be visible for inactive windows.

The code itself is not difficult to understand and the comments are useful too. The only issue I think may that debugging truncation issues may be difficult due to how tables like parsed_sections can get abit convoluted.

Any suggestions to fix that?

Honestly, not sure because the sections table in itself needs two indices to access a specific component. I don't even think named components would help.

famiu · 2021-09-27T09:29:06Z

LGTM. Once neovim releases a WinResized autocommand most of the truncation logic in generate_statusline can be moved to a function triggered by it.

Actually, I don't think so, since it needs to modify the generated statusline string directly.

True, but that autocommand at the bare minmum will allow for truncation to be visible for inactive windows.

The code itself is not difficult to understand and the comments are useful too. The only issue I think may that debugging truncation issues may be difficult due to how tables like parsed_sections can get abit convoluted.

Any suggestions to fix that?

Honestly, not sure because the sections table in itself needs two indices to access a specific component. I don't even think named components would help.

Yeah, that's an issue. If only Lua allowed taking variable references...

famiu · 2021-09-28T08:49:20Z

This doesn't seem to work anymore after I rebased it against the develop branch, not sure why

famiu · 2021-09-28T09:10:43Z

This doesn't seem to work anymore after I rebased it against the develop branch, not sure why

This seems to be caused by #120

famiu · 2021-09-28T09:47:40Z

Removing the vim.schedule wrapper does seem to fix both #119 and also allow truncation to work, but then it causes the NvimTree problem...

famiu · 2021-09-28T09:53:22Z

Removing the vim.schedule wrapper does seem to fix both #119 and also allow truncation to work, but then it causes the NvimTree problem...

Honestly, at this point I'd suggest removing the vim.schedule() part and ask @xeluxee to open an issue on NvimTree and ask NvimTree to add an autocmd for when it's done doing its work, so @xeluxee can then add that to their update triggers. As I see it now, vim.schedule() is kind of unpredictable and a very hacky "fix", so maybe it's better to remove it altogether.

ram02z · 2021-09-28T10:52:40Z

Honestly, at this point I'd suggest removing the vim.schedule() part and ask @xeluxee to open an issue on NvimTree and ask NvimTree to add an autocmd for when it's done doing its work, so @xeluxee can then add that to their update triggers. As I see it now, vim.schedule() is kind of unpredictable and a very hacky "fix", so maybe it's better to remove it altogether.

I don't personally have a problem when vim.schedule is not used to update the windows, and NerdTree is the only plugin that we noticed misbehaved without the workaround. I am wondering if #120 is still required once vim.schedule is removed, feline fallbacks to the global statusline anyway.

famiu · 2021-09-28T13:28:21Z

I am wondering if #120 is still required once vim.schedule is removed, feline fallbacks to the global statusline anyway.

It's not

famiu · 2021-09-28T16:50:00Z

Since this seems to be working fine I'm going to merge this now. Make a new issue if this causes any problems

famiu · 2021-09-30T12:55:17Z

@ram02z it seems that truncation doesn't work correctly still because in Vim's statusline, in order to have a literal % character in it, you have to escape it with another %, which is what the line_percentage provider uses. As a result it gives an incorrect length for it. Not sure how to fix it

famiu · 2021-10-01T05:38:54Z

I might have to end up removing truncation. It still seems to have a lot of flaws (such as the one mentioned above), and it's not really very reliable, plus it seems to have a massive performance hit as well and it's kind of impossible to make it zero-cost.

famiu · 2021-10-01T05:44:30Z

I've opened an issue in the Neovim repo that'd make the process much easier: neovim/neovim#15849

But until that's implemented, I'm strongly in favor of removing truncation. No truncation is better than half-assed truncation imo.

famiu · 2021-10-02T02:41:01Z

I'm going to remove truncation for now until either the feature gets implemented in Neovim or until I can find a more performant and reliable way to get the statusline length

shadmansaleh · 2021-10-02T15:47:29Z

You can use build_stl_str_hl function through lua ffi to get expanded result.

Even if neovim implements expanding stl expressions it'll still be costly . As it can't really be lighter then what neovim currently does while drawing statusline.

famiu · 2021-10-02T16:35:32Z

You can use build_stl_str_hl function through lua ffi to get expanded result.

Oh? I'm not sure how to do that though

Even if neovim implements expanding stl expressions it'll still be costly . As it can't really be lighter then what neovim currently does while drawing statusline.

But at least then it'll be more accurate

shadmansaleh · 2021-10-02T16:46:21Z

Oh? I'm not sure how to do that though

It'll be something like this

local ffi = require('ffi')

local M = {}

ffi.cdef [[
typedef unsigned char char_u;
typedef struct window_S win_T;
extern win_T   *curwin;
typedef struct {} stl_hlrec_t;
typedef struct {} StlClickDefinition;
typedef struct {} StlClickRecord;
int build_stl_str_hl(
    win_T *wp,
    char_u *out,
    size_t outlen,
    char_u *fmt,
    int use_sandbox,
    char_u fillchar,
    int maxwidth,
    stl_hlrec_t **hltab,
    StlClickRecord **tabtab
);
]]

function M.expand_stl_expr(stl_fmt)
    local stlbuf = ffi.new('char_u [?]', 256)
    local fmt = ffi.cast('char_u *', stl_fmt)
    ffi.C.build_stl_str_hl(
        ffi.C.curwin,
        stlbuf,
        256,
        fmt,
        0,
        ffi.cast('char_u', 0x20),
        vim.fn.winwidth(0),
        nil,
        nil
    )
    return ffi.string(stlbuf)
end

return M

Note: Untested code .

famiu · 2021-10-02T17:04:22Z

Oh? I'm not sure how to do that though

It'll be something like this

local ffi = require('ffi')

local M = {}

ffi.cdef [[
typedef unsigned char char_u;
typedef struct window_S win_T;
extern win_T   *curwin;
typedef struct {} stl_hlrec_t;
typedef struct {} StlClickDefinition;
typedef struct {} StlClickRecord;
int build_stl_str_hl(
    win_T *wp,
    char_u *out,
    size_t outlen,
    char_u *fmt,
    int use_sandbox,
    char_u fillchar,
    int maxwidth,
    stl_hlrec_t **hltab,
    StlClickRecord **tabtab
);
]]

function M.expand_stl_expr(stl_fmt)
    local stlbuf = ffi.new('char_u [?]', 256)
    local fmt = ffi.cast('char_u *', stl_fmt)
    ffi.C.build_stl_str_hl(
        ffi.C.curwin,
        stlbuf,
        256,
        fmt,
        0,
        ffi.cast('char_u', 0x20),
        vim.fn.winwidth(0),
        nil,
        nil
    )
    return ffi.string(stlbuf)
end

return M

Note: Untested code .

Oh wow, this actually seems to work pretty well. Thanks a lot for this!

famiu force-pushed the feat/add-truncation branch from fea12d3 to 4173f99 Compare September 26, 2021 14:00

famiu force-pushed the feat/add-truncation branch from 8aa26e9 to 41c3989 Compare September 26, 2021 17:44

ram02z suggested changes Sep 26, 2021

View reviewed changes

famiu force-pushed the feat/add-truncation branch from ff7fa32 to 561b6ce Compare September 28, 2021 08:48

famiu mentioned this pull request Sep 28, 2021

[Bug] Inactive statusline flashing when changing window focus #119

Closed

feat: add smart component truncation

a7ae973

fix error when the active or inactive table is empty

dd7261b

famiu force-pushed the feat/add-truncation branch from 1b632c7 to dd7261b Compare September 28, 2021 14:16

famiu merged commit f8fb9c2 into develop Sep 28, 2021

famiu deleted the feat/add-truncation branch September 28, 2021 16:50

	elseif component.truncate_hide then
	statusline_length = statusline_length - parsed_component.len
	parsed_sections[indices[1]][indices[2]] = {str = '', len = 0}

feat: add smart component truncation #110

feat: add smart component truncation #110

Conversation

famiu commented Sep 26, 2021

famiu commented Sep 26, 2021

famiu commented Sep 26, 2021

ram02z commented Sep 26, 2021

famiu commented Sep 26, 2021

ram02z commented Sep 26, 2021 • edited Loading

famiu commented Sep 26, 2021

famiu commented Sep 26, 2021

ram02z commented Sep 26, 2021

famiu commented Sep 26, 2021 • edited Loading

ram02z commented Sep 26, 2021

famiu commented Sep 26, 2021

famiu commented Sep 26, 2021

famiu commented Sep 26, 2021

ram02z left a comment • edited Loading

Choose a reason for hiding this comment

ram02z Sep 26, 2021

Choose a reason for hiding this comment

famiu Sep 27, 2021

Choose a reason for hiding this comment

ram02z Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

famiu Sep 27, 2021

Choose a reason for hiding this comment

ram02z commented Sep 26, 2021

famiu commented Sep 27, 2021

famiu commented Sep 27, 2021

ram02z commented Sep 27, 2021

famiu commented Sep 27, 2021

famiu commented Sep 28, 2021

famiu commented Sep 28, 2021 • edited Loading

famiu commented Sep 28, 2021 • edited Loading

famiu commented Sep 28, 2021

ram02z commented Sep 28, 2021 • edited Loading

famiu commented Sep 28, 2021

famiu commented Sep 28, 2021

famiu commented Sep 30, 2021

famiu commented Oct 1, 2021 • edited Loading

famiu commented Oct 1, 2021

famiu commented Oct 2, 2021

shadmansaleh commented Oct 2, 2021 • edited Loading

famiu commented Oct 2, 2021

shadmansaleh commented Oct 2, 2021 • edited Loading

famiu commented Oct 2, 2021

ram02z commented Sep 26, 2021 •

edited

Loading

famiu commented Sep 26, 2021 •

edited

Loading

ram02z left a comment •

edited

Loading

ram02z Sep 27, 2021 •

edited

Loading

famiu commented Sep 28, 2021 •

edited

Loading

famiu commented Sep 28, 2021 •

edited

Loading

ram02z commented Sep 28, 2021 •

edited

Loading

famiu commented Oct 1, 2021 •

edited

Loading

shadmansaleh commented Oct 2, 2021 •

edited

Loading

shadmansaleh commented Oct 2, 2021 •

edited

Loading