-
-
Notifications
You must be signed in to change notification settings - Fork 686
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
Theme definition #3242
base: main
Are you sure you want to change the base?
Theme definition #3242
Conversation
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! |
Is there a full config example for this? I'd like to grab this and compile it locally to test it out .. |
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:
Good catch, we didn't account for these. Perhaps we can add a
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
I don't have a strong opinion here, so up to you. A few considerations:
Legit! Let's keep things simple.
Yes, these are legacies! It's totally fine to remove them if you want.
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? |
@imsnif Thanks for the thorough answers! Will incorporate this feedback and have another go at Strider. |
@glenn-lytx below is a sample of the themes block I've been using locally (also used in the screenshots), just need to change
|
Awesome, thanks! I'll give it a try |
e9bebbc
to
5c845d3
Compare
Revised this quite heavily with the suggested changes:
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! |
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. |
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? |
@imsnif I didn't find time to put a cap on the sole failing integration test this week, but I was wondering what would be the best way to confirm the outstanding product related points? Would you prefer a description incorporating any changes to what was discussed in #2297 here in the comments, or a separate PR to website repo that includes the user-facing documentation? In the interest of providing something in time for the Friday review bandwidth you carved out for me (thanks!) I'll try to preemptively give the former here, updated to incorporate all the discussions we've had (this is also what's presently working): Updated SpecWherever colors are specified, they use the existing definition prior to this change, using any of:
A theme consists of a
Any omitted declarations default to the same fixed colors they would prior to this change (constructed from reverse engineering what named fixed colors mapped to what parts of the UX). In order to make sure the theme is coherent, these declarations cannot be partially defined: if they are specified at all, all 6 colors must be set in them (or return an error at startup). They are:
The following additional
And finally, a key Complete example configuration for a named theme
|
Hey @DeaconDesperado - thanks for putting this together. This is great and really helps me reviewing this PR quickly. I appreciate the clear communication and your meticulousness! This looks really good to me and I hardly have any comments. Just a few things:
To make sure I understand this correctly: an entire component can be omitted from the styling (eg.
Could we make this apply to the mouse-selection and search results too? These happen here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/panes/grid.rs#L1088 (and a few lines below in the same function)
Could we also add a Otherwise - this looks very good and I'm happy to go forward with this and start reviewing the implementation once you're ready. Fair warning: I'm in the process of revamping the UI of the status-bar to solve the colliding keybindings issue, so I imagine I'm going to create some chaos there for you. I'll do my best to make it minimal and help with the merge afterwards. Amazing work! Looking forward to iterating more and getting this merged. ADDENDUM: I realized I didn't see any reference to this, so just to make sure: the old themes will still work, right? We have to remain backwards compatible (I'm even in favor of allowing theme authors to mix the new component declarations into the old color themes if they want, but this is not a must IMO). |
That's correct. I think it makes the most sense to have theme authors always provide a declaration in full or not at all, since partial application of defaults could lead to weird text / background mismatches.
I'll add these to the punchlist 😄.
Yes, there is a Edit: As it is currently, the new vs old styles can't be mixed together and are configured disjointly. If your named theme includes a |
Hey, I couldn't find the declaration in the link and I'm still not up to speed on the implementation details - so just to make sure (because this is a showstopper and I don't want to needlessly add work for you in the end): if I have an arbitrary file (not included in the Zellij repo) that has the old theme declaration with colors, that works now - will it continue working after this? It's totally fine if we can't mix these declarations, but if that is the case, let's error if they are mixed. |
Hmm the link does seem to be broken when highlighting the lines. Yes, they will continue to work without problem. The old format is just mapped into the new one in memory and at runtime when configuration loads. |
a5f58bf
to
0bc59cd
Compare
I believe this should now be ready for review. I added a few test cases to validate the configuration works as expected (and errors under the right conditions). Per our separate discussion in Discord, I removed any commits I had made to the e2e tests in an effort to get them to work with my local setup (we ran into OSX environment-specific issues). Will rely on the CI/CD feedback for these. |
Hey @DeaconDesperado - first off: fantastic work once again! I had a list of things to check on the compatibility/API level and this implementation passed them with flying colors. The This looks great all in all - I have a first round of rejects. Let's iterate over this a bit and once we're satisfied I'll do a deeper dive into the code 1. Regarding the
|
Thanks! Once I managed to understand how the plugin API works it felt a bit natural to anticipate certain backward compatibility needs there (it's very nicely designed!) 1We can remove this, yeah. The thought behind it had been that perhaps there could be a separate node 2Pushed two commits to fix this, quick oversight on my part to not backport the same fix from the UI ribbon component into its separate implementations in tab-bar + compact-bar . 3This presently uses one of the emphasis colors that would map for 4/5I suspect the disappearing text is in fact the test theme, if you're testing with the one I pasted in the comment earlier I had to rearrange a few definitions. I'll paste a new one in a following comment. Will definitely look to collapse that extra space between cells as well. One other thing: in testing some of the existing themes as-is (dracula) I'm noticing that by prohibiting partial application of colors, we might make it impossible to make some elements keep transparency? EG you must provide a Dracula original: Dracula now: Perhaps it makes sense to make background optional? |
Updated test theme
|
54c8ddf
to
0566317
Compare
Hey, took this for another spin and all the issues seem good to me! I replied in-line to the frame-highlight issue. Otherwise, I ran the CI again and if all is well I'm going to give this another week to see if we get any comments from @dj95 (no pressure at all!!) and then I'm happy to merge. After this is merged, I'll write up some documentation for this and would be happy to get your input if you'd be up for it. EDIT: seems like we have another snapshot to update: https://github.com/zellij-org/zellij/actions/runs/13020325545/job/36471935504?pr=3242#step:11:2117 |
Hi, first of all, thanks a lot for asking me for feedback. I really appreciate that. It's one of the favorite features, I wish for a long time for. It looks awesome and I had a lot of fun tinkering around with the theme spec while testing it. In the first tests, I've tested zjstatus (my status bar plugin with some custom rendering), which works totally fine. No issues at all. However, I've found some smaller issues within the ui components. If there's something I did wrong on my end, please don't hesitate to point it out. I can also provide the code for testing it, in case you are interested.
|
Yes looks like this is during the e2e test suite... I've only been triggering tests with |
No worries @DeaconDesperado - please don't update the snapshots manually, it's a lot of needless meticulous work. If it comes to it I'll do it. If you could take a look at the issues @dj95 found though, that would be great. |
Hey @dj95 I really appreciate you giving this a go and for the kind words on the implementation 😄
With fixes + demo for ribbon positioning using your plugin source code: @imsnif I didn't manage to get the e2e tests running to generate that specific insta snapshot. Would def appreciate if you could add that commit for me... I looked through the others in there and it seemed to be just that one test that logs out the |
Awesome! Thanks a lot for your quick fix @DeaconDesperado . Now it looks exactly the same. |
Pushed up a fix for the optional styling of When setting an explicit I think this is the correct behavior, matching also what @dj95 as a user expected. But it bears mention there's nowhere I could find in the zellij codebase where the explicit width setting is used. It did seem that for several components though, there is a habit of adding |
Im not sure if I understood your comment 100%. But in the plugin code, I set the width to 12, which includes the chars for the arrow itself, as the string "ribbon 1" contains 8 chars und you then need 1 for spacing and 1 for the arrow on each side. But I agree, that it's much more intuitive, when you just need to specify the text width and do not keep the 4 extra chars for the arrow around it in mind. Although this would impose a breaking change in the behavior of the UI components. |
Draft implementation of the specification laid out in #2297. Would love feedback on this!
Rough outline of the changes:
Styling
, and the necessary protobuf records to match the spec from [Proposal] New theme definition spec #2297Styling
Styling
<->Palette
in order to preserve backward compatibility for themes:Palette
in their config).Styling
->Palette
conversion is performed in theplugin_api
layer, so the older message format is still propagated to plugins.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 fromcrate::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: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
, theribbon
implemented directly in the status-bar plugin uses alternating background colors to differentiate "tabs", whereas the UI component (eg insession-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 aPalette
to aStyling
. 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 forPaletteSource
. 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:
snapshot
assertion (temporarily ignored here)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