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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Oct 25, 2023
Merged

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

merged 8 commits into from
Oct 25, 2023

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 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
@ayamir ayamir requested a review from Jint-lzxy October 21, 2023 09:36
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.

CharlesChiuGit
CharlesChiuGit approved these changes Oct 23, 2023
View reviewed changes
Copy link
Collaborator

@CharlesChiuGit CharlesChiuGit 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.

CharlesChiuGit

This comment was marked as duplicate.

Sign in to view

CharlesChiuGit

This comment was marked as duplicate.

Sign in to view

@Jint-lzxy
Copy link
Collaborator

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

@CharlesChiuGit
Copy link
Collaborator

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
lua/modules/configs/ui/lualine.lua Outdated Show resolved Hide resolved
lua/modules/configs/ui/lualine.lua Outdated Show resolved Hide resolved
@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 <[email protected]>

* fixup!: remove `custom_theme`.

* unify variable name style.

Signed-off-by: ayamir <[email protected]>

* 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 <[email protected]>
Co-authored-by: Jint-lzxy <[email protected]>
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 <[email protected]>

* fixup!: remove `custom_theme`.

* unify variable name style.

Signed-off-by: ayamir <[email protected]>

* 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 <[email protected]>
Co-authored-by: Jint-lzxy <[email protected]>
@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

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

@CharlesChiuGit CharlesChiuGit CharlesChiuGit 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 @CharlesChiuGit