Custom Styles #1785

kolektiv · 2024-02-27T08:53:41Z

(Recreated PR!)

As mentioned in #1153 - I've had a quick little play at a simple way for people to set styles. I've tried to make it fairly minimal rather than introduce a whole theme system or something similar. Some things to note:

I've just used Ratatui colors as a basic component.
- That means that atuin-client now needs a dependency on Ratatui (but I don't think that's an issue; it's a dependency of crates which already have it).
- Ratatui color deserialization was very basic indeed (only named colors) until 0.26, after which it supports hex (RGB) and indexed colors, so I've moved Ratatui to a workspace dependency and bumped the version (haven't tested for side effects!)
Styles can be set under a new styles table in the config. I've currently implemented support for two styles, just as a proof - the command in the history list, and the selected command in the history list. They can be set something like the following (obviously, compact table notation is also fine):

[styles]
command = "#555555"

[styles.command_selected]
bold = true
foreground = "#90fa00"

Style values can be of two kinds (as seen above):
- A simple color value, in which case it will be treated as the foreground color.
- A table of style properties which make up a style, and which consists of...
  - foreground (color)
  - background (color)
  - underline (color)
  - bold (bool)
  - crossed_out (bool)
  - italic (bool)
  - underlined (bool)
- Properties which are not set will not be set on the style (there is currently no inheritance, etc.)

Here's how that particular configuration looks in my terminal (using the compact style).

Opinions/objections are welcome. Obviously, a couple of slight structural tweaks are needed, but I've tried to avoid anything too invasive.

Checks

I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
I have checked that there are no existing pull requests for the same thing

kolektiv · 2024-02-27T09:04:01Z

Just as an extra note if this is agreed as an approach, next steps would presumably be

Implement the rest of the style-able features (extend the Styles type, etc. and any drawing functions which currently create styles)
Create some basic docs?

atuin-client/Cargo.toml

ellie · 2024-02-27T13:30:44Z

atuin-client/src/settings.rs

@@ -303,6 +304,107 @@ impl Default for Stats {
    }
 }

+#[derive(Clone, Debug, Default, Deserialize)]
+pub struct Styles {


I know this isn't the only thing here, but would you be open to the following

Split settings into a module

Break the style stuff into it's own file

Don't worry about the rest for now, but settings.rs is getting fairly unwieldy now imo!

Yeah, settings.rs is a bit on the chunky side, and another hundred lines hasn't helped, clearly. When you say another module, are you thinking about a settings.rs as a root, containing only top-level construction-type code and then a settings/ dir containing things like styles.rs for example? That probably wouldn't be too much work I think, and I could probably do that as part of this PR as housekeeping.

If you are thinking about a bit of a crate re-org, how would you see it looking longer term? As far as I can tell at the moment, Settings is only actually created (new()) within atuin generally anyway, and then passed to client, etc. I wonder whether having the root type for Settings be part of atuin and then having atuin-client specific settings be defined in atuin-client and composed as part of the root settings type (and then passed back to atuin-client) might be a better long-term way? That way each component (client, and any future components) only get passed the config that's relevant to them, avoiding dependencies on things they don't care about, while still being able to have a single config management point. Just thinking out loud though at the moment.

When you say another module, are you thinking about a settings.rs as a root, containing only top-level construction-type code and then a settings/ dir containing things like styles.rs for example?

Exactly that!

And yes, the re-org I was considering is more-or-less what you say. However, I'd also like to break all "history" specific stuff out into an atuin-history crate. Then, we can make the main atuin binary just connect a few things together. Kinda like atuin-config.

Still a bit of a fuzzy idea though, and I'd like to see how other things shake out first. I'd also like to consider moving to KDL for config.

(gone way beyond the scope of this PR now, but just context setting)

That makes sense, I'll have a quick play around and see how dramatic it would be to include that in this set of changes. Just taking the struct members out shouldn't mean any changes to consumers, if we wanted to group some of the current settings into new types (e.g. paths) then code consumers would need updating and that might get quite extensive (shouldn't have a problem with the config file itself if new structs are serde flattened). Will see how it looks!

(I mean the settings module rather than a re-org! That can wait...)

Ok, I've started playing with a bit of a settings refactor, which can be seen in the latest commit. What I've done for now...

Pulled groups of settings out into submodules of settings with vaguely sensible names (I hope) and any specialized types where relevant

Re-exported those types from the original settings module, to avoid needing to change other code

What I've also done for one of those sets of properties (the display submodule) is to pull the properties into a new settings type at that submodule level, as well as a function for setting defaults for that submodule on the ConfigBuilder instance. I've then replaced the raw properties on settings with the new settings type, but with a serde attribute to flatten the type, so the structure of the actual config file is unchanged. That obviously does need some changes elsewhere, so those changes are reflected wherever there is now a ".display" included in a settings path.

The new submodule settings type is a potential option, just to see how well that fits in. I don't have any strong opinions on that one, see what you think and whether it's worth the slight extra code.

ellie · 2024-02-27T13:34:53Z

I like this approach, thank you! Just a few comments, nothing major though

ellie · 2024-02-27T13:36:21Z

Oh one other thought

It would be neat if we could also style the UI beyond colours this way. Like the differences between compact/full

EG

[styles.search_box]
border = none
etc

that way, full/compact could just be presets. Not at all required for this PR though

kolektiv · 2024-02-27T16:18:35Z

It would be neat if we could also style the UI beyond colours this way. Like the differences between compact/full

Yeah it would. It would be a bit of an expansion - at the moment the contents of styles maps basically 1-1 to the Ratatui type, and if you started to include a more general concept of style it would be more of a theme than a set of styles, but I agree that would be a good direction. I'll have a think about that one - it may be that something like theme = layout styles + text styles is a good rule of thumb, and would be a good evolution from here.

kolektiv · 2024-02-28T00:38:08Z

atuin-client/src/settings/display.rs

+// Settings
+
+#[derive(Clone, Debug, Deserialize)]
+pub struct Settings {


This is the new settings type mentioned...

kolektiv · 2024-02-28T00:38:31Z

atuin-client/src/settings/display.rs

+
+// Defaults
+
+pub(crate) fn defaults(


...along with a simple function to set defaults.

atuin-client/src/settings.rs

kolektiv · 2024-02-28T00:40:16Z

atuin-client/src/settings.rs

-    pub word_chars: String,
-    pub scroll_context_lines: usize,
-    pub history_format: String,
-    pub prefers_reduced_motion: bool,



This is where the new display settings type actual gets used, flattened, rather than having a set of properties here

kolektiv · 2024-02-28T00:41:28Z

atuin/src/command/client/search.rs

@@ -158,7 +158,7 @@ impl Cmd {
            settings.filter_mode = self.filter_mode.unwrap();
        }
        if self.inline_height.is_some() {
-            settings.inline_height = self.inline_height.unwrap();
+            settings.display.inline_height = self.inline_height.unwrap();


Any display settings are now a level deeper because of the new settings type, even though it's not visible externally in config.

ellie · 2024-02-28T13:26:07Z

um ok what github?

Could you reopen this please, but use a branch that is not main? It behaves very strangely in terms of permissions.

The rebased code is here: https://github.com/atuinsh/atuin/tree/kolektiv/styles

I could open the PR, but would rather it be credited to you of course.

tldr is that GitHub allows me to push to your branch, if you open a PR. I rebased this locally, pushed, and it just overwrote your main with the current main 🙃 ffs. I also now don't have permission to sort this out, as the PR was auto-closed and cannot be reopened.

Sorry about that 😭

ellie · 2024-02-28T13:38:07Z

Otherwise - thanks for sorting this out. I'm pretty happy with the approach here!

I'm a little unsure on the submodule Settings type, but I think it's ok for now and tidier than what we have.

The only other thing I'd like, is to ensure that a variety of already existing config files still load OK.

This doesn't necessarily need to be done here, as we have broken this a few times in the past. So perhaps a wider issue of config testing

kolektiv · 2024-02-28T15:07:22Z

Hah, I'm glad it's not only me that regularly gets tripped up by GitHub when it decides to be "helpful". No problem, what I'll do is grab that branch in my fork and work on there, and see where we end up. I'll go through and add in a "complete" (hopefully) set of styles.

I'm also not 100% sure on the submodules Settings type, but I think it's marginally neater, and it can always be done away with if/when a new approach is taken to config, so I'll go ahead and do it for each submodule. There "shouldn't" (TM) be any changes to config file reading like this, but I completely agree that actually checking that is probably sensible!

I had a look at KDL incidentally, as you mentioned it somewhere above - I'd not come across it before. Interesting. My only personal pain from that is that all my config is generated by nix, and I don't think it can generate KDL at the moment, but that's clearly a pretty selfish reason not to do it if it makes sense!

joaomendoncaa · 2024-03-23T02:50:54Z

Sorry for the ping everyone but any advancements on this initiative @kolektiv? Excited about it!

kolektiv · 2024-03-23T02:53:00Z

Yes, I'm going to try and pick it back up this weekend! I ended up travelling at short notice, so I'll put something together from here 🙂

xhalo32 · 2024-04-06T11:48:56Z

Hi, I added some more styling options and merged with the main branch in my fork.

As this feature still requires some work and is now entangled with the settings refactoring, should we split the settings refactoring part into its own PR?

joaomendoncaa · 2024-04-06T15:29:41Z

Hi, I added some more styling options and merged with the main branch in my fork.

As this feature still requires some work and is now entangled with the settings refactoring, should we split the settings refactoring part into its own PR?

If they are not mutually dependent on each other I'd love if the custom styles could be merged even if settings refactoring is not complete. Thanks for all the work.

ellie reviewed Feb 27, 2024

View reviewed changes

atuin-client/Cargo.toml Outdated Show resolved Hide resolved

ellie reviewed Feb 27, 2024

View reviewed changes

kolektiv commented Feb 28, 2024

View reviewed changes

atuin-client/src/settings.rs Outdated Show resolved Hide resolved

kolektiv commented Feb 28, 2024

View reviewed changes

ellie closed this Feb 28, 2024

philtweir mentioned this pull request Jul 7, 2024

Customizable Themes #2236

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Styles #1785

Custom Styles #1785

kolektiv commented Feb 27, 2024

kolektiv commented Feb 27, 2024

ellie Feb 27, 2024

kolektiv Feb 27, 2024

ellie Feb 27, 2024

kolektiv Feb 27, 2024

kolektiv Feb 27, 2024

kolektiv Feb 28, 2024

ellie commented Feb 27, 2024

ellie commented Feb 27, 2024

kolektiv commented Feb 27, 2024

kolektiv Feb 28, 2024

kolektiv Feb 28, 2024 •

edited

Loading

kolektiv Feb 28, 2024

kolektiv Feb 28, 2024

ellie commented Feb 28, 2024 •

edited

Loading

ellie commented Feb 28, 2024

kolektiv commented Feb 28, 2024

joaomendoncaa commented Mar 23, 2024

kolektiv commented Mar 23, 2024 •

edited

Loading

xhalo32 commented Apr 6, 2024

joaomendoncaa commented Apr 6, 2024

Custom Styles #1785

Custom Styles #1785

Conversation

kolektiv commented Feb 27, 2024

Checks

kolektiv commented Feb 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellie commented Feb 27, 2024

ellie commented Feb 27, 2024

kolektiv commented Feb 27, 2024

Choose a reason for hiding this comment

kolektiv Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellie commented Feb 28, 2024 • edited Loading

ellie commented Feb 28, 2024

kolektiv commented Feb 28, 2024

joaomendoncaa commented Mar 23, 2024

kolektiv commented Mar 23, 2024 • edited Loading

xhalo32 commented Apr 6, 2024

joaomendoncaa commented Apr 6, 2024

kolektiv Feb 28, 2024 •

edited

Loading

ellie commented Feb 28, 2024 •

edited

Loading

kolektiv commented Mar 23, 2024 •

edited

Loading