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

Unify theme support code #3743

Closed
wants to merge 4 commits into from
Closed

Conversation

apjanke
Copy link
Contributor

@apjanke apjanke commented Apr 1, 2015

Overview

This PR unifies the theme support code between the themes plugin and core oh-my-zsh by folding the themes plugin in to lib/themes-and-appearance.zsh and having oh-my-zsh.sh use its functions for the initial theme loading. It adds code for robustness, including the support for unloading themes, debugging the theme loading process, and fixing local declarations inside themes.

Fixes #3742.
Closes #3703.
Fixes #3704.
Fixes #3586.
Fixes #3585.
Fixes #8889.
Closes #3587.
Closes #3337.
Closes #3739.
Closes #3741.
Closes #3703.
Closes #2962. ("random" functionality provided via a keyword arg instead of defined theme.)
Closes #2113. ($ZSH_THEME="" will disable theming.)
Closes #4471 (which changes search logic in theme plugin; this PR effectively includes that change.)
Closes #4605 (this also removes the lstheme cd and adds looking in $ZSH_CUSTOM/themes

What this accomplishes

Behavior of themes loaded by oh-my-zsh.sh during the initialization process and the theme() function is now consistent.

Themes can be cleanly switched during a zsh session. Previously, there was no way to cleanly unload a theme, so loading a new theme using the themes plugin could end up with a mix of settings from the new theme and previously loaded themes. This adds a mechanism to "unload" a theme by resetting theme-related variables to defaults and removing theme-installed chpwd/preexec/precmd hooks.

Themes defined in $ZSH_CUSTOM are supported everywhere now, so they can be used in initial startup or theme() calls, and are included in random theme selection.

Adds new theme next and theme <n> calling forms to let you step through the list of all defined themes. Adds new theme off command for unloading the theme and reverting to ZSH default values.

Fixes leakage of local variables defined inside themes. Previously, they were sourced in the top-level shell namespace, so local declarations had no effect.

Adds "blacklist" support to exclude certain themes from being picked by random or next, as requested in #3704. (Also important for working with the theme debugging code.)

Adds optional debugging output to the theme loading process so you can evaluate themes for correctness and side effects.

Implementation

Fixes the local definitions in themes by sourcing the theme definition from inside a function call, to provide a layer of indirection. The additional function call stack frame becomes the context for local declarations.

Fixes themes that were relying on not-really-local behavior for parameters declared as local inside the theme definitions. Mostly these were of the pattern local ret_status="..."; SOME_PROMPT='${ret_status}' and were fixed by moving ${ret_status} outside the single-quotes or replacing them with double-quotes, so the parameter substitution happens at theme loading time instead of prompt evaluation time.

Fixes the lstheme() function to remove the pwd side effects, and has all code that wants to get a list of themes call lstheme instead of looking through the theme directories themselves. Now everything has a consistent view of what themes are defined. The total ordering of themes is well-defined and stable, so it can be used as a reference for loading themes by index.

The theme next command uses that lstheme ordering to allow you to iterate through the whole list of themes. This is mostly useful for debugging and development work.

The non-existent $ZSH/completions directory added to $fpath by oh-my-zsh.sh is created, as a place for the _theme completion definition to go. This avoids adding $ZSH/lib to $fpath.

The parts of the code that track hook functions and debugging state do set difference operations on arrays. To be compatible with ZSH 4.x, this is done with a custom _omz_array_setdiff function instead of using the ${foo:|bar} substitution, which is new in ZSH 5.x.

Possible conflicts

The theme and lstheme functions are now always defined when oh-my-zsh is loaded, instead of just when the themes plugin is loaded.

If any themes declare parameters as local but are actually relying on them behaving as globals, this could break those themes, because local parameters really will be local now. (That is a bug in those theme definitions, since they're misusing local.) This PR includes fixes to all the themes in the repo that were doing so.

Conflicts with #2401 since the code diverges significantly.

# variable.
#
# As a consequence of this argument syntax, themes may not be named "random", "next",
# or an integer number.
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to remember to document that somewhere in the wiki.

@ncanceill
Copy link
Contributor

I am up for testing this, can you find a few more?

@apjanke
Copy link
Contributor Author

apjanke commented Apr 8, 2015

@mcornella, maybe you'd be up for reviewing and testing this too? I don't really know any of the other OMZ maintainers since I'm new here.

@apjanke
Copy link
Contributor Author

apjanke commented Jun 10, 2015

@avit, @agnoster, any chance you'd be interested in testing this, since you've worked with some of the theme functionality?

@agnoster
Copy link
Contributor

I'm afraid my knowledge of zsh is limited to what I needed to kludge together my theme. Sounds cool though!

@mcornella
Copy link
Member

Sorry @apjanke, adding this to my list of pending PRs to test (which is getting a little bigger now 😉).

@apjanke
Copy link
Contributor Author

apjanke commented Jul 3, 2015

Note to follow up on: some users are defining themes inside subdirectories under custom/themes so Git submodules can be used. PowerLevel9K does this. This has the effect of creating themes with / in the name. Need to decide whether this is actually supported.

Both the new theme code and the old stuff will not include these "nested" themes in the theme name listing. But they will be loaded if requested specifically. Need to decide if this is actually supported, and if it is, add recursive search to the theme enumeration code.

@apjanke
Copy link
Contributor Author

apjanke commented May 7, 2016

Rebased on master to get a clean merge.

@robbyrussell
Copy link
Member

@apjanke Hey there -- can you rebase one more time? I feel like the changes in the lib/theme-and-appearance.zsh require some careful attention

@apjanke
Copy link
Contributor Author

apjanke commented Jul 19, 2017

Sure. Going to be a bit – I'm in a heavy work and travel period right now. Feel free to ping me in a couple weeks if I forget.

apjanke and others added 4 commits July 19, 2017 22:31
Rewrite theme() and lstheme() to remove side effects, add debugging,
and fix `local` inside theme definitions.
Add `theme next` for stepping through theme list, and "blacklist" support.
Modify tools/theme_chooser to use new theme functions.
Fix quoting issue in theme debugging code.
@apjanke
Copy link
Contributor Author

apjanke commented Jul 20, 2017

Rebased. I looked closely at lib/theme-and-appearance.zsh, and I think the rebase merge is good. It was kind of a pain, though. Maybe we could merge this soon to avoid further rebases?

@mcornella mcornella added Topic: completion Pull Request or issue regarding completion Area: init Issue or PR related to the initializer Area: plugin Issue or PR related to a plugin Status: conflicts Pull Request that has conflicts with the master branch labels Mar 24, 2019
mcornella added a commit to mcornella/ohmyzsh that referenced this pull request Feb 26, 2023
- Reorganize sections
- Add and clarify comments
- Clean up logic for dealing with ls color flags
- Set both $LSCOLORS and $LS_COLORS defaults
- Set zsh completion coloring before first prompt

Closes ohmyzsh#3743
Closes ohmyzsh#6353
Closes ohmyzsh#8315
Closes ohmyzsh#11091

Co-authored-by: Andrew Janke <janke@pobox.com>
Co-authored-by: Marcelo Parada <marcelo.parada@axoninsight.com>
Co-authored-by: Uy Ha <hchanuy@gmail.com>
Co-authored-by: Valentin Uveges <valentin.uveges@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Issue or PR related to core parts of the project Area: init Issue or PR related to the initializer Area: plugin Issue or PR related to a plugin Area: theme Issue or PR related to a theme Status: conflicts Pull Request that has conflicts with the master branch Topic: completion Pull Request or issue regarding completion
Projects
Archived in project
5 participants