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

[Proposal] New theme definition spec #2297

Open
cyberglot opened this issue Mar 16, 2023 · 35 comments
Open

[Proposal] New theme definition spec #2297

cyberglot opened this issue Mar 16, 2023 · 35 comments
Labels
help wanted Extra attention is needed

Comments

@cyberglot
Copy link

As pointed out in issue #2160, the theme definition spec doesn't follow any rules besides “green is the main colour for some reason”. When scrolling through the theme gallery page, I thought the same image was mistakenly being rendered for all themes. But no, they're all green-biased.

Here's some pictures of what I had to do to restore the dracula theme vibes:
Screenshot 2023-03-16 at 11 26 17
Screenshot 2023-03-16 at 11 42 52
(It's important to notice that the current dracula theme absolutely follows a coherent implementation given zellij's interface.)

While it works, it's highly confusing and frustrating, and I believe the dev experience here could be vastly improved. For single file configurations or scripts, I use helix, and I've just noticed that they have a pretty good spec for themes:

# User Interface
# --------------
"ui.background" = { fg = "text", bg = "base" }

"ui.linenr" = { fg = "surface1" }
"ui.linenr.selected" = { fg = "lavender" }

"ui.statusline" = { fg = "text", bg = "mantle" }
"ui.statusline.inactive" = { fg = "surface2", bg = "mantle" }
"ui.statusline.normal" = { fg = "base", bg = "lavender", modifiers = ["bold"] }
"ui.statusline.insert" = { fg = "base", bg = "green", modifiers = ["bold"]  }
"ui.statusline.select" = { fg = "base", bg = "flamingo", modifiers = ["bold"]  }

"ui.popup" = { fg = "text", bg = "surface0" }
"ui.window" = { fg = "crust" }
"ui.help" = { fg = "overlay2", bg = "surface0" }

"ui.bufferline" = { fg = "surface1", bg = "mantle" }
"ui.bufferline.active" = { fg = "text", bg = "base", modifiers = ["bold", "italic"] }
"ui.bufferline.background" = { bg = "surface0" }

"ui.text" = "text" 
"ui.text.focus" = { fg = "text", bg = "surface0", modifiers = ["bold"] }

"ui.virtual" = "overlay0"
"ui.virtual.ruler" = { bg = "surface0" }
"ui.virtual.indent-guide" = "surface0"

"ui.selection" = { bg = "surface1" }

"ui.cursor" = { fg = "base", bg = "secondary_cursor" }
"ui.cursor.primary" = { fg = "base", bg = "rosewater" }
"ui.cursor.match" = { fg = "peach", modifiers = ["bold"] }

"ui.cursorline.primary" = { bg = "cursorline" }

"ui.highlight" = { bg = "surface1", modifiers = ["bold"] }

"ui.menu" = { fg = "overlay2", bg = "surface0" }
"ui.menu.selected" = { fg = "text", bg = "surface1", modifiers = ["bold"] }

"diagnostic.error" = { underline = { color = "red", style = "curl" } }
"diagnostic.warning" = { underline = { color = "yellow", style = "curl" } }
"diagnostic.info" = { underline = { color = "sky", style = "curl" } }
"diagnostic.hint" = { underline = { color = "teal", style = "curl" } }

error = "red"
warning = "yellow"
info = "sky"
hint = "teal"

[palette]
# catppuccin palette colors
rosewater = "#f5e0dc"
flamingo = "#f2cdcd"
pink = "#f5c2e7"
mauve = "#cba6f7"
red = "#f38ba8"
maroon = "#eba0ac"
peach = "#fab387"
yellow = "#f9e2af"
green = "#a6e3a1"
teal = "#94e2d5"
sky = "#89dceb"
sapphire = "#74c7ec"
blue = "#89b4fa"
lavender = "#b4befe"

text = "#cdd6f4"
subtext1 = "#bac2de"
subtext0 = "#a6adc8"
overlay2 = "#9399b2"
overlay1 = "#7f849c"
overlay0 = "#6c7086"
surface2 = "#585b70"
surface1 = "#45475a"
surface0 = "#313244"

base = "#1e1e2e"
mantle = "#181825"
crust = "#11111b"

# derived colors by blending existing palette colors
cursorline = "#2a2b3c"
secondary_cursor = "#b5a6a8"

(code stolen from catppuccin/helix)

I believe it could be greatly improved by stealing helix's interface. I'd be happy to write down a spec that works for zellij's current UI and implement it if the authors/maintainers are happy with my suggestion.

@imsnif
Copy link
Member

imsnif commented Mar 17, 2023

I've recently been convinced that we should indeed devote more attention to this, and I'm happy to hear you'd like to take this up.

My initial concern was that the UI of Zellij is very much in flux still. We'll likely be adding things to it in the future (just recently we added the swap layouts indicator on the bottom right of the screen, for example) and it might even change entirely. I would like to be able to have a spec that is coherent on the one hand as you mentioned, but also relatively future-proof (as is the case now).

My suggestion for an approach here would be to provide configuration parameters for Zellij's graphical language rather than its graphical elements. An example would be unselected_ribbon_background_color rather than tab_background_color. What do you think? Or do you have a different idea?

@vivax3794
Copy link

what about some sort of hierarchical structure?
like for the selected tab names for example might look for tab_name_selected, then tab_name, status_line_text, text, black (for example, whatever makes the most sense)

so themes would provide colors for the generic stuff first and work their way down (if needed) and new features would just look correct with existing themes (most of the time)

@imsnif
Copy link
Member

imsnif commented Apr 15, 2023

what about some sort of hierarchical structure? like for the selected tab names for example might look for tab_name_selected, then tab_name, status_line_text, text, black (for example, whatever makes the most sense)

so themes would provide colors for the generic stuff first and work their way down (if needed) and new features would just look correct with existing themes (most of the time)

One of the things I'd like to do with the plugin systems is to allow plugin authors access to the Zellij UI elements. Meaning they would be able to render a selected_ribbon or an unselected_ribbon with text, an on-click action, etc. (other examples are rendering a frame around some text or image, rendering keybindings and having them consistent with how Zellij renders them, etc.)

For this reason I'd rather go with ribbons, frames, key, key_modifier, etc. I'm happy to have these be hierarchical themselves - but I'd rather start here and then drill down into more specific things like "tabs" (in case coloring just the UI elements won't be specific enough for some users) if the need arises. I hope this makes sense @vivax3794 ?

Otherwise - this issue is basically waiting for someone to pick it up. This would involve coming up with a spec for these requirements (mostly involving mapping the UI elements we currently have and their different states - eg. selected/unselected/etc.) and then implementing it.

I would be happy to help out in the process as much as I can.

@vivax3794
Copy link

@imsnif That makes sense, and i think UI elements should cover most cases.

Because the current system of colors is really bad imo, for a few reasons; it is hard to know what each color is used for, and you can't change more specific stuff.

My interest in this issue originally comes from wanting to set a different text color for the tab names then what the background is, which both use black; which is also another issue, themes might change what each color is, like i set my "black" to white.

@kristian-clausal
Copy link

kristian-clausal commented Jul 4, 2023

gruvbox-light-modified {
        bg 0 0 0
        fg 0 0 0
        white 0 0 0
        red 0 0 0
        green 0 0 0
        yellow 0 0 0
        blue 0 0 0
        magenta 0 0 0
        cyan 0 0 0
        black 0 0 0
        orange 0 0 0
    }

I just spent half an hour going back and forth trying to figure out (by bisecting), which of these color keywords is used for the border of unfocused panes, but it turns out it's none of them. I thought I was going crazy.

Screenshot at 2023-07-04 14-07-34

EDIT: The color is taken from the terminal's palette!

If enviroment colors (LS_COLORS?) are available to Zellij, like they appear to be here, it would be handy to be able to use them as color keywords in targeted palettes later on. border-unfocussed: terminal-text, etc. Having it hardcoded like this is confusing, and a bit weird that it seems to be only for unfocussed pane borders.

@imsnif
Copy link
Member

imsnif commented Jul 4, 2023

Hey @cyberglot - are you still interested in working on this? It's of course understandable if life (or just anything else, really) got in the way, but if this is the case maybe let's see if someone else wants to have a go?

@imsnif
Copy link
Member

imsnif commented Jul 10, 2023

I think at this point this issue is up for grabs. If anyone wants to work on this (be aware that this is a mid-large-ish task - so please only commit if you want to see this through), make your voice heard.

@imsnif imsnif added the help wanted Extra attention is needed label Jul 10, 2023
@Zykino
Copy link
Contributor

Zykino commented Aug 12, 2023

Just as a note for anyone who is interested, apparently OSC strings may be used to change the them in standard terminal (see #2633). So even if the person wanting to find/implement a better theme spec may not want to do the OSC thing in the same time, it may be interesting to have some way to translate one to the other.
Even if only on paper.

Also I thay that… but I don’t know how this works so maybe I misunderstood something 😅

@jscarrott
Copy link

The issue with the current theming config has shown up with the new session manager #2747 . It uses the bg colour to highlight the currently selected session. Except the bg colour has been set for most themes to the colour you expect the default background to be, for nord this is #2e3440. Unsurprisingly this is the colour most people who want a consistent theme will have set the terminal background to, meaning it renders as invisible. I've changed it to be a much brighter colour so it works but it's hard to say what else uses the bg colour as it isn't very descriptive.

@Zykino
Copy link
Contributor

Zykino commented Sep 5, 2023

It was "fixed" by using the background color because someone on discord did not see the selection on the default theme (and/or their terminal’s theme) 🤣.

We all agree that a better theming is needed. But for now imsnif is working on others stuffs, I personally don’t use theme much so any proposal I could come up with will have flaws too. And the other devs don’t seems to be interested either.

To advance this issue, we need a concrete proposal that imsnif (or someone else) can take, and implement "blindly". (Sure the proposal can be improved upon with discussions before that.)

I like a lot the helix one as I understand it from the first message:

  • Each ui element is defined separately
  • Each element have a fg and bg properties to set respective colors, they also have a modifiers array for bold/italic, …
  • Some elements (all?) have an underline property that itself have 2 properties color and style => I think for zellij this should be for everyone or nobody: either we can underline all text or none, we do not have diagnostics
  • All colors are custom names set by the theme. They are grouped in a palette section with name=value. => This let the themes to easily reuse color anywhere they want independently of each others.

I think those points are a sane start. Then we will need to list every elements, maybe in the same way as the helix example:

  • background
  • selection
  • tab.inactive
  • tab.active
  • soloUser
  • multiUser <--- this one should be an array so multiUser[0] = …, multiUser[1] = …, <…>

I’m not really sure of all the elements from the top of my head, and we could have some extras for the plugins, maybe.

Last thing, maybe we should also set the delimiter (the arrow, or as the --simplified-ui) so anything visual is modifiable by a theme.

Obviously, this message is not a full spec, but if you want to help, please use it as a draft if it help you !

@DeaconDesperado
Copy link

DeaconDesperado commented Nov 27, 2023

Hey friends: first off thank you very much for your work on this multiplexer, since adopting it a month or so ago it's really magnified my productivity 🙇

Over the past week I've been looking into this issue and I think I will soon have something approaching a "walking skeleton" implementation of the design originally proposed up thread.

The gist is (names subject to change obv):

  • Change Palette to assign arbitrary color names by composing a BTreeMap<String, PaletteColor> internally
    • Provide default entries if absent for the current named colors (green, cyan and the like) to preserve backward compatibility with any existing user configurations, falling back ultimately to those defined in shared::default_palette.
  • Introduce new data structs for theming specific UI components:
    • StyleSpec{ fg: PaletteColor, bg:PaletteColor }: Value type for assigning actual colors from Palette to UI elements. Presently doing this at runtime by name lookup in Palette. Can probably also add text modifiers like bold and ul to this type, similar to what delta/gitconfig do
    • ThemeAssignments { selected_ribbon: StyleSpec, unselected_ribbon: StyleSpec ...}: Where the various StyleSpecs get attached to UI elements, still working out the full inventory from looking the default_plugins. It's this struct that would then be referenced from plugins, have a proto equivalent, etc.
    • Build up one from the other using the existing KDL macros in kdl::Themes::from_kdl

A hypothetical KDL config using this setup could look like:

theme "kanagawa_wave"

themes {
  kanagawa_wave {
    palette {
        sumiInk0  "#16161D
        waveBlue1 "#223249"
        waveBlue2 "#2D4F67"
		// ... other named colors

    }
    styling {
      selected_ribbon {
        fg sumiInk0
        bg waveBlue1
        // ... other UI elements
      }  
  }
}

A few questions that came up in the process of working on it:

Backward compatibility:

It's definitely possible to keep existing user theme configurations rendering as expected on upgrade. This could be accomplished by making the default implementation for ThemeAssignments use the existing named colors under the assumption that they're included in the palette map.

It seems less straightforward to have plugin code be backward compatible, since they make heavy use of the existing Palette structs' named fields. These would be moved to reference the UI elements by name from ThemeAssignments. Would it be okay to make that kind of breaking change?

(De)serialization

Deserialization from config seems to be entirely handled by the kdl crate with no usage of serde_derive? Palette and it's friends do derive Serialize and Deserialize however.

I think it's a nice UX to have the dynamic color names part of the theme block in configuration, but ideally the palette map itself would not be stored as a member of Theme in memory unless necessary. Where / how are the Serialize and Deserialize impls of Theme and Style used? Is this to support proto versions for remote / multi-user sessions? Can the structure for those vary from the one for the KDL configuration?

@oredaze
Copy link

oredaze commented Dec 30, 2023

In the meantime can something be done about the unselected pane border color, like make it use the fg or white from your theme?

@imsnif
Copy link
Member

imsnif commented Feb 6, 2024

Hey @DeaconDesperado - still interested in working on this?

@DeaconDesperado
Copy link

@imsnif yessir, can probably have a draft pushed up in a few days.

@imsnif
Copy link
Member

imsnif commented Feb 6, 2024

Nice! Let's start with just the spec, ok? Specifically just the UI components part. I'm not saying no to your other cool suggestions of color aliases and such but I'd like to start small (and also with a spec before we start coding) and work our way from there.

We have also introduced UI components since this suggestion came to be, which should make the implementation a little easier: https://zellij.dev/documentation/plugin-ui-rendering#using-the-built-in-ui-components

@DeaconDesperado
Copy link

Sounds good, no worries. Mostly just started the implementation in the previous comments to build my own understanding: totally amenable to change.

When I get back to the desk I'll draft up a specification and we can work together to refine it.

@DeaconDesperado
Copy link

A proposal based upon the UI Components as well as some of the comments in this thread. Hoping this is a good place to start!

A few notes in the spirit of keeping it simple to start 😄 :

  • color refers to a color specified as it is today, using any of:
    • A hexadecimal string
    • A triplet of u8 for TrueColor
    • A single u8 for a 256 color
    • We can explore the alias idea later when implementing the KDL mapping, for now just going with this formal definition since the aliases would just be pointers.
  • Also leaving aside setting text variants like bold and underline for the moment
    • These also seem pretty easy to add if we can nail down the color part.
  • Style definitions are "flat":
    • Statefulness of elements (eg "selected" vs "unselected") is represented with a separate definition.
    • This keeps the spec flexible enough to accommodate new states, eg the possibility of an eventual button_active and button_disabled

UI Components

These correspond directly to the packaged UI components for use in plugins.

Each of these settings accept five colors. Four correspond to the indicies used with color_range to set foreground text colors. The last is a background color. This allows stateful elements to change all colors to represent their state.

If omitted, text_unselected and text_selected will default to the first four colors from the user's terminal palette (black, red, green, yellow), and the user's terminal background.

If any of the remaining are omitted, they will default to the corresponding value of text_unselected | text_selected (and recursively to the terminal default if those are also omitted).

  • text_unselected
  • text_selected
  • ribbon_unselected
  • ribbon_selected
  • table_title
  • table_cell_unselected
  • table_cell_selected
  • list_unselected
  • list_selected

Core components

These correspond to other components of the Zellij UI that would benefit from consistent styling, but don't yet have reusable UI components.

  • key: Used consistently to style a key hint, for example <n> in the Tips dialog. Accepts only a single text color.
  • key_modifier: Used to consistently style a modifier key hint, for example Alt in the Tips dialog. Accepts only a single text color
  • frame_selected: Used to style the stroke and title on active pane frames. Accepts a single color.
  • frame_unselected: Use to style the stroke and title on unselected (inactive) pane frames. Accepts a single color

@oredaze
Copy link

oredaze commented Feb 7, 2024

From reading your proposal it is unclear to me if we can set a component (lets say frame_unselected) to a specific color that our terminal uses, say gray (bright variant of black). Which is what I want to do.
In fact if the default is for zellij to pull all colors from your terminal instead of setting truecolor/256 themes (and of course be adjustable, because I don't like green selected frames) it would be most sensible.

I bet money that most, if not everyone, who sets custom colors in both their terminal and zellij, want to have both themes match (same greens, etc.) and that would save time.

@DeaconDesperado
Copy link

DeaconDesperado commented Feb 7, 2024

@oredaze Definitely! (I count myself among that userbase).

Might be unclear by design though presently? I want to respect @imsnif's request that we first align on enumerating the UI elements and account for how the index-based contextual colors for them would behave. The "default to your terminal" behavior described for text_* above is to preserve a sane spec with entries for every text color index, assuming the absence of explicit theme settings.

I do think we could support your use case of explicitly setting any color to your terminal's when implementing the Configuration + KDL schema. Similar in spirit the idea of having named color aliases in the config, we could reserve a subset of names (probably the same ones from the existing Pallette struct) that correspond directly to your terminal colors, allowing it to be delegated to the emulator.

@imsnif
Copy link
Member

imsnif commented Feb 8, 2024

Hey @DeaconDesperado - great (and fast!) work.

Some comments:

  1. I think maybe each element needs to receive six rather than five color options (one for the base color, four for the four indices and one for the background).
  2. I think the idea of styling the key modifiers is great, but I'm a little unsure about it... what happens if a key modifier is on a ribbon (as is the case in the status bar first line)?
  3. I love being able to style the frame individually, can we give it index colors as well? In the near future plugins will be able to change the frame color of individual panes for emphasis. Would be nice if this could be themed.
  4. Command panes (eg. panes started with zellij run) have a frame with an EXIT CODE and some controls. Do you think these should be colored explicitly as their own elements (eg. error code color, OK code color, key codes...)?
  5. Regarding terminal colors - I'm pretty sure if a theme defines the first 16 colors in the 256 color range they'll use the system (terminal) colors... do check me on this though. Might be nice to add aliases as an "ease of use" sort of thing in this case, but let's not conflate things and leave this for later. There's plenty of work as is to see this through.

@DeaconDesperado
Copy link

  1. My mistake, I had incorrectly thought the index 0 wasn't distinct from base color. Will revise.
  2. It's a good point I hadn't considered, the key and key_modifier are cases where a hierarchical model (although more complex) could be desirable? Applying the "keep it simple" principle, how would you feel about just creating separate definitions for all present instances of them? EG in addition to key, key_modifier for the tips, also have key_ribbon, key_modifier_ribbon. The color chosen would have to be the same for both selected + unselected states, but I think that might just be good consistent highlighting UX (also matches the present behavior).

    Covering part of point 4 we could also add key_frame, key_modifier_frame: since those controls only appear on active frames the selected here would be implied.
  3. Sounds good, let's definitely add this, brings it closer in line with the rest of the elements. In the case of pane_frames: true these same colors would apply to the pane label as well (the numbered Pane #x, or in the case of zellij run, the command that was ran).
  4. I must admit, I missed this feature as I typically run with pane frames off (and hence the other omissions!). I think it makes sense to add exit_code_success and exit_code_error to cover them.
  5. I did test it out and this does seem to be the case. Definitely feels like we can punt the ease of use stuff for now.

@imsnif
Copy link
Member

imsnif commented Feb 9, 2024

  1. Hum... While I totally get this would be a really nice addition, I'm a little unsure about its ROI. This will add a lot of complexity to this whole feature and to styling in plugins in general. Right now in the newer plugins (eg. the session-manager) I style keybindings differently because it fits the UI of the app. Since I use the color indices to do this, it still fits the general theme of the app. How about we put a pin in this for now and add it later if there's demand?

  2. Fantastic.

  3. Sounds great

Would you like to write up a revised spec with all our decisions (unless there are more questions?) so that you can start implementing?

@DeaconDesperado
Copy link

  1. Sounds good to me. We can follow that convention for all of them.

I'll rewrite it updated with these changes later today. Thanks!

@DeaconDesperado
Copy link

DeaconDesperado commented Feb 9, 2024

UI Components

These correspond directly to the packaged UI components for use in plugins.

Each of these settings accept six colors:

  • A base color for when no index-based coloration is applied
  • Four index-based colors for use with color_range
  • A background color

If omitted, text_unselected and text_selected will default to the first five values in the TrueColor range from the user's palette (black, red, green, yellow, blue), and the user's terminal background.

If any of the remaining are omitted, they will default to the corresponding value of text_unselected | text_selected.

  • text_unselected
  • text_selected
  • ribbon_unselected
  • ribbon_selected
  • table_title
  • table_cell_unselected
  • table_cell_selected
  • list_unselected
  • list_selected

Frames

These correspond to the stroke around a pane, it's label, and the exit code messages shown in the case of zellij run -- <cmd>.

As these are text / foreground only elements, they accept only five colors:

  • A base color for when no index-based coloration is applied
  • Four index-based colors for use with color_range

Settings:

  • frame_selected: Used to style the stroke and label (or command ran in the case of zellij run) on active pane frames.
  • frame_unselected: Use to style the stroke and label on unselected (inactive) pane frames.
  • exit_code_success: Use to style the exit code message of zellij run for exit code zero (success).
  • exit_code_error: Use to style the exit code message of zellij run for non-zero (error) exit codes.

@imsnif
Copy link
Member

imsnif commented Feb 12, 2024

Hey @DeaconDesperado - this looks great. I left some minor comments below, but I think we're good to start implementing.

I suggest you start with the UI components themselves (you can test with some screens in the session-manager, though note that not everything was moved to the ui components yet), and then decide whether it will be less work to move all plugins to use the UI components or to apply these colors to the elements not yet using the UI components. Up to you though.

First and foremost though - whatever we do we must make sure the current UI/UX and behavior are kept exactly the same and that old themes will still work.

Let me know if you have any questions (and you are of course invited to chat about it on Discord/Matrix - my availability might be a little higher there).

Good luck! Looking forward to seeing this implemented.

If omitted, text_unselected and text_selected will default to the first five values in the TrueColor range from the user's palette (black, red, green, yellow, blue), and the user's terminal background.

Some terminals don't support true color, so I think it would be best to keep the current behavior: if anything is omitted it defaults to the fixed color it has now (while these may not be the most beautiful, they were carefully chosen for readability and care was taken not to use the system colors for the same reason).

If any of the remaining are omitted, they will default to the corresponding value of text_unselected | text_selected.

Same here. It's also good not to mix things, as a default text background for example is not the same as a default ribbon background (selected or unselected). But this might just be misunderstanding your intention. I just want to make sure we're on the same page here before the implementation.

@DeaconDesperado
Copy link

DeaconDesperado commented Feb 12, 2024

Agree, certainly enough here to get started on implementation, which I can spend some cycles on today

First and foremost though - whatever we do we must make sure the current UI/UX and behavior are kept exactly the same and that old themes will still work.

I think this might be where the previous points about the configuration implementation and the possibility of aliases become relevant? I want to be mindful that we're trying to avoid swelling the scope here but it does seem like a way we could secure backward compatibility?

My expectation had been after doing some hacking that the best way to keep existing themes working would be to make the 17 existing color settings themselves named aliases that default to their assumed values. Assignments to the UI elements listed out in the specification we created could then be done by name. The default mapping for those could provide the names directly as they appear to today

To use an example, the selected_ribbon would implicitly default to:

selected_ribbon black red orange magenta blue green

Since black and green are the current assignments for selected base and background, and the remaining four are the set emphases.

Other named colors could be added to the above since effectively the names themselves just become labels (similar to the suggestion in the OP). It does introduce another validation invariant insofar as all the named colors set must exist in the theme's palette, but this could probably be tested for early / fail fast in config parsing (and would be met by definition for existing themes per above).

This also ensures support for existing configs that will lack kdl blocks for the UI assignments entirely, we can statically supply the above as a default for all the UI elements:

// Same old dracula theme, as it's set today...
dracula {
         fg 248 248 242
         bg 40 42 54
         red 255 85 85
         green 80 250 123
         yellow 241 250 140
         blue 98 114 164
         magenta 255 121 198
         orange 255 184 108
         cyan 139 233 253
         black 0 0 0
         white 255 255 255
         // Assignments don't appear in this existing theme, so default them to:
         /- style {
             selected_ribbon black red orange magenta blue green
             //... others, I haven't completed the full invetory of existing assignments yet :sweat_smile:
         }
}

I think it would be best to keep the current behavior: if anything is omitted it defaults to the fixed color it has now

Agreed, will amend that.

@imsnif
Copy link
Member

imsnif commented Feb 13, 2024

I think this might be where the previous points about the configuration implementation and the possibility of aliases become relevant? I want to be mindful that we're trying to avoid swelling the scope here but it does seem like a way we could secure backward compatibility?

Hmm... I'm not 100% convinced here tbh. While I totally get this will be an elegant solution, I feel from the user's perspective it might be a bit odd. Why these specific aliases for colors? Can I make my own custom aliases? Why does magenta work and pink not? etc.

I feel it will be clearer if we add the element theming on top of the previous specification so that it will override it if it exists. I also think this will be considerably less work.

@DeaconDesperado
Copy link

DeaconDesperado commented Feb 13, 2024

Yes, I suppose there is some risk of user's perceived ambiguity in the labels with that approach: A burden of explanation as to why historically some color names are special and appear in the example themes, that you can add new ones to the list, etc...

The reason I felt a slightly inclined towards it is that the only backward-compatible alternatives I could find involve keeping the former Palette struct a part of the plugin contract (as expressed in style.proto), in addition to a new set of data objects for this styling spec. Do correct me if you have something else in mind, but as best as I could understand it, this meant that plugins' styling logic would need variations that speak both the Palette's named colors as well as this new style definition.

Since that added complexity affects all plugin authors, it felt like a good case where the implementer of the styles "taking the pain early" might be a good tradeoff by pushing all the color labelling downwards into a config-only concern. Also considering the comments up thread propose a similar UX, might be desirable/familiar from some user's standpoint.

That said, I certainly don't feel strongly and can draft up the version that just uses an override (assuming I've understood it correctly). It is possible my impression here might be based off a slightly outdated version of the code that made less direct usage of the reusable UI components.

@imsnif
Copy link
Member

imsnif commented Feb 15, 2024

So, the protobuffers unfortunately cannot be changed - only added to (otherwise we'll be breaking the compatibility of old plugins).

I really think there should be a separation between the old "legacy" themes and the new ones. I think this separation should be part of the plugin contract. I'm very much into providing color aliases to make things easier, but I don't want to mix this with the old themes. I feel it will just be more confusing and difficult to explain.

@DeaconDesperado
Copy link

Makes total sense, I don't know how I overlooked that consideration 😬 . I think we're aligned then and I have what's needed to make progress on this for now. Thanks!

@imsnif
Copy link
Member

imsnif commented Feb 16, 2024

Looking forward to seeing this implemented! Let me know if you have any questions (here or on Discord/Matrix, as you wish).

@DeaconDesperado
Copy link

Sorry for the delay on this: made good progress on it so far and should have some time to wrap it up in the next week. Thanks!

@DeaconDesperado
Copy link

Draft PR is up! I left a (somewhat long, sorry!) outline of the open questions, maybe we can continue with review there. Thanks!

@DeaconDesperado
Copy link

Quick heads up: I updated the draft this week to incorporate some of the feedback.

@svallory
Copy link

Thank god I found this thread. I've also spent a lot of time trying to customize ZJ and couldn't get to a result I like. I'm eager to this PR from @DeaconDesperado merged. Thanks, man!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

9 participants