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: themes support +2 new themes, blue and gold #550

Closed
wants to merge 3 commits into from

Conversation

psprint
Copy link
Contributor

@psprint psprint commented Jul 29, 2023

Description

I think that the theming potential behind +zinit-message should be used. All styles like {error}, {warn} (or the new {w} that includes Warning: prefix – cool idea πŸ˜„), {pname} have their corresponding keys in ZINIT hash. I think that exporting the tabular hash data (2 columns with pairs of key/value) to a separate file is a good idea also from code cleanliness perspective.

Related Issue(s)

I'm refreshing an older PR #445, done before the color data has been refactored by @vladdoster ,

Motivation and Context

The +zinit-message function has been created with theming in mind from the start. It only felt that the styles database should first grow a little before another themes would be added. Now it seems complete, so it's a good moment of adding some themes.

The patch is very simple – it only moves the ZINIT+=( … … ) assignment to a separate file share/themes/default.zsh and then is duplicated to blue.zsh and gold.zsh and then the copies are altered to different styling. Nothing can broke…

Usage examples

ZITHEME=gold /bin/zsh
ZITHEME=blue /bin/zsh
ZITHEME=default /bin/zsh

Default theme – no changes:

Plugin installation:

2023-07-29-171944_1899x240_scrot

Wrong ice error:

2023-07-29-171909_1893x234_scrot

New "Blue" theme:

Plugin installation:

2023-07-29-172042_1905x241_scrot

Wrong ice error:

2023-07-29-172013_1902x229_scrot

New "Gold" theme:

Plugin installation:

2023-07-29-172128_1900x257_scrot

Wrong ice error:

2023-07-29-172105_1905x228_scrot

How Has This Been Tested?

Like the Usage examples section.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Bug fix (non-breaking change which fixes an issue)
  • Documentation change
  • New feature (non-breaking change which adds functionality)

Checklist:

  • All new and existing tests passed.
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.

@psprint
Copy link
Contributor Author

psprint commented Jul 30, 2023

@vladdoster @pschmitt @alichtman: ping? are you guys on vacation? ;)

@psprint psprint mentioned this pull request Jul 30, 2023
7 tasks
@psprint
Copy link
Contributor Author

psprint commented Aug 11, 2023

@alichtman: you seem to be the only active maintainer. Could you merge this straightforward, harmless patch?

@alichtman
Copy link
Member

I'd ping @vladdoster. I haven't merged any real code to this ever, really. Don't have time / bandwidth to be a "good" maintainer

@psprint
Copy link
Contributor Author

psprint commented Aug 13, 2023

Ping @vladdoster then. The patch is harmless, could you merge?

@psprint
Copy link
Contributor Author

psprint commented Aug 14, 2023

@vladdoster: Hi. What holds you before merging this PR?

@vladdoster
Copy link
Member

vladdoster commented Aug 15, 2023

@psprint,

This is cool, but I'd like to see some of the available col-* options removed due to lack of use.

Has someone asked for the ability to theme zinit logging? It seems unnecessary...


FYI, I have not been online for last two months for personal reasons. I should be more active in the future.

Screenshot 2023-08-15 at 00 42 26

@psprint
Copy link
Contributor Author

psprint commented Aug 15, 2023

I've removed the 11 unused entries from themes via this script:

for col in `grep -o col-[[:alnum:]]* share/themes/blue.zsh `; do
if ! \ag ${col##col-}\} zinit*.zsh &>/dev/null; then print missentry $col;
    read -q && \
        if sed -r -i -e "s/[[:space:]]+${col}[[:space:]]+\\\$'[^']*'//g" share/themes/*.zsh
         then
              print replaced
         fi
fi
done

It's output is:

missentry col-quo
yreplaced
missentry col-quos
yreplaced
missentry col-aps
yreplaced
missentry col-tab
yreplaced
missentry col-bapo
yreplaced
missentry col-nst
bmissentry col-term
yreplaced
missentry col-baps
yreplaced
missentry col-nu
nmissentry col-bar
nmissentry col-bcmd
yreplaced
missentry col-txt
yreplaced
missentry col-bspc
missentry col-uname
nmissentry col-uninst
yreplaced
missentry col-failure
yreplaced
missentry col-ndsh
missentry col-uname

I think that the theming is a good idea because:

  • it allows for light/dark setups; for example, I'm using white terminal background and blue theme is much more readable,
  • no requests for this feature doesn't mean it isn't wanted, it's just that users didn't thought that such a detailed approach to messaging is possible to expect from devs,
  • it is a cleanup/refactor of the code at the same time – it moves the tabular data out to separate files,
  • `it's very easy change.

@psprint
Copy link
Contributor Author

psprint commented Aug 17, 2023

@vladdoster: I've did what you requested and all CI checks pass (except for docs, which I cannot generate because no docker), so could you merge?

@psprint
Copy link
Contributor Author

psprint commented Aug 19, 2023

Why isn't this PR yet merged? Is it because of doubts of usability? If yes, then I tell you that once it's merged, you'll never look back on the old, unseparated (tabular data in zinit.zsh) approach… PLEASE MERGE there isn't anything to dispute about more…

@psprint
Copy link
Contributor Author

psprint commented Aug 19, 2023

@vladdoster: ping above comment

@psprint
Copy link
Contributor Author

psprint commented Aug 21, 2023

@vladdoster: Could we please stop playing in a mouse and a cat, and merge this straightforward, harmless and cleanup patch?

@psprint
Copy link
Contributor Author

psprint commented Aug 21, 2023

@vladdoster: We could easily add support for this neon theme with the pr merged:

https://www.reddit.com/r/zsh/comments/15rj8rq

@psprint
Copy link
Contributor Author

psprint commented Aug 22, 2023

@vladdoster you seem active today. Why not merge this straightforward, harmless, cleanup patch? What is the reason?

@vladdoster vladdoster closed this Jan 14, 2024
@zdharma-continuum zdharma-continuum locked as resolved and limited conversation to collaborators Jan 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants