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

feat: add number styling #409

Merged
merged 1 commit into from
Apr 29, 2023
Merged

feat: add number styling #409

merged 1 commit into from
Apr 29, 2023

Conversation

ttytm
Copy link
Contributor

@ttytm ttytm commented Apr 1, 2023

Thanks for the maintenance. It's nice to see the repositories active development.

This PR extends the options for buffer indices and numbers with the style options 'superscript' and 'subscript'. Current boolean values will continue to work as they are.
I did my best to follow the conventions I've seen in the repo.

lua/barbar/render.lua Outdated Show resolved Hide resolved
@ttytm ttytm requested a review from romgrk April 1, 2023 19:14
lua/barbar/render.lua Outdated Show resolved Hide resolved
Comment on lines 146 to 157
---@param num number
---@param style barbar.config.options.icons.numbers.value
local function style_number(num, style)
local digits
local num_as_str = tostring(num)
if style == 'subscript' then
digits = SUBSCRIPT_DIGITS
elseif style == 'superscript' then
digits = SUPERSCRIPT_DIGITS
else
return num_as_str
end
Copy link
Owner

Choose a reason for hiding this comment

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

You know what, pass those digits as an argument to style_number and we can keep the string constants. E.g. style_number(num, style, digits). The digits would be either a table or nil. The string comparison happens once per render, I'm cool with that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good call, that can be resolved outside the loop.

Copy link
Contributor Author

@ttytm ttytm Apr 1, 2023

Choose a reason for hiding this comment

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

Wouldn't this give us two of those?

buffer_index.text = style_number(
  i,
  icons_option.buffer_index,
  icons_option.buffer_index == 'superscript' and SUPERSCRIPT_DIGITS
    or icons_option.buffer_index == 'subscript' and SUBSCRIPT_DIGITS
    or nil) .. ' '

Partial reverting the full encapsulation I did in the second commit might be more elegant. Only calling style_number() when superscript or subscript is specified but not passing digits as arg.

buffer_number.text = ((icons_option.buffer_number == "superscript" or icons_option.buffer_number == "subscript")
        and style_number(bufnr, icons_option.buffer_number)
        or tostring(bufnr)) .. ' '

What you think?

edit: Ah I see, it would still take two comparisons then!
If you like a helper function for the one comparison let me know.

Copy link
Owner

Choose a reason for hiding this comment

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

You'd assign digits outside of the loop.

local digits = nil
if opt == 'super' then digits = SUPER end
if opt == 'sub' then digits = SUB end

for ...
  text = style_number(..., digits)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, didn't realized the outer loop. Thought the loop that needed to be solved was solved with iron-e suggestions. Thanks!

Copy link
Collaborator

@Iron-E Iron-E Apr 8, 2023

Choose a reason for hiding this comment

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

No matter what, certain options have to be resolved in the loop. That commit just increased the perf of doing that.

If @romgrk is okay with it, you can implement the with style_numbers resolving digits internally and do a benchmark to see what the impact is.

You can use this code to do that

If the impact is significant, we can just have a number_style option that can be resolved externally. Less configurability, but makes things faster.

Copy link
Contributor Author

@ttytm ttytm Apr 8, 2023

Choose a reason for hiding this comment

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

Thanks for the snip! On my box it just had little impact during normal usage while running style_number in a loop of 1000 where we currently set buffer_index.text. Highest 0.0036389999999997s. Lowest 0.00029000000000146s.

Metrics comparing function calls:
10.000 loops to just get a number from index.
0.0017560000000003s
10.000 loops of style_number, digits passed as arg
0.0039189999999996s
10.000 loops of style_number, digits resolved in a condition inside the function (like the current code does)
0.0038720000000017
10.000 loops of style_number(internal condition), returning a "non-styled" number.
0.0017619999999953s

So there doesn't seem to be any significance resolving digits in style_number or passing them as arg.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing that. We can let romgrk decide from here 🙂

Copy link
Owner

Choose a reason for hiding this comment

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

Let's pick whatever option is more simple, doesn't seem to have a significant performance impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the state now that includes the suggestion after the initial PR is fairly concise and readable 🙂:+1:

@Iron-E Iron-E added the request New feature or request label Apr 2, 2023
@ttytm ttytm mentioned this pull request Apr 7, 2023
@Iron-E Iron-E added this to the 1.6 milestone Apr 9, 2023
@Iron-E
Copy link
Collaborator

Iron-E commented Apr 29, 2023

I can solve the merge conflicts, don't worry

@Iron-E Iron-E merged commit 8523e5d into romgrk:master Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants