Skip to content

fix(lualine): automatically load theme for the active colorscheme#1031

Merged
Jint-lzxy merged 8 commits intomainfrom
fix/lualine-theme
Oct 25, 2023
Merged

fix(lualine): automatically load theme for the active colorscheme#1031
Jint-lzxy merged 8 commits intomainfrom
fix/lualine-theme

Conversation

@ayamir
Copy link
Owner

@ayamir ayamir commented Oct 20, 2023

By utilizing "auto" theme as mentioned here, this PR corrects the appearance of lualine when using other colorschemes.
Of course, users using catppuccin won't be influenced.

@ayamir
fix(lualine): use 'auto' theme when using others themes.
64de25f 
Signed-off-by: ayamir <lgt986452565@gmail.com>
@ayamir ayamir changed the title fix(lualine): use 'auto' theme when using others themes. fix(lualine): use 'auto' theme when using others colorschemes. Oct 20, 2023
Jint-lzxy
Jint-lzxy reviewed Oct 20, 2023
View reviewed changes
@ayamir ayamir requested a review from Jint-lzxy October 21, 2023 09:36
@ayamir
unify variable name style.
1363a5f 
Signed-off-by: ayamir <lgt986452565@gmail.com>
Jint-lzxy
Jint-lzxy reviewed Oct 22, 2023
View reviewed changes
Copy link
Collaborator

@Jint-lzxy Jint-lzxy left a comment

Choose a reason for hiding this comment

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

I mean we can keep changes minimal like this b/c our palette will also "try it's best to generate one if there's no theme available for the currently selected colorscheme."

@ayamir
Copy link
Owner Author

ayamir commented Oct 22, 2023

图片
IMO it's better to return "auto" here b/c our code based on catppuccin may not suitable for every theme like gruvbox in the screenshot.

charliie-dev
charliie-dev approved these changes Oct 23, 2023
View reviewed changes
Copy link
Collaborator

@charliie-dev charliie-dev left a comment

Choose a reason for hiding this comment

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

TBH, I cant really tell the difference between two proposes in my own tests.
But ayamir's propose seems to use less code without changing the visual effect, so I think this PR is fine.

charliie-dev

This comment was marked as duplicate.

Sign in to view

charliie-dev

This comment was marked as duplicate.

Sign in to view

@Jint-lzxy
Copy link
Collaborator

Jint-lzxy commented Oct 23, 2023

IMO it's better to return "auto" here b/c our code based on catppuccin may not suitable for every theme like gruvbox in the screenshot.

Agree with this lol I was just wondering if lualine's implementation of the auto theme could lead to performance overheads b/c we basically only need the function to check whether there's a theme available for the current colorscheme (and if there is, load that instead of generating ours). So I only took the relevant parts from lualine/themes/auto.lua in this proposal :D

I also think maybe we can simplify our implementation a bit lol (cause the modifications to those color keys look "quite" similar). Would it be better to just use a status variable to indicate the currently active theme and tweak utils.gen_hl to generate the colors where appropriate?

cc @CharlesChiuGit @ayamir

@charliie-dev
Copy link
Collaborator

charliie-dev commented Oct 23, 2023

Aha, I see your point now. ys, I think it's better to tweak utils.gen_hl to generate colors would be a nice approach.

@ayamir
Copy link
Owner Author

ayamir commented Oct 24, 2023 •
edited
Loading

It seems that the performance overhead is insignificant when I tested the two implementations using vim-startuptime.
image
image

Jint-lzxy
Jint-lzxy reviewed Oct 24, 2023
View reviewed changes
@ayamir
Copy link
Owner Author

ayamir commented Oct 25, 2023

Using "auto" for catppuccin differs from our customized appearance. So I reverted to the initial commit status and unified code styles. It satisfied the requirement as I mentioned here.

Jint-lzxy
Jint-lzxy approved these changes Oct 25, 2023
View reviewed changes
Copy link
Collaborator

@Jint-lzxy Jint-lzxy left a comment

Choose a reason for hiding this comment

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

LGTM!

@Jint-lzxy Jint-lzxy changed the title fix(lualine): use 'auto' theme when using others colorschemes. fix(lualine): automatically load theme for the active colorscheme Oct 25, 2023
@Jint-lzxy Jint-lzxy merged commit 8670c90 into main Oct 25, 2023
@Jint-lzxy Jint-lzxy deleted the fix/lualine-theme branch October 25, 2023 12:14
ttbug pushed a commit to ttbug/nvimconf that referenced this pull request Oct 26, 2023
@ayamir @Jint-lzxy
fix(lualine): automatically load theme for the active colorscheme (ay…
f16d346 
…amir#1031)

* fix(lualine): use 'auto' theme when using others themes.

Signed-off-by: ayamir <lgt986452565@gmail.com>

* fixup!: remove `custom_theme`.

* unify variable name style.

Signed-off-by: ayamir <lgt986452565@gmail.com>

* Revert "unify variable name style."

This reverts commit 1363a5f.

* Revert "fixup!: remove `custom_theme`."

This reverts commit 55732e1.

* fixup!: unify code style.

* fixup:! handle `is_catppuccin` in `gen_hl`.

* chore: cleanup

---------

Signed-off-by: ayamir <lgt986452565@gmail.com>
Co-authored-by: Jint-lzxy <50296129+Jint-lzxy@users.noreply.github.com>
singlemancombat pushed a commit to singlemancombat/nvim-config that referenced this pull request Oct 26, 2023
@ayamir @Jint-lzxy
fix(lualine): automatically load theme for the active colorscheme (ay…
5a38bcb 
…amir#1031)

* fix(lualine): use 'auto' theme when using others themes.

Signed-off-by: ayamir <lgt986452565@gmail.com>

* fixup!: remove `custom_theme`.

* unify variable name style.

Signed-off-by: ayamir <lgt986452565@gmail.com>

* Revert "unify variable name style."

This reverts commit 1363a5f.

* Revert "fixup!: remove `custom_theme`."

This reverts commit 55732e1.

* fixup!: unify code style.

* fixup:! handle `is_catppuccin` in `gen_hl`.

* chore: cleanup

---------

Signed-off-by: ayamir <lgt986452565@gmail.com>
Co-authored-by: Jint-lzxy <50296129+Jint-lzxy@users.noreply.github.com>
@TonyWu20 TonyWu20 mentioned this pull request Oct 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@charliie-dev charliie-dev charliie-dev approved these changes

@Jint-lzxy Jint-lzxy Jint-lzxy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ayamir @Jint-lzxy @charliie-dev