feat(themes; beautiful): add partial support for GTK+3 themes #2129

actionless · 2017-11-29T17:05:37Z

codecov · 2017-11-29T17:27:40Z

Codecov Report

Merging #2129 into master will increase coverage by 0.12%.
The diff coverage is 93.2%.

@@            Coverage Diff             @@
##           master    #2129      +/-   ##
==========================================
+ Coverage   83.83%   83.95%   +0.12%     
==========================================
  Files         464      466       +2     
  Lines       31798    32151     +353     
==========================================
+ Hits        26657    26993     +336     
- Misses       5141     5158      +17

Flag	Coverage Δ
#c_code	`72.6% <ø> (+0.01%)`	⬆️
#functionaltests	`72.35% <93.2%> (+0.45%)`	⬆️
#lua53	`87.14% <93.2%> (+0.11%)`	⬆️
#samples	`70.89% <ø> (ø)`	⬆️
#unittests	`59.37% <7.63%> (-0.83%)`	⬇️

Impacted Files	Coverage Δ
lib/beautiful/init.lua	`82.5% <100%> (+0.44%)`	⬆️
themes/gtk/theme.lua	`92.81% <92.81%> (ø)`
lib/beautiful/gtk.lua	`93.52% <93.52%> (ø)`
xwindow.c	`92.39% <0%> (-0.55%)`	⬇️
objects/client.c	`79.77% <0%> (-0.17%)`	⬇️
lib/awful/wibar.lua	`81.08% <0%> (+0.67%)`	⬆️
lib/wibox/widget/textbox.lua	`93.66% <0%> (+0.7%)`	⬆️
lib/awful/menu.lua	`50.84% <0%> (+0.72%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25adaae...73d3de6. Read the comment docs.

actionless · 2017-11-29T18:30:47Z

weird, locally test is passing however in travis style_context:get_font() and :get_property() are working as expected but not :lookup_color()

psychon · 2017-11-30T08:37:15Z

== Running /home/travis/build/awesomeWM/awesome/tests/test-awful-client.lua ==
[...]
awesome: lvm.c:436: luaV_equalobj: Assertion `(((t1)->value_).gc)->tt == 5' failed.
2017-11-29 21:41:24 E: awesome: signal_fatal:448: signal 6, dumping backtrace

Uhm... I've seen that error before recently, on some other PR. Ah, #2119 (comment)
So... yay, we got something randomly failing again. :-(

psychon

Basically "no". This code pulls a random list of colors from a random list of Gtk widgets without saying what it does in its docs, and then randomly messes with the resulting colors in a magic way to produce a color theme.

Claiming that this reads a GTK+3 theme seems a lot like a lie to me.

Anyway, I left lots of detailed comments.

psychon · 2017-11-30T08:39:27Z

.travis.yml

@@ -42,7 +42,7 @@ install:
  - sudo apt-get install -y libcairo2-dev gir1.2-gtk-3.0 xmlto asciidoc libpango1.0-dev libxcb-xtest0-dev libxcb-icccm4-dev libxcb-randr0-dev libxcb-keysyms1-dev libxcb-xinerama0-dev libdbus-1-dev libxdg-basedir-dev libstartup-notification0-dev imagemagick libxcb1-dev libxcb-shape0-dev libxcb-util0-dev libx11-xcb-dev libxcb-cursor-dev libxcb-xkb-dev libxkbcommon-dev libxkbcommon-x11-dev

  # Deps for tests.
-  - sudo apt-get install -y dbus-x11 xterm xdotool xterm xvfb zsh
+  - sudo apt-get install -y dbus-x11 xterm xdotool xterm xvfb zsh gtk+3.0


Uhm, what?
From the build log:

Note, selecting 'libgtk3.0-cil-dev' for regex 'gtk+3.0' Note, selecting 'monodoc-gtk3.0-manual' for regex 'gtk+3.0' Note, selecting 'libgtk3.0-cil' for regex 'gtk+3.0' Note, selecting 'gir1.2-dbusmenu-gtk3-0.4' for regex 'gtk+3.0' Note, selecting 'libinfgtk3-0.5-0' for regex 'gtk+3.0' Note, selecting 'libseed-gtk3-0' for regex 'gtk+3.0' Note, selecting 'libavahi-ui-gtk3-0' for regex 'gtk+3.0' Note, selecting 'libwxgtk3.0-dev' for regex 'gtk+3.0' Note, selecting 'libcanberra-gtk3-0' for regex 'gtk+3.0' Note, selecting 'libwxgtk3.0-0' for regex 'gtk+3.0' Note, selecting 'libwxgtk3.0-0-dbg' for regex 'gtk+3.0' Note, selecting 'libcanberra-gtk3-0-dbg' for regex 'gtk+3.0' libcanberra-gtk3-0 is already the newest version (0.30-0ubuntu3). libcanberra-gtk3-0 set to manually installed.

Also see commit 98b931c.

psychon · 2017-11-30T08:41:53Z

themes/gtk/theme.lua

+
+theme.bg_normal     = theme.gtk.menubar_bg_color
+theme.bg_focus      = theme.gtk.selected_bg_color
+theme.bg_urgent     = theme.xrdb.color9


So this mixes the GTK theme with "random" values from xrdb? Why should the result end up being a "nice" color scheme?

so far it's taking from xrdb only colors for errors, i can also try to render some kind of GtkButton with "Destroyable" role (ie red one) and use color from it

That's not necessary, as error and warning colors are exported.

since in majority of other gtk themes stylecontext values are partially missing, destructive/suggested/etc-action-approach would be more robust

psychon · 2017-11-30T08:43:01Z

themes/gtk/theme.lua

+theme.bg_focus      = theme.gtk.selected_bg_color
+theme.bg_urgent     = theme.xrdb.color9
+theme.bg_minimize   = theme.gtk.wm_border_unfocused_color
+theme.bg_systray    = theme.bg_normal


Just remove this. systray.lua does beautiful.bg_systray or beautiful.bg_normal. In the other theme this might serve documentary purposes for people who want to hand-edit it, but I guess this one isn't meant for hand-editing...?

i think it's still leaving for user a lot of opportunity to customize, in this particular example — to choose for systray gtk.bg_color, gtk.base_color or gtk.menubar_bg_color, etc

psychon · 2017-11-30T08:44:39Z

themes/gtk/theme.lua

+theme.fg_normal     = theme.gtk.menubar_fg_color
+theme.fg_focus      = theme.gtk.selected_fg_color
+theme.fg_urgent     = theme.bg_normal
+theme.fg_minimize   = theme.bg_normal


This seems quite random, too. Urgent / minimised clients get menubar_bg_color as their foreground color and color9 / wm_border_unfocused_color as their background color? That's not how these colors are supposed to be used, I think.

psychon · 2017-11-30T08:49:24Z

themes/gtk/theme.lua

+        local bg_numeric_value = tonumber("0x"..s) - darker_n
+        if bg_numeric_value < 0 then bg_numeric_value = 0 end
+        if bg_numeric_value > 255 then bg_numeric_value = 255 end
+        result = result .. string.format("%2.2x", bg_numeric_value)


What's the meaning of this format string? From looking at man 3 printf I... still have no clue. What's the difference to %02x?

psychon · 2017-11-30T09:08:39Z

lib/beautiful/gtk.lua

+  --window:set_override_redirect(true)
+  local style_context = window:get_style_context()
+
+  local font = style_context:get_font(Gtk.StateFlags.NORMAL)


This function is deprecated: https://developer.gnome.org/gtk3/stable/GtkStyleContext.html#gtk-style-context-get-font
(And no, I don't understand the replacement nor how to use it)

psychon · 2017-11-30T09:08:52Z

lib/beautiful/gtk.lua

+
+  local font = style_context:get_font(Gtk.StateFlags.NORMAL)
+  --result.font_family = font:get_family()
+  --result.font_size = font:get_size()/1024


There seems to be a lot of "you should clean this up" around here...

it's actually related to your other coment regarding deprecation of :get_font()

psychon · 2017-11-30T09:10:31Z

lib/beautiful/gtk.lua

+        entry_style_context, style_context_property, Gtk.StateFlags.NORMAL
+    )
+  end
+  entry:destroy()


I think the docs for this module should say what this actually does... So the bg/fg_colors come from the background-color and color of a GtkWindow, the border width is the border-top-width of a GtkEntry and the base_color and text_color come from the background and foreground color of a GtkEntry?!?

right, if you take a look on gtk3-widget-factory (or any other gtk3 app) you will see what stylecontext values bg/fg are used for windows, text/base - for entries

however if the same values are defined in stylecontext, stylecontext have a higher prority in my code (because of pixmaps/gradients issue described in the previous comment)

psychon · 2017-11-30T09:12:05Z

lib/beautiful/gtk.lua

+        entry_style_context, style_context_property, Gtk.StateFlags.NORMAL
+    )
+  end
+  entry:destroy()


Also, I doubt that this entry:destroy() is necessary. And from the docs for it... I doubt this even more, I think this does not actually do anything (besides setting a destroyed flag).
https://developer.gnome.org/gtk3/stable/GtkWidget.html#gtk-widget-destroy

psychon · 2017-11-30T09:14:21Z

lib/beautiful/gtk.lua

+        button_style_context, style_context_property, Gtk.StateFlags.NORMAL
+    )
+  end
+  button:destroy()


Can the above code be made more readable by e.g. moving it into a table?

{ object = Gtk.Button(), properties = { Gtk.StateFlags.NORMAL = { border_radius = "border-radiues", etc } }

That way, all that code is only needed once instead of copy&pasted three times.

psychon · 2017-11-30T10:27:29Z

Perhaps this makes more sense as a self-contained tool? It does all the magic that is done here and then writes (either on stdout or into a file) a theme for awesome? That way people can hand-edit the result if they want.

psychon · 2018-05-29T10:46:20Z

lib/beautiful/gtk.lua

+    ))
+    local label = Gtk.Label()
+    local label_style_context = label:get_style_context()
+    local gdk_scale = os.getenv("GDK_SCALE") or 1.0


Doesn't this need a tonumber()?

psychon · 2018-05-29T10:47:38Z

lib/beautiful/gtk.lua

+    local label = Gtk.Label()
+    local label_style_context = label:get_style_context()
+    local gdk_scale = os.getenv("GDK_SCALE") or 1.0
+    local xrdb_scale = dpi(10000) / 10000


Uhm. Why not just beautiful.xresources.get_dpi() / 96?

psychon · 2018-05-29T10:49:56Z

themes/gtk/theme.lua

+    return darker(color, is_dark(color) and -ratio or ratio)
+end
+
+local function choose_contrast_color(reference, candidate1, candidate2)  -- luacheck: no unused


Why is the no unused needed?

because the function itself is not used in the code, but only in the theming examples in the comments

psychon · 2018-05-29T10:51:24Z

themes/gtk/theme.lua

+         filter  = awful.widget.tasklist.filter.currenttags,
+-        buttons = tasklist_buttons,
+-        widget_template = beautiful.tasklist_widget_template
+        buttons = tasklist_buttons


Uhm, is the patch backwards?

psychon · 2018-05-29T10:51:50Z

themes/gtk/theme.lua

+    },
+    id     = 'background_role',
+    widget = wibox.container.background,
+    create_callback = function(self, c, index, objects) --luacheck: no unused args


Just make this function(_, c) and you do not need the luacheck comment

psychon · 2018-05-29T10:52:13Z

themes/gtk/theme.lua

+)
+
+theme.wibar_bgimage = theme.gtk.menubar_bg_image
+--print(theme.wibar_bgimage)


oops, those are leftovers from the next version, which support pixmap-ish themes better, but it wil take some time to come.. :-D

psychon

The commit message could need some rewriting and there seem to be unrelated changes in there, but the rest does not seem tooooo bad. I am not sure all of this makes sense, but since it is just in a theme I won't look too closely.

actionless · 2018-05-29T23:32:10Z

squashed + sanitized the commit message

actionless · 2018-06-26T13:48:20Z

@Elv13 @blueyed it still needs one more review

Elv13

Looks like all my older comments have been addressed. +1

Elv13 · 2018-06-26T14:22:00Z

@actionless oh, an you might want to update the optional dependency section of README.md

I have updated the NEWS changelog for 4.3, does this description is fine with you (or propose something better):

### A new GTK color palette based theme

A new theme has been added. It reads the GTK theme colors and use them in the
`wibar`, `menu` and `titlebar`. It helps create an uniform look and feel
between the window manager and client applications with minimal efforts.

actionless · 2018-06-26T14:25:02Z

actually it reads not only colors but also border thickness and shape and few more things, however for those things support is very partial so mb your description will be fine (so users won't be expecting too much from it :-) )

yup, i'll update the readme. idk which is the minimal supported gtk3 version but the one installed inside travis seems to be compatible enough :-)

actionless · 2018-06-26T14:26:59Z

seems at least 3.10 is supported:

https://travis-ci.org/awesomeWM/awesome/jobs/396908930#L520

actionless · 2018-06-26T14:32:08Z

actionless dismissed stale reviews from Elv13 and psychon via c660893 just now

fuck :-(

Elv13 · 2018-06-26T14:36:36Z

Do you want me to merge it now? Psychon is away until mid August so the review wont be updated.

actionless · 2018-06-26T14:38:53Z

i've been testing gtk.lua code (not theme itself) for more than a year so it should be quite robust

last issue which i found and resolved (about 1-2 months ago) was what it wasn't working properly in some edge cases of GTK+3 scaling + xrdb dpi

Elv13 · 2018-06-26T14:42:05Z

i've been testing gtk.lua code (not theme itself) for more than a year so it should be quite robust

I have been testing the new layout stuff for years too, we all have heaps of unmerged code ;)

So do I press the button or not? Your call.

actionless · 2018-06-26T14:42:34Z

yes, it's good to merge

Elv13 · 2018-06-26T14:43:43Z

🥇

actionless · 2018-06-26T14:43:48Z

thanks! 🎉

actionless mentioned this pull request Nov 29, 2017

RFC: make awesome look like a current GTK+ 3 theme #1560

Closed

actionless force-pushed the gtk-theme branch 2 times, most recently from b60d204 to d3c78d9 Compare November 29, 2017 17:17

awesomeWM deleted a comment from codecov bot Nov 29, 2017

actionless force-pushed the gtk-theme branch from d3c78d9 to 6c47698 Compare November 29, 2017 17:27

actionless force-pushed the gtk-theme branch 5 times, most recently from 65eda3d to 2a17cfc Compare November 29, 2017 18:29

actionless force-pushed the gtk-theme branch 11 times, most recently from 560db39 to edc1fe2 Compare November 29, 2017 20:54

actionless requested review from blueyed, psychon and Elv13 November 29, 2017 21:10

actionless force-pushed the gtk-theme branch from 5b62f9e to 6e59d7b Compare November 29, 2017 21:36

psychon requested changes Nov 30, 2017

View reviewed changes

psychon reviewed May 29, 2018

View reviewed changes

psychon previously approved these changes May 29, 2018

View reviewed changes

actionless force-pushed the gtk-theme branch from ccd56b9 to fb843e7 Compare May 29, 2018 19:53

actionless added this to the next: pull requests to be merged soon milestone May 29, 2018

actionless force-pushed the gtk-theme branch from fb843e7 to 289868b Compare May 29, 2018 23:16

actionless mentioned this pull request May 29, 2018

Enable Mergify for dev workflow #2260

Closed

Elv13 previously approved these changes Jun 26, 2018

View reviewed changes

actionless dismissed stale reviews from Elv13 and psychon via c660893 June 26, 2018 14:31

feat(themes; beautiful): add partial support for GTK+3 themes

73d3de6

actionless force-pushed the gtk-theme branch from c660893 to 73d3de6 Compare June 26, 2018 14:36

Elv13 merged commit 4bd5b19 into awesomeWM:master Jun 26, 2018

actionless deleted the gtk-theme branch June 26, 2018 14:43

feat(themes; beautiful): add partial support for GTK+3 themes #2129

feat(themes; beautiful): add partial support for GTK+3 themes #2129

Conversation

actionless commented Nov 29, 2017

codecov bot commented Nov 29, 2017 • edited Loading

Codecov Report

actionless commented Nov 29, 2017

psychon commented Nov 30, 2017

psychon left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

actionless Dec 3, 2017 • edited Loading

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

Choose a reason for hiding this comment

actionless Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psychon commented Nov 30, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psychon left a comment

Choose a reason for hiding this comment

actionless commented May 29, 2018

actionless commented Jun 26, 2018

Elv13 left a comment

Choose a reason for hiding this comment

Elv13 commented Jun 26, 2018

actionless commented Jun 26, 2018

actionless commented Jun 26, 2018

actionless commented Jun 26, 2018

Elv13 commented Jun 26, 2018

actionless commented Jun 26, 2018

Elv13 commented Jun 26, 2018

actionless commented Jun 26, 2018

Elv13 commented Jun 26, 2018

actionless commented Jun 26, 2018

codecov bot commented Nov 29, 2017 •

edited

Loading

actionless Dec 3, 2017 •

edited

Loading

actionless Nov 30, 2017 •

edited

Loading