Set a font for gtk applications in the fonts kcm
Needs ReviewPublic

Authored by gikari on Jun 16 2019, 7:43 PM.

Details

Summary

To increase usability, fonts for gtk applications is now set in respective fonts kcm, instead of separate gtk kcm.

Not sure if we should provide option to modify font only for gtk applications: why would anyone need to set different fonts for gnome and kde apps?

Note: if fonts configuration option will be removed from kde-gtk-config, this patch will be needed (D21524), so gtk2rc file won't be overridden without font string.

Depends on D21524

BUG: 138866

Test Plan
  1. Remove these strings from ~/.gtkrc-2.0. They are no longer generated.
include "/usr/share/themes/Breeze/gtk-2.0/gtkrc"
style "user-font" 
{
	font_name="Noto Sans"
}
widget_class "*" style "user-font"
  1. To test gtk3 applications reloading on X11, install xsettingsd.
  2. After installing, restart kded5.
  3. Open gtk2 app, gtk3 app and open font settings.
  4. Change General Font and check if earlier opened applications are changing their font after hitting Apply
  5. Reopen gtk applications to check if configuration is applied

Diff Detail

Repository
R119 Plasma Desktop
Branch
set-gtk-font-in-fonts-kcm (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15636
Build 15654: arc lint + arc unit
gikari created this revision.Jun 16 2019, 7:43 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 16 2019, 7:43 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gikari requested review of this revision.Jun 16 2019, 7:43 PM
GB_2 added a reviewer: Plasma.Jun 16 2019, 7:55 PM

Wow, thanks very much! To answer some questions:

Not sure if we should provide option to modify font only for gtk applications: why would anyone need to set different fonts for gnome and kde apps?

Agreed. I don't think a separate control is needed. Just use the "General" font size to GTK apps.

Note: if fonts configuration option will be removed from kde-gtk-config, this patch will be needed (D21524), so gtk2rc file won't be overridden without font string.

It will be, so yes, it looks like we'll need that. I'll see if I can rustle up some reviewers!

abetts added a subscriber: abetts.Jun 17 2019, 9:32 AM

How does this look?

There is no visual change.

ngraham edited reviewers, added: apol; removed: VDG.Jun 17 2019, 1:38 PM
GB_2 added a subscriber: GB_2.Jun 17 2019, 1:42 PM

So when this lands I guess we need to remove the font option from the GTK KCM.

Messing with GTK settings INI files from various places is not a good idea imho. Also, this approach doesn't work on Wayland. Ideally, we had a daemon that monitors all settings and applies them in a central place, so the settings knobs could be where they belong.

gikari planned changes to this revision.Jun 17 2019, 9:53 PM

Messing with GTK settings INI files from various places is not a good idea imho. Also, this approach doesn't work on Wayland. Ideally, we had a daemon that monitors all settings and applies them in a central place, so the settings knobs could be where they belong.

You're right, for some reason settings.ini does not work in Wayland. However gsettings does, but dependency on gtk API disturbs me.
As for the daemon: what should it look like? Some sort of a gtk-settings-only file-watcher, that monitors Plasma config files with relevant info like fonts/cursors/etc and that changes gtk configs accordingly, whenever Plasma files changes?

However gsettings does, but dependency on gtk API disturbs me.

I think that's only a problem if every KCM ends up depending on GTK. If it's a dedicated proxy service binary, should be fine.

Some sort of a gtk-settings-only file-watcher, that monitors Plasma config files with relevant info like fonts/cursors/etc and that changes gtk configs accordingly, whenever Plasma files changes?

Pretty much. Actually, for most appearance settings (color scheme, widget style, icon settings) it should already be signalled automatically via DBus through plasma-integration with no config reading on its own.

but dependency on gtk API disturbs me.

plasma-pa also depends on GSettings, so it's not unprecedented

Actually, for most appearance settings (color scheme, widget style, icon settings) it should already be signalled automatically via DBus through plasma-integration with no config reading on its own.

I have found those lines in colorscheme config (and the similar in icon settings, but nothing in widget style):

QDBusMessage message = QDBusMessage::createSignal(QStringLiteral("/KGlobalSettings"),
                                                  QStringLiteral("org.kde.KGlobalSettings"),
                                                  QStringLiteral("notifyChange"));
message.setArguments({
    0, //previous KGlobalSettings::PaletteChanged. This is now private API in khintsettings
    0  //unused in palette changed but needed for the DBus signature
});
QDBusConnection::sessionBus().send(message);

Is this it? Should my daemon use the similar DBus signatures for fonts?

If I understand correctly, what I need to do is:

  1. Write similar to above lines in fonts kcm code (No sure about arguments actually - what they should be?)
  2. Write a daemon that listens DBus with those signatures and when specific setting changes (for now it's only fonts, then it will be cursors or something else) it changes corresponding setting in gtk via GSettings
gikari updated this revision to Diff 64002.Aug 18 2019, 9:38 PM
  • Delegate gtk fonts configuration to Gtk Daemon
nicolasfella added inline comments.Aug 18 2019, 10:03 PM
kcms/fonts/fonts.cpp
603

QStringLiteral() around strings

608

I think the specific string should be determined by the gtkconfig daemon

611

We know it will be about GTK, so changeFont should be enough.

Also, setFont would be more consistent with other API naming

davidedmundson requested changes to this revision.Mon, Aug 19, 10:41 AM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
kcms/fonts/CMakeLists.txt
20 ↗(On Diff #64002)

We have this exporting of KINIT_DBUS_INTERFACES_DIR from the library rather than just hardcoding /usr/share/dbus/interfaces/ so that packagers can do whatever weird stuff packagers do where things are in different prefixes.

Where does org.kde.gtkConfigd.xml come from?

This revision now requires changes to proceed.Mon, Aug 19, 10:41 AM
gikari added inline comments.Mon, Aug 19, 3:58 PM
kcms/fonts/CMakeLists.txt
20 ↗(On Diff #64002)

Xml comes from gtk configuration daemon I'm currently working on, whose source code is currently here. But it was recomended to me to make the daemon as the patch to either plasma-desktop, plasma-workspace or kde-gtk-config repos.

If I understand correctly, this is not a correct way to import dbus interface, but unfortunately I don't know how to do it properly. I tried what I learned from the wiki tutorial about dbus (which is rather old by the way). This is CMake code from the repo above to generate xml for development.

set(DBUS_PREFIX "~/kde/usr/share/dbus-1/interfaces")

set(SOURCES "kdegtkconfigd.cpp" ${CMAKE_CURRENT_BINARY_DIR}/org.kde.gtkConfigd.xml)
set(HEADERS "kdegtkconfigd.h")

qt5_generate_dbus_interface(kdegtkconfigd.h org.kde.gtkConfigd.xml OPTIONS -A)

qt5_add_dbus_adaptor(SOURCES org.kde.gtkConfigd.xml
        kdegtkconfigd.h KdeGtkConfigd)

install(
    FILES ${CMAKE_CURRENT_BINARY_DIR}/org.kde.gtkConfigd.xml
    DESTINATION ${DBUS_PREFIX}
    )
davidedmundson added inline comments.Mon, Aug 19, 4:08 PM
kcms/fonts/CMakeLists.txt
20 ↗(On Diff #64002)

If you put that daemon in plasma-desktop, then you can just do
qt5_add_dbus_interface("../somePath.xml") here and my comment becomes a non-issue.

If it ends up in gtk-config for separation, then we should continue that separation and just include the .xml file in two places.

But it was recomended to me to make the daemon as the patch

Discussed where?

I'm curious of the long term plans and how this fits in with krdb which does most other syncing.

But it was recomended to me to make the daemon as the patch

Discussed where?

I'm curious of the long term plans and how this fits in with krdb which does most other syncing.

@ngraham suggested that in VDG room (Matrix Message Link)

Please keep in mind that @davidedmundson is 1000 times more technically skilled than I so if our advice is ever in conflict, you should listen to him rather than me. :)

gikari updated this revision to Diff 64329.Thu, Aug 22, 5:09 PM
  • Delegate gtk fonts configuration to Gtk Daemon
gikari updated this revision to Diff 64330.Thu, Aug 22, 5:11 PM
gikari marked an inline comment as done.
  • Remove redundant xml stuff
gikari updated this revision to Diff 64331.Thu, Aug 22, 5:13 PM
  • Remove redundant saveFontToGtkSettings method declaration
gikari updated this revision to Diff 64339.Thu, Aug 22, 6:11 PM
gikari marked 2 inline comments as done.
  • Remove redundant includes
gikari updated this revision to Diff 64342.Thu, Aug 22, 6:14 PM
  • Remove QStandardPaths as redundant include
gikari added a comment.EditedThu, Aug 22, 6:27 PM

Since awesome @nicolasfella committed his changes to Gtk Daemon, I have modified the interface according to it. No more wacky xml stuff, because the daemon became a kded module with the adequate dbus signatures.

Now I'm interested where to put already written daemon: to plasma-desktop, plasma-workspace or make it part of kde-gtk-config (aka Gtk KCM). If the first, should I create a separate patch or make it as a part of this one?

P.S. I'm terribly inconsiderate at merge conflicts. Sorry for that wall of changes from above.

gikari updated this revision to Diff 64517.Sat, Aug 24, 9:01 PM

Create a Gtk Config kded module

To actually apply settings to gtk applications DBus call
should have a reciever. This creates it as kded module.

gikari edited the test plan for this revision. (Show Details)Sat, Aug 24, 9:07 PM
gikari edited the test plan for this revision. (Show Details)
gikari edited the test plan for this revision. (Show Details)Sat, Aug 24, 9:15 PM
gikari updated this revision to Diff 64619.Sun, Aug 25, 9:34 PM
gikari edited the test plan for this revision. (Show Details)

Fix harfbuzz include directories when Pango verison is 1.44 or greater

broulik added inline comments.Mon, Aug 26, 9:54 AM
kcms/fonts/fonts.cpp
607

Given this is a kded module and as such a regular qt application, you should be able to get all of this automatically.
Is the QGuiApplication::fontChanged emitted when you change font settings?

kcms/fonts/fonts.cpp
607

There are two designs which are both sensible:

  1. we have a daemon that runs all the time so that it can listen for changes. When that happens it syncs things
  1. something that the fonts module explicitly calls (through whatever means) which then syncs things.

Both have an advantage, and a disadvantage.

This is both running all the time *and* is being explicitly called, which is the worst of both worlds.

gikari added inline comments.Mon, Aug 26, 2:22 PM
kcms/fonts/fonts.cpp
607

@broulik I do not understand. KDEDModule (which is a parent of GtkConfig) is QObject, not a QGuiApplication. Maybe I am missing something? You want me to create a connection with that signal within a module, so I don't have to ask from KCM to change a font via DBus?

broulik added inline comments.Mon, Aug 26, 2:24 PM
kcms/fonts/fonts.cpp
607

kded (the application you're running in) is a QApplication, so from the constructor of your kdedmodule you could do

#include <QApplication>
connect(qApp, &QGuiApplication::fontChanged, this, [this](this QFont &font) {
    // now check if you have the new fonts
});

I can't guarantee this will work (accessing font from there will actually deadlock :D but I am curious whether that signal is emitted properly, so you dont have to listen to or ask the font kcm to tell you.

gikari added inline comments.Mon, Aug 26, 7:39 PM
kcms/fonts/fonts.cpp
607

So, I wrote this:

GtkConfig::GtkConfig(QObject *parent, const QVariantList&) :
    KDEDModule(parent), configEditor {new ConfigEditor()}
{
    qDebug() << "Parent: " << parent;
    QGuiApplication *qapp = static_cast<QGuiApplication *>(parent);
    qDebug() << "Qapp: " << qapp;
    connect(qapp, &QGuiApplication::fontChanged, this, [this](const QFont &font) {
        qDebug() << "Font recieved";
        qDebug() << font;
    });
    qDebug() << "GTK configuration module loaded";
}

And when I save fonts via KCM nothing (Except the "parent", "qapp" and "GTK ..." lines) was printed in konsole, where I run Kded5. So, it means signal was not send?

gikari added inline comments.Wed, Aug 28, 7:54 PM
kcms/fonts/fonts.cpp
607

@davidedmundson So, what are advantages and disadvantages of each of those options and what way do you recommend me go?

broulik added inline comments.Mon, Sep 16, 2:14 PM
kcms/fonts/fonts.cpp
607

Are you sure parent is a QGuiApplication? I don't think so. Perhaps you want to connect to QGuiApplication::instance() instead.

gikari added inline comments.Mon, Sep 16, 10:54 PM
kcms/fonts/fonts.cpp
607

Just tested. It works. So the plan is to remove explicit call from the font kcm and replace it with that signal binding? For fonts it sure will work, but what about other settings, that will be in the future, like color schemes, icons etc? Whose signals we will receive?