Differential D28076

[GTK3] Add module to reload colorscheme in GTK3 apps without restarting them
ClosedPublic
Actions

Authored by cblack on Mar 16 2020, 2:01 PM.

Details

Reviewers
gikari
Group Reviewers
Plasma
Commits
R99:306e01d86ab9: [GTK3] Add module to reload colorscheme in GTK3 apps without restarting them
R99:6f01f8040007: [GTK3] Add module to reload colorscheme in GTK3 apps without restarting them
Summary

The title is somewhat self explanatory.

Test Plan

gtk-app-here --gtk-module colorreload-gtk-module

Diff Detail

Repository
R99 KDE Gtk Configuration Tool
Branch
color-reload-module (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23759
Build 23777: arc lint + arc unit
cblack requested review of this revision.Mar 16 2020, 2:01 PM
cblack created this revision.
cblack updated this revision to Diff 77730.Mar 16 2020, 2:02 PM

Drop extraneous meson.build

cblack updated this revision to Diff 77731.Mar 16 2020, 2:05 PM

Fix memory leak

cblack updated this revision to Diff 77733.Mar 16 2020, 2:16 PM

Replace 801 with something more semantically meaningful

gikari requested changes to this revision.Mar 16 2020, 2:18 PM
gikari added a subscriber: gikari.
gikari added inline comments.
color-reload-module/CMakeLists.txt
1

Would you mind setting this at the parent CMakeLists.txt?

2

Would you mind making this as a subproject?

10

Also, could you remove spaces around parentaces to match the CMake style with the one, that is in kded and kconf_update directories?

14

Isn't the PUBLIC/PRIVATE modifiers a good modern CMake practice? The same for target_include_directories

This revision now requires changes to proceed.Mar 16 2020, 2:18 PM
cblack updated this revision to Diff 77739.Mar 16 2020, 2:47 PM
cblack marked 4 inline comments as done.

Address feedback

gikari added inline comments.Mar 16 2020, 3:01 PM
color-reload-module/CMakeLists.txt
10 ↗(On Diff #77730)
add_library(colorreload-gtk-module SHARED
    reloader.c
)
15 ↗(On Diff #77730)

Sorry, this is a nitpick, but could you please make this look like this:

target_link_libraries(colorreload-gtk-module
  PRIVATE
    ${GLIB_LIBRARIES}
    ${GTK_LIBRARIES}
gikari added inline comments.Mar 16 2020, 3:05 PM
color-reload-module/CMakeLists.txt
2 ↗(On Diff #77730)

Why is this needed?

4 ↗(On Diff #77730)

Move to parent CMakeLists.txt

5 ↗(On Diff #77730)

Same

7 ↗(On Diff #77730)

Remove space before ( and I guess this could also go to parent CMakeLists.txt?

cblack added inline comments.Mar 16 2020, 3:09 PM
color-reload-module/CMakeLists.txt
2

That's a Meson thing, not a CMake thing. CMake just has add_subdirectory, afaik.

cblack updated this revision to Diff 77740.Mar 16 2020, 3:10 PM
cblack marked 7 inline comments as done.

Address feedback

gikari accepted this revision.Mar 16 2020, 3:28 PM

Nice! Does it need to be manually passed to GTK apps when testing D28072?

This revision is now accepted and ready to land.Mar 16 2020, 3:28 PM
gikari added inline comments.Mar 16 2020, 3:30 PM
CMakeLists.txt
3 ↗(On Diff #77730)

Extra space

cblack added a comment.Mar 16 2020, 6:23 PM
In D28076#628452, @gikari wrote:

Nice! Does it need to be manually passed to GTK apps when testing D28072?

GTK needs to be configured to load the GTK module colorreload-gtk-module.

cblack updated this revision to Diff 77764.Mar 16 2020, 6:24 PM

Minor formatting improvements

gikari added a comment.Mar 16 2020, 6:28 PM

GTK needs to be configured to load the GTK module colorreload-gtk-module.

You mean we need to add gtk-modules=colorreload-gtk-module to settings.ini? And how to add this module to autoload in Wayland?

cblack added a comment.Mar 16 2020, 6:49 PM
In D28076#628617, @gikari wrote:

GTK needs to be configured to load the GTK module colorreload-gtk-module.

You mean we need to add gtk-modules=colorreload-gtk-module to settings.ini? And how to add this module to autoload in Wayland?

Expose modules on the Modules property of org.gtk.Settings via DBUS.

Closed by commit R99:306e01d86ab9: [GTK3] Add module to reload colorscheme in GTK3 apps without restarting them (authored by cblack). · Explain WhyMar 16 2020, 6:52 PM
This revision was automatically updated to reflect the committed changes.
gikari added a comment.Mar 16 2020, 8:44 PM

Seems like I've made a mistake. This does not build.

/home/gikari/kde/src/kde-gtk-config/color-reload-module/reloader.c:21:10: fatal error: gtk/gtk.h: No such file or directory
   21 | #include <gtk/gtk.h>
      |          ^~
cblack added a comment.Mar 16 2020, 8:56 PM
In D28076#628706, @gikari wrote:

Seems like I've made a mistake. This does not build.

/home/gikari/kde/src/kde-gtk-config/color-reload-module/reloader.c:21:10: fatal error: gtk/gtk.h: No such file or directory
   21 | #include <gtk/gtk.h>
      |          ^~

Builds for me.

gikari added a comment.Mar 16 2020, 10:03 PM

I tried to change these ones to GTK3_LIBRARY and GLIB2_LIBRARY, but now it throws this:

/home/gikari/kde/src/kde-gtk-config/color-reload-module/reloader.c:37: undefined reference to `g_type_check_instance_cast'
color-reload-module/CMakeLists.txt
11 ↗(On Diff #77730)

It seems like you use other variables, than in other parts of kde-gtk-config.
In other places it uses GTK3_LIBRARY and GLIB2_LIBRARY variables from find module. The versions also appear in target_include_directories command.

cblack added a comment.Mar 16 2020, 10:37 PM
In D28076#628760, @gikari wrote:

I tried to change these ones to GTK3_LIBRARY and GLIB2_LIBRARY, but now it throws this:

/home/gikari/kde/src/kde-gtk-config/color-reload-module/reloader.c:37: undefined reference to `g_type_check_instance_cast'

Looks like my cached version was behaving properly, while a clean build wasn't. Investigating.

bcooksley added a subscriber: bcooksley.Mar 17 2020, 8:38 AM

This change, and changes subsequent to it have broken the build of kde-gtk-config.

Please see https://build.kde.org/view/Failing/job/Plasma/job/kde-gtk-config/job/kf5-qt5%20SUSEQt5.12/ and https://build.kde.org/view/Failing/job/Plasma/job/kde-gtk-config/job/kf5-qt5%20FreeBSDQt5.13/

Plasma developers - this is one of several breakages which have occurred over the last couple of days which you have failed to address in a timely fashion.
Should this behaviour continue then CI coverage for Plasma will be discontinued and Plasma repositories will be blacklisted from the CI system and be unable to be used by other KDE software projects.

cblack reopened this revision.Mar 17 2020, 6:35 PM
This revision is now accepted and ready to land.Mar 17 2020, 6:35 PM
cblack updated this revision to Diff 77857.Mar 17 2020, 6:35 PM

Rebase on master

cblack updated this revision to Diff 77858.Mar 17 2020, 6:38 PM

Update CMakeLists.txt

cblack updated this revision to Diff 77862.Mar 17 2020, 7:22 PM

Fix bad symbol management

gikari added inline comments.Mar 17 2020, 9:57 PM
color-reload-module/CMakeLists.txt
4 ↗(On Diff #77730)

Why is this needed? Aren't the CMake default languages in project (parent CMakeLists) both CXX and C?

color-reload-module/reloader.c
36 ↗(On Diff #77730)

This is hardcoded directory. The $XDG_CONFIG_HOME may be set to something other, not only the default.

cblack updated this revision to Diff 77871.Mar 17 2020, 11:50 PM

Address feedback

gikari added inline comments.Mar 27 2020, 5:11 PM
color-reload-module/CMakeLists.txt
2 ↗(On Diff #77730)

May be this could be moved to parent CMakeLists with other includes?

cblack updated this revision to Diff 78804.Mar 29 2020, 4:52 PM

Use KDE_INSTALL_LIBDIR

cblack marked 5 inline comments as done.Mar 29 2020, 10:15 PM
gikari accepted this revision.Mar 29 2020, 10:17 PM
Closed by commit R99:6f01f8040007: [GTK3] Add module to reload colorscheme in GTK3 apps without restarting them (authored by cblack). · Explain WhyMar 29 2020, 10:23 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 77731

View Options

CMakeLists.txt

Loading...
View Options

color-reload-module/CMakeLists.txt

Loading...
View Options

color-reload-module/reloader.c

Loading...