Refractor: group colors #802
Conversation
| @@ -97,22 +90,6 @@ pub trait Prompt: Send { | |||
| &self, | |||
| history_search: PromptHistorySearch, | |||
| ) -> Cow<str>; | |||
| /// Get the default prompt color | |||
| fn get_prompt_color(&self) -> Color { | |||
There was a problem hiding this comment.
I'm just wondering if there's a way to do this without creating a breaking change. Removing these 4 functions will break everyone who's using reedline and making custom prompts, right?
There was a problem hiding this comment.
I don't see a way to keep these functions.
These 4 functions have default implementations and are the current way to change the default colors of reedline. Looking at the reverse dependancies of reedline I picked a few to check if they have there own implementations ans so this would be a breaking change:
That don't:
That do:
Since changing the colors are already used by existing users, at the very least I would need to expose Theme record for diatom. If having a breaking change isn't a show stopper, I will add that.
There was a problem hiding this comment.
of course, I'm most concerned about nushell with this change. I see you say that nu-cli doesn't break.
There was a problem hiding this comment.
Glade this isn't a blocker. Added builder methods so diathom-cli (and other users) can still change the colors.
|
I think this is cool and an interesting way to abstract the colors into a theme, but I worry about existing users as mentioned above. Also the "theme" name should probably be less generic. |
|
I thought Theme was in line with Prompt. What would you prefer? ReedlineTheme or ColorTheme? I don't have strong opinions about the name and am open to change it. |
|
Maybe ReedlineTheme. I'm just thinking that everyone calls their theme things "Theme". When we're in nushell, I don't want to be confused about which theme we're talking about. |
|
Looks like there are some conflicts to resolve. |
Fixed. |
|
Thanks. Are you up for adding a PR to nushell with config settings in order to give this a thorough test? |
|
ping @adamschmalhofer |
|
Am Sonntag, dem 07.07.2024 um 06:37 -0700 schrieb Darren Schroeder:
Thanks. Are you up for adding a PR to nushell with config settings in
order to give this a thorough test?
I do plan on continuing this work and I care about reedline, because I
have switched my shell after 25 years from bash to nushell. Before
adding the theming to nushell, I would first want to continue this work
on reedline. At the moment reedline uses different Color-enums
depending on the favorite color of nushell (crossterm::style::Color vs.
nu_ansi_term::Color). The next step would be to unify them e.g. by
switching to Style.
Since we are talking about my future plans: My other main focus will be to get the vi mode in a state that it is possible to add a Helix mode as @sholderbach described in #642 .
|
|
ok, i don't want to land this and then be stuck in a place where someone else has to implement something in nushell without fully understanding these changes. so we can wait on landing this until you're ready to land its counterpart in nushell. Thanks for pitching in! |
Well, if changes would be need to be done in nushell for this pull request, I would do them. Most projects that I know of (including myself) prefer small incremental changes rather than a large "dump". This reduces the problem of merge conflicts and helps the code reviews stay focused. Does a pull request need a visible user facing feature in nushell to be considered? Or is the problem that it isn't properly tested, because nushell doesn't change the colors? In that case would a pull request for diatom-cli be sufficient? I'm just a bit confused and would like to know for this and future pull requests. |
|
Thanks for responding. I'm hyped about this PR. Here is my thinking/reasoning:
does all that help understand my position? |
|
Thanks for taking the time to elaborate. I think to part where we disagree is where we cut the line on what counts as ui features. I wouldn't count these changes as that, as it really just groups the code differently which is an important step on supporting themes, but IMHO not a ui change itself. I would also argue that even a limited testing by merging is better than no testing until it is a larger change. We don't need to agree, I can accept that answer as is. Often there isn't really an objective place to draw a line, but it has to be done nonetheless. How would you like to continue? Should I ping you here when each additional step is completed or would you prefer to only get an update when the reedline part is done? Or even only once the reedline and nushell parts are completed? |
|
Maybe start with a todo list and ping me on each item? I'm up for whatever is the easiest usually. |
|
@adaschma are you still in on this? i was testing this PR and don't see any problems right now. |
|
Hi @fdncred I haven't worked on this since. I had started some work started on the vi-mode, but even that I stopped working on about two months ago. I had some urgent life todos and I haven't been in the right head space for programming on reedline, since. I have started some other work, that I plan on finishing next week before giving reedline my full attention again. I still think it can be merged as is. |
|
Sounds like a fine plan. Hope all is well on your urgent life todos. Glad to have you around. |
In #800 (comment) @fdncred said:
This pull request groups the statically defined colors into a single record. This is the first step if we would want to support configurable colors at a later time. In addition it changes the fg color of selected text to black so there is a contrast to the bg color. This improves the readability and it looks nicer.
The colors of the menu and the highlighter have not been moved to the theme record. It is at least theoretically possible to have different menus and highlighters with different colors and I conservatively didn't want to ruin that.