feat(ui): Add use_selection_fg flag to control selection foreground color

[Upsylonbare]

When set to False (default True), keep the foreground color of the selected object (commit hash, date, etc ...).

It changes the following:

• Added a new use_selection_fg flag to the Theme struct, allowing users to toggle whether the selection foreground color is applied.
• Set use_selection_fg to true by default in Theme::Default.
• Extended unit tests to validate the behavior of use_selection_fg.

I followed the checklist:

• I added unittests
• I ran make check without errors
• I tested the overall application
• I added an appropriate item to the changelog

[extrawurst]

@Upsylonbare can you elaborate on what the motivation for this change is? Maybe a screenshot of the old vs. new. to help illustrate the issue you try to solve

[Upsylonbare]

Sure, with default selection_bg (Blue):

here is the before :

and after : (with use_selection_fg to false)

When using darker selection_bg, this is more useful
Without

With:

[naseschwarz]

Thank you for your PR. I've tested this and it works nicely.

Please consider adding an entry to CHANGELOG.md.

[vlad-anger]

@Upsylonbare I also would like to keep fg colors same on selection,
good PR!
Can we do better without introducing extra use_selection_fg?
If make selection_fg optional, no extra fields required

For theme.ron that will mean doing
selection_fg: Some(None) (to override default)

And default impl. in code:
selection_fg: Some(Color::White)
So we keep previous behavior

Here's patch implementing that based on your commits,
let me know what you think:
https://gist.github.com/vlad-anger/a5fcd217fad7c68459980618e375d806

[naseschwarz]

Here's patch implementing that based on your commits, let me know what you think:

Wouldn't this break user's theme configurations as it would change the selection_fg type?

[vlad-anger]

Wouldn't this break user's theme configurations as it would change the selection_fg type

Correct, it will.
Only for users, who define custom theme and they
specified custom selection_fg - will be required
to do minor tweak.

Advantage is simplicity, logic incapsulated within one field

Maybe with my approach selection_fg: Some(Some(your_color))
(ehh tha'ts because of how Patch struct works, we already need to wrap in Some)
will look a bit outstanding of other colors config
Otherwise we need to take extra field for extra logic
branch of selection_fg, which also feels a bit much
So i'm not sure

[extrawurst]

Breaking is fine as long as we document it properly in the changelog.

[cruessler]

Another option: we already have more than one code path for loading themes, so adding another one could potentially automate the migration. I’m all for making users’ lives easier, but I also understand if we don’t want to increase code complexity in this instance.

gitui/src/ui/style.rs

Lines 304 to 324 in 232ad89

	if let Ok(patch) = Self::load_patch(theme_path).map_err(|e| {
	log::error!("theme error [{:?}]: {e}", theme_path);
	e
	}) {
	theme.apply(patch);
	} else if let Ok(old_theme) = Self::load_old_theme(theme_path)
	{
	theme = old_theme;
	if theme.save_patch(theme_path).is_ok() {
	log::info!(
	"Converted old theme to new format. ({:?})",
	theme_path
	);
	} else {
	log::warn!(
	"Failed to save theme in new format. ({:?})",
	theme_path
	);
	}
	}

[Upsylonbare]

Another option: we already have more than one code path for loading themes, so adding another one could potentially automate the migration. I’m all for making users’ lives easier, but I also understand if we don’t want to increase code complexity in this instance.

gitui/src/ui/style.rs

Lines 304 to 324 in 232ad89

	if let Ok(patch) = Self::load_patch(theme_path).map_err(|e| {
	log::error!("theme error [{:?}]: {e}", theme_path);
	e
	}) {
	theme.apply(patch);
	} else if let Ok(old_theme) = Self::load_old_theme(theme_path)
	{
	theme = old_theme;
	if theme.save_patch(theme_path).is_ok() {
	log::info!(
	"Converted old theme to new format. ({:?})",
	theme_path
	);
	} else {
	log::warn!(
	"Failed to save theme in new format. ({:?})",
	theme_path
	);
	}
	}

Hi, I don't get your point here. To me the apply function you are referring is the one that load the ron theme into a theme struct. My patches only use this theme struct while rendering the line. But I may be wrong.
Can you share more details about what you are meaning ?

[Upsylonbare]

@vlad-anger I firstly thought the feature the way you wrote it in your gist but as said it breaks retrocompatibility with 'old' theme.
As @extrawurst accept it, I can rewrite the PR if everyone is fine with it.

[vlad-anger]

Honestly i'm not sure what's better
selection_fg: Some(Some(your_color)) will also look a bit weird in theme config,
because of how patchy works, while having extra field also not perfect : )

[Upsylonbare]

Honestly i'm not sure what's better selection_fg: Some(Some(your_color)) will also look a bit weird in theme config, because of how patchy works, while having extra field also not perfect : )

Yes you're right.
To me this can be merge as is at it is a more verbose way to write theme and don't introduce messy writings in theme's file. It seems more "simpler" to the user. The "Some()" uses in the theme file will surely evolve in the future though. I think this kind of writing doesn't please anyone but we can't change it for now.
I'm the kind of guy that personalize a lot its tools so will surely do other PR in the style.rs file. I can keep your remarks in a corner of my head.

[extrawurst]

@Upsylonbare please resolve conflicts.
@naseschwarz can I get a final review and merge_ready if you think its a go?

[Upsylonbare]

Sorry first time I deal with Github PR, I think I've clicked on the wrong button

[extrawurst]

Thank you guys!