Skip to content
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

lighttable: colors customizable through CSS #1897

Closed
wants to merge 6 commits into from

Conversation

moy
Copy link
Contributor

@moy moy commented Dec 11, 2018

Most colors used to display thumbnails were hardcoded values. For a
white background theme, the colors were rather dark. With this patch,
one can get a light theme for the lighttable with eg.

@define-color thumbnail_bg_color #eee;
@define-color thumbnail_selected_bg_color shade(@thumbnail_bg_color, .9);
@define-color thumbnail_hover_bg_color shade(@thumbnail_bg_color, .8);
@define-color thumbnail_outline_color #7f7f7f;
@define-color thumbnail_selected_outline_color shade(@thumbnail_outline_color, .8);
@define-color thumbnail_hover_outline_color shade(@thumbnail_outline_color, .6);
@define-color thumbnail_font_color #aaa;
@define-color thumbnail_selected_font_color shade(@thumbnail_font_color, .8);
@define-color thumbnail_hover_font_color shade(@thumbnail_font_color, .6);
@define-color thumbnail_border_color #222;
@define-color thumbnail_selected_border_color #eee;

Screenshot:
light

src/gui/gtk.c Outdated
@@ -1184,6 +1184,41 @@ int dt_gui_gtk_init(dt_gui_gtk_t *gui)
c[DT_GUI_COLOR_BRUSH_TRACE] = (GdkRGBA){ 0., 0., 0., 0.8 };
gtk_style_context_lookup_color(ctx, "brush_trace", &c[DT_GUI_COLOR_BRUSH_TRACE]);

c[DT_GUI_COLOR_THUMBNAIL_BG] = (GdkRGBA){ 0.4, 0.4, 0.4, 1.0 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct, it may or may not work as gtk_style_context_lookup_color() does not say that the color is not changed if not found. In dt we currently do:

if(!gtk_style_context_lookup_color(ctx, "thumbnail_bg_color", &c[DT_GUI_COLOR_THUMBNAIL_BG]))
   c[DT_GUI_COLOR_THUMBNAIL_BG] = (GdkRGBA){ 0.4, 0.4, 0.4, 1.0 };

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice it does work at least for me, but why not be more strict. We'll need to update the similar code right above too (I'm guilty for it too). Actually, I think it's time to introduce a helper function to make this a one-liner. Will do later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@TurboGit TurboGit self-assigned this Dec 11, 2018
@TurboGit TurboGit added the feature: enhancement current features to improve label Dec 11, 2018
@TurboGit TurboGit added this to the 2.6.1 milestone Dec 11, 2018
@TurboGit
Copy link
Member

Sounds good now! I plan to merge in 2.6.1, ok for you?

@moy
Copy link
Contributor Author

moy commented Dec 12, 2018

I plan to merge in 2.6.1, ok for you?

Yes. It should be harmless change, but better resist the temptation to merge it before 2.6, just in case.

The previous code was using:

  c[...] = defaut;
  gtk_style_context_lookup_color(..., &c[...]);

This relied on the fact that c[...] is not modified by
gtk_style_context_lookup_color when the color isn't found in the CSS.
This isn't guaranteed, so the construct wasn't safe.

Change this to explicitly assign the default color when
gtk_style_context_lookup_color() fails.
Most colors used to display thumbnails were hardcoded values. For a
white background theme, the colors were rather dark. With this patch,
one can get a light theme for the lighttable with eg.

@define-color thumbnail_bg_color #eee;
@define-color thumbnail_selected_bg_color shade(@thumbnail_bg_color, .9);
@define-color thumbnail_hover_bg_color shade(@thumbnail_bg_color, .8);
@define-color thumbnail_outline_color #7f7f7f;
@define-color thumbnail_selected_outline_color shade(@thumbnail_outline_color, .8);
@define-color thumbnail_hover_outline_color shade(@thumbnail_outline_color, .6);
@define-color thumbnail_font_color #aaa;
@define-color thumbnail_selected_font_color shade(@thumbnail_font_color, .8);
@define-color thumbnail_hover_font_color shade(@thumbnail_font_color, .6);
@define-color thumbnail_border_color darktable-org#222;
@define-color thumbnail_selected_border_color #eee;
The + button was re-using the +/- button from parametric masks,
forcing a +-shape using CPF_ACTIVE. This has the bad side effect that
the button was always drawn as CPF_PRELIGHT, i.e. as if the mouse
pointer was hovering the button. In particular, this is inconsistant
with the "multiple instance" button next to it.

Remove the CPF_ACTIVE from the button's definition, and only activate
it while drawing the shape.
The cairo_set_source_rgba was useless, since the foreground color is
already set while drawing the button. It was harmful because it
disabled any manual CSS customization for the +/- button.
This makes the switch more consistent with the preset, reset and
multiple instance buttons right next.

This makes the code of togglebutton a bit closer to the one of button.
@moy
Copy link
Contributor Author

moy commented Dec 21, 2018

I've pushed a few more commits & rebased. All of them can wait for 2.6.1. The last 3 commits are not directly CSS-related, let me know if you prefer a separate PR.

@TurboGit
Copy link
Member

conflict resolved and manually merged.

@TurboGit TurboGit closed this Dec 26, 2018
[DT_GUI_COLOR_FILMSTRIP_BG] = { "filmstrip_bg_color", { 0.2, 0.2, 0.2, 1.0 } },
};

for(int i = 0; i < DT_GUI_COLOR_LAST; i++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just spotted this mistake. The index i start to 0, first item name is "darkroom_bg_color" but it is set into the c[0] which is actually DT_GUI_COLOR_BG. And since the loop is going up to DT_GUI_COLOR_LAST-1 then we read last element in the struct which is not set.

Can you have a look?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 46b1d45

@moy
Copy link
Contributor Author

moy commented Dec 26, 2018

Thanks for the fix. Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: enhancement current features to improve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants