[DRAFT]: Theme definition

[DeaconDesperado]

Draft implementation of the specification laid out in #2297. Would love feedback on this!

Rough outline of the changes:

• Introduces a new struct, Styling, and the necessary protobuf records to match the spec from [Proposal] New theme definition spec #2297
• Implements KDL deserialization for Styling
• Provides bidirectional conversion for Styling <-> Palette in order to preserve backward compatibility for themes:
  • Assignment of colors from the old palette to the new spec was reconstructed gradually by manual comparison.
  • In some instances the same older named color may be used in multiple places (the new specification is more flexible and has more assignable styles, this only affects the default where user still has a Palette in their config).
• Preserve backward compatibility for plugins:
  • Styling -> Palette conversion is performed in the plugin_api layer, so the older message format is still propagated to plugins.
• Replace all references to named colors with style specifications

Open questions / notes

client_id_to_colors

This function is used to colorize pane frames and cursors in multi-user sessions. While mentioned in some of the comments, the proposed specification in #2297 didn't explicitly lay these colors out, and there are (presently) 10 of them (larger in number than the other specs). We could consider adding a separate style block for these, repurposing some of the emphases from frame_unselected, or using some sane defaults? This draft presently sets them to values from crate::shared::colors for the time being.

Configuration shape

KDL prohibits anonymous nodes, and lists are represented as sequential values after the node name. In order to support all three representations of color presently used (single digit, 3 digit rgb, hex) the style specifications all are named individually, and macros from the original Palette are reused to deserialize the color, EG:

text_unselected {
  base "#DCD7BA"
  emphasis_1 "#ACD7CD"
  emphasis_2 "#DCD8DD"
  emphasis_3 "#DCD899"
  emphasis_4 "#ACD7CD"
  background "#16161D"
}

It's arguably a bit redundant to have these indexes named sequentially this way? It might be possible to consolidate all the emphases to one list, but then that one node would need to be extracted into all 4 indexes somehow, which presents the prospect of mixing the EightBit/RGB/Hex representations... WDYT?

Currently, Styling's members are stored as fixed length arrays: it felt like a reasonable way to preserve the ability to add more colors in the future if needed. An alternative could be this structure though.

UI Components

Given that I'm only just getting familiar with the codebase, I opted to make these changes in place without also doing a migration of plugins to the various UI components 😅 . The coloration should be consistent however, only preserving differences where they are an artifact of the present implementation.

An example: When using simplified_ui, the ribbon implemented directly in the status-bar plugin uses alternating background colors to differentiate "tabs", whereas the UI component (eg in session-manager) uses divider spacers colored to match the terminal background. Where encountered, these kinds of differences are preserved (for now).

ThemeHue + PaletteSource

The swap of foreground/background based on ThemeHue is preserved only when converting a Palette to a Styling. The only place I could find this value to be set was in this function, which doesn't seem to be called anywhere. Perhaps it's leftover from an earlier implementation? I seemed to uncover similar for PaletteSource. Not sure if I'm missing something here...

Strider

I was unfortunately unable to do as much vetting of strider specifically, encountered #3064 when trying to test it out. Might require some help with this.

Still TODO

In addition to incorporating any feedback, still need to:

• Update test suites that use snapshot assertion (temporarily ignored here)
• Investigate any e2e test suites that need to be updated
• Document the new theme KDL and update the examples
• Rebase and clean up commit messages
• Might make sense to extend the zellij convert-theme utility to migrate existing configurations to the new format?

Screenshots

Comparison of default theme (left new, right old)

Welcome screen

Frameless layout

Run pane exit codes

Simplified UI (no arrow fonts)

Compact layout

[imsnif]

I'm very excited to see this happening!!

Just a heads up: I'm in the process of wrapping up v0.40.0 in the next week or so, so it will take me a short while to get to this. But rest assured, I will! I ask for your patience and thank you again for what seems like a tremendous amount of work!

[glenn-lytx]

Is there a full config example for this? I'd like to grab this and compile it locally to test it out ..

[imsnif]

Hey @DeaconDesperado - amazing and very thorough work here! Thank you for your patience and apologies for taking so long to give you an initial review. I'll try to be faster from now on so we can iterate over this quickly.

If it's alright with you, I prefer to go over the actual code changes once the product side (API, config, spec, behaviour, etc.) has been finalized. I think it'll save both of us some work, because changes in that area can cause sweeping changes in its implementation.

So to your questions:

Open questions / notes

client_id_to_colors

Good catch, we didn't account for these. Perhaps we can add a multiplayer_user_ids section to the theme definition?

Configuration shape

It's arguably a bit redundant to have these indexes named sequentially this way? It might be possible to consolidate all the emphases to one list, but then that one node would need to be extracted into all 4 indexes somehow, which presents the prospect of mixing the EightBit/RGB/Hex representations... WDYT?

I actually like it this way. I understand it can seem a bit redundant, but I think in the context of a theme configuration this is much clearer than emphasis <color_1> <color_2> <color_3> <color_4> - especially considering the different color formatting as you mentioned.

Currently, Styling's members are stored as fixed length arrays: it felt like a reasonable way to preserve the ability to add more colors in the future if needed. An alternative could be this structure though.

I don't have a strong opinion here, so up to you. A few considerations:

1. Performance (unless I'm wildly missing something) is not an issue for this feature whatsoever. So there's no need to be very cognizant of memory copying/cloning (in the context of the fixed-size arrays). Parsing the configuration is an order of magnitude slower, serializing the struct across the wasm boundary is an order of magnitude slower and otherwise sending it between threads is something that happens only on application startup (or configuration change) in which cases there are much slower things that happen as part of the process (eg. loading plugins, rendering, etc.)
2. If you decide to go with fixed-size arrays after all, I'd rather not have them in a public interface, and care needs to be taken in their setters so as not to crash ever if they overflow.
3. I usually prefer to be verbose in these sorts of things. If it were me I'd have a specialized struct similar to the one you mentioned with emphasis_1, emphasis_2, etc. as explicit members. But really - up to you.

UI Components

Given that I'm only just getting familiar with the codebase, I opted to make these changes in place without also doing a migration of plugins to the various UI components 😅 . The coloration should be consistent however, only preserving differences where they are an artifact of the present implementation.

Legit! Let's keep things simple.

ThemeHue + PaletteSource

The swap of foreground/background based on ThemeHue is preserved only when converting a Palette to a Styling. The only place I could find this value to be set was in this function, which doesn't seem to be called anywhere. Perhaps it's leftover from an earlier implementation? I seemed to uncover similar for PaletteSource. Not sure if I'm missing something here...

Yes, these are legacies! It's totally fine to remove them if you want.

Strider

These issues were fixed in v0.40.0 and iirc I even moved lots of stuff to UI components, so I hope this will make things easier!

Anything else I can do to help move this along?

[DeaconDesperado]

@imsnif Thanks for the thorough answers! Will incorporate this feedback and have another go at Strider.

[DeaconDesperado]

@glenn-lytx below is a sample of the themes block I've been using locally (also used in the screenshots), just need to change theme_name to suit:

themes {
  theme_name {
     styling {
          text_unselected {
              base "#DCD7BA"
              emphasis_1 "#ACD7CD"
              emphasis_2 "#DCD8DD"
              emphasis_3 "#DCD899"
              emphasis_4 "#ACD7CD"
              background "#16161D"
          }
          text_selected {
              base "#16161D"
              emphasis_1 "#ffa066"
              emphasis_2 "#16161D"
              emphasis_3 "#16161D"
              emphasis_4 "#16161D"
              background "#9CABCA"
          }
          ribbon_unselected     {
              base "#DCD7BA"
              emphasis_1 "#9CABCA"
              emphasis_2 "#7AA89F"
              emphasis_3 "#ACD7CD"
              emphasis_4 "#DCD819"
              background "#363646"
          }
          ribbon_selected {
              base "#16161D"
              emphasis_1 "#ffa066"
              emphasis_2 "#1A1A22"
              emphasis_3 "#2A2A37"
              emphasis_4 "#363646"
              background "#76946A"
          }
          table_title {
              base "#DCD7BA"
              emphasis_1 "#7FB4CA"
              emphasis_2 "#A3D4D5"
              emphasis_3 "#7AA89F"
              emphasis_4 "#DCD819"
              background "#252535"
          }
          table_cell_unselected {
              base "#DCD7BA"
              emphasis_1 "#DCD7CD"
              emphasis_2 "#DCD8DD"
              emphasis_3 "#DCD899"
              emphasis_4 "#ACD7CD"
              background "#1F1F28"
          }
          table_cell_selected {
              base "#16161D"
              emphasis_1 "#181820"
              emphasis_2 "#1A1A22"
              emphasis_3 "#2A2A37"
              emphasis_4 "#363646"
              background "#76946A"
          }
          list_unselected {
              base "#DCD7BA"
              emphasis_1 "#DCD7CD"
              emphasis_2 "#DCD8DD"
              emphasis_3 "#DCD899"
              emphasis_4 "#ACD7CD"
              background "#1F1F28"
          }
          list_selected {
              base "#16161D"
              emphasis_1 "#181820"
              emphasis_2 "#1A1A22"
              emphasis_3 "#2A2A37"
              emphasis_4 "#363646"
              background "#76946A"
          }
          frame_unselected {
              base "#DCD8DD"
              emphasis_1 "#7FB4CA"
              emphasis_2 "#A3D4D5"
              emphasis_3 "#7AA89F"
              emphasis_4 "#DCD819"
          }
          frame_selected {
              base "#76946A"
              emphasis_1 "#C34043"
              emphasis_2 "#C8C093"
              emphasis_3 "#ACD7CD"
              emphasis_4 "#DCD819"
          }
          exit_code_success {
              base "#76946A"
              emphasis_1 "#A3D5D5"
              emphasis_2 "#76946A"
              emphasis_3 "#76946A"
              emphasis_4 "#76946A"
          }
          exit_code_error {
              base "#C34043"
              emphasis_1 "#DCD819"
              emphasis_2 "#C34043"
              emphasis_3 "#C34043"
              emphasis_4  "#C34043"
          }
    }
  }
}

[glenn-lytx]

@glenn-lytx below is a sample of the themes block I've been using locally (also used in the screenshots), just need to change theme_name to suit:

Awesome, thanks! I'll give it a try

[DeaconDesperado]

Revised this quite heavily with the suggested changes:

• Changed to an explicit struct, StyleDeclaration, in lieu of fixed size arrays + position indexing.
• Introduced a new type MultiplayerColors and handling for multi-user sessions.
• Corrected handling of styles in UI components (previous draft did not properly style tables and lists independent of styling in text).

To validate the behavior of the UI components, I set up a very simple toy example based on rust-plugin-example that just renders all the elements in various states (with silly, but noticeable color differences between states)

Let me know if the high level approach here now looks sound, and then I'll proceed with improving the test coverage for backward compatiblity and the aforementioned product stuff to support migration (docs, possible script to migrate old palette to new themes). Also LMK if we prefer that in a separate PR. Thanks!

[imsnif]

Hey @DeaconDesperado - I'm happy to see the work going on. Kudos on your perseverance and consistency, this looks like quite a great effort!

I'm sorry it's taking me a bit of time to get to this, but rest assured - I plan to ASAP! I have not forgotten nor do I mean to let your work go to waste.

[imsnif]

Hey @DeaconDesperado - this sounds good (though I will as mentioned wait with going over the exact code until we finalize the product stuff).

Including a conversion script is a very nice touch, but assuming we preserve backward compatibility and the existing themes will continue working as expected, I think it's alright. This is mostly meant to allow people to more easily create new themes with greater flexibility.

For the interests of efficiency, I have put aside some time every week on Friday morning CEST to go over this PR. So that you'll know when I'll do the next round and are able to prepare for it. I'll try to be available for quick questions in between so that we can get this going.

What else do you need from me for the time being?