[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

GNOME Infrastructure Team  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |OBSOLETE

--- Comment #17 from GNOME Infrastructure Team  ---
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from
further activity.

You can subscribe and participate further through the new bug through this link
to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/886.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

Martin Blanchard  changed:

   What|Removed |Added

 Attachment #369162|reviewed|none
 status||
 Attachment #369162|0   |1
is obsolete||

--- Comment #16 from Martin Blanchard  ---
Created attachment 369588
  --> https://bugzilla.gnome.org/attachment.cgi?id=369588=edit
'gtk-fontconfig-timestamp' and 'gtk-modules' for Wayland

Sixth version. Plug the potential leak.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

--- Comment #15 from Bastien Nocera  ---
(In reply to Martin Blanchard from comment #13)
> (In reply to Bastien Nocera from comment #11)
> > (In reply to Martin Blanchard from comment #9)
> > > (In reply to Bastien Nocera from comment #8)
> > > > It also looks easy enough to extend and add support for the 2 other
> > > > GtkSettings if you're interested.
> > > 
> > > Hummm, isn't 'gtk-enable-animations' already implemented as a gsetting?
> > 
> > Yes, incorrectly so in the Wayland backend. In the X11 backend, animations
> > are force-disabled if the GSettings is false, then controlled by
> > gnome-settings-daemon, which will disable them if a software renderer is
> > used, or an external VNC client is connected.
> 
> That makes sense. Shouldn't that be handled in a separate bug though?

Up to you. It can be a separate patch in this same bug though.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

Bastien Nocera  changed:

   What|Removed |Added

 Attachment #369162|none|reviewed
 status||

--- Comment #14 from Bastien Nocera  ---
Review of attachment 369162:

Just some nits. You don't need to upload a new version, I'd wait until a GTK+
developer can review and merge this now.

::: gdk/wayland/gdkscreen-wayland.c
@@ +99,3 @@
   GdkWaylandScreen *screen_wayland = GDK_WAYLAND_SCREEN (object);

+  if (screen_wayland->dbus_proxy && screen_wayland->dbus_setting_change_id)

Linefeed after &&
Add > 0 to change_id conditional.

@@ +947,3 @@
+g_warning ("Could not handle fontconfig init: timestamp out of
bound");
+
+  g_variant_unref (value);

You need to call those outside the if block, otherwise you'll be leaking the
value if it doesn't match the expected type.

@@ +959,3 @@
+  screen_wayland->dbus_settings.modules = g_variant_dup_string (value,
NULL);
+
+  g_variant_unref (value);

Same thing here.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

--- Comment #13 from Martin Blanchard  ---
(In reply to Bastien Nocera from comment #11)
> (In reply to Martin Blanchard from comment #9)
> > (In reply to Bastien Nocera from comment #8)
> > > It also looks easy enough to extend and add support for the 2 other
> > > GtkSettings if you're interested.
> > 
> > Hummm, isn't 'gtk-enable-animations' already implemented as a gsetting?
> 
> Yes, incorrectly so in the Wayland backend. In the X11 backend, animations
> are force-disabled if the GSettings is false, then controlled by
> gnome-settings-daemon, which will disable them if a software renderer is
> used, or an external VNC client is connected.

That makes sense. Shouldn't that be handled in a separate bug though?

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

Martin Blanchard  changed:

   What|Removed |Added

 Attachment #369118|needs-work  |none
 status||
 Attachment #369118|0   |1
is obsolete||

--- Comment #12 from Martin Blanchard  ---
Created attachment 369162
  --> https://bugzilla.gnome.org/attachment.cgi?id=369162=edit
'gtk-fontconfig-timestamp' and 'gtk-modules' for Wayland

Fifth version: Keep dbus connection's cancellable alive all along
GdkWaylandScreen's lifetime.

(In reply to Bastien Nocera from comment #10)
> @@ +929,3 @@
> +  g_clear_object (_wayland->dbus_cancellable);
> 
> No, don't do this, keep it around.
> Instead, you'll want to cancel it it in dispose().

Ok.

> @@ +955,3 @@
> +}
> +
> +  g_variant_unref (value);
> 
> g_variant_unref() doesn't like being passed NULL.

Ouch...

> @@ +963,3 @@
> +{
> +  modules = g_variant_get_string (value, );
> +  screen_wayland->dbus_settings.modules = g_strndup (modules, length);
> 
> You're leaking the old value of dbus_settings.modules.
> 
> Seeing as the string will be nul-terminated, free the old string, and use
> g_variant_dup_string() to assign the new value.

Well, I'm expecting this to be called only once, at object's initialisation
time. Are there any case where this would be called again for the same
GdkWaylandScreen instance? Anyway, string gets freed now.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

--- Comment #11 from Bastien Nocera  ---
(In reply to Martin Blanchard from comment #9)
> (In reply to Bastien Nocera from comment #8)
> > It also looks easy enough to extend and add support for the 2 other
> > GtkSettings if you're interested.
> 
> Hummm, isn't 'gtk-enable-animations' already implemented as a gsetting?

Yes, incorrectly so in the Wayland backend. In the X11 backend, animations are
force-disabled if the GSettings is false, then controlled by
gnome-settings-daemon, which will disable them if a software renderer is used,
or an external VNC client is connected.

> > ::: gdk/wayland/gdkscreen-wayland.c
> > @@ +70,3 @@
> > Why use a struct here, and not directly a timestamp? The struct will be
> > initialised to 0 when it's instantiated.
> 
> I've kept the struct so that dbus settings are grouped, that make it clear
> that they'll be handle together.

That's fine. If a GTK+ reviewer doesn't like it, we can unbundle it.

> > @@ +713,3 @@
> > Why the guard here? Wouldn't you want to return the "0" instead of an error
> > here?
> 
> You're right, the distinction between a somehow fake initial value and 0 is
> of little interest here.

Well, 0 is a valid value. This will break if somebody rewinds their clock back
to the 1970, but the value itself is of little interest, we could use an always
incrementing serial, but a timestamp will likely make races and debugging
easier.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

Bastien Nocera  changed:

   What|Removed |Added

 Attachment #369118|none|needs-work
 status||

--- Comment #10 from Bastien Nocera  ---
Review of attachment 369118:

Looks good otherwise.

::: gdk/wayland/gdkscreen-wayland.c
@@ +929,3 @@
+  proxy = g_dbus_proxy_new_for_bus_finish (result, NULL);
+
+  g_clear_object (_wayland->dbus_cancellable);

No, don't do this, keep it around.

Instead, you'll want to cancel it it in dispose().

@@ +955,3 @@
+}
+
+  g_variant_unref (value);

g_variant_unref() doesn't like being passed NULL.

@@ +963,3 @@
+{
+  modules = g_variant_get_string (value, );
+  screen_wayland->dbus_settings.modules = g_strndup (modules, length);

You're leaking the old value of dbus_settings.modules.

Seeing as the string will be nul-terminated, free the old string, and use
g_variant_dup_string() to assign the new value.

@@ +966,3 @@
+}
+
+  g_variant_unref (value);

Ditto.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

Martin Blanchard  changed:

   What|Removed |Added

 Attachment #368982|needs-work  |none
 status||
 Attachment #368982|0   |1
is obsolete||

--- Comment #9 from Martin Blanchard  ---
Created attachment 369118
  --> https://bugzilla.gnome.org/attachment.cgi?id=369118=edit
'gtk-fontconfig-timestamp' and 'gtk-modules' for Wayland

Fourth version: Handle both 'gtk-fontconfig-timestamp' and 'gtk-modules'.

(In reply to Bastien Nocera from comment #8)
> It also looks easy enough to extend and add support for the 2 other
> GtkSettings if you're interested.

Hummm, isn't 'gtk-enable-animations' already implemented as a gsetting?

> ::: gdk/wayland/gdkscreen-wayland.c
> @@ +70,3 @@
> Why use a struct here, and not directly a timestamp? The struct will be
> initialised to 0 when it's instantiated.

I've kept the struct so that dbus settings are grouped, that make it clear that
they'll be handle together.

> @@ +116,3 @@
> You can replace both lines with g_clear_object();

Done.

> @@ +713,3 @@
> Why the guard here? Wouldn't you want to return the "0" instead of an error
> here?

You're right, the distinction between a somehow fake initial value and 0 is of
little interest here.

> @@ +872,3 @@
> You can replace this loop with g_variant_lookup_value()
> Don't forget to exit early if the timestamp wasn't found.

Done.

> @@ +914,3 @@
> Either you should not use an error at all (you don't do anything with it),
> or you should have a few cases and print errors if the error is something
> other than the call being cancelled, or org.gtk.Settings not existing.

Agreed, my bad.

> @@ +969,3 @@
> You'll also want to add a cancellable here, which you would cancel and
> destroy in the finalize function.

Done.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

Bastien Nocera  changed:

   What|Removed |Added

 Attachment #368982|none|needs-work
 status||

--- Comment #8 from Bastien Nocera  ---
Review of attachment 368982:

Thanks for looking into this again, we're nearly there.

It also looks easy enough to extend and add support for the 2 other GtkSettings
if you're interested.

::: gdk/wayland/gdkscreen-wayland.c
@@ +70,3 @@
   GHashTable *settings;
   GsdXftSettings xft_settings;
+  GsdFcSettings fc_settings;

Why use a struct here, and not directly a timestamp? The struct will be
initialised to 0 when it's instantiated.

@@ +116,3 @@

+  if (screen_wayland->dbus_proxy)
+g_object_unref (screen_wayland->dbus_proxy);

You can replace both lines with g_clear_object();

@@ +713,3 @@
+  if (strcmp (name, "gtk-fontconfig-timestamp") == 0)
+{
+  if (wayland_screen->fc_settings.timestamp > 0)

Why the guard here? Wouldn't you want to return the "0" instead of an error
here?

@@ +872,3 @@
+return;
+
+  g_variant_get (changed_properties, "a{sv}", );

You can replace this loop with g_variant_lookup_value()
Don't forget to exit early if the timestamp wasn't found.

@@ +914,3 @@
+  if (proxy == NULL)
+{
+  g_clear_error ();

Either you should not use an error at all (you don't do anything with it), or
you should have a few cases and print errors if the error is something other
than the call being cancelled, or org.gtk.Settings not existing.

@@ +969,3 @@
+GTK_SETTINGS_DBUS_PATH,
+GTK_SETTINGS_DBUS_NAME,
+NULL,

You'll also want to add a cancellable here, which you would cancel and destroy
in the finalize function.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

Martin Blanchard  changed:

   What|Removed |Added

 Attachment #368197|needs-work  |none
 status||
 Attachment #368197|0   |1
is obsolete||

--- Comment #7 from Martin Blanchard  ---
Created attachment 368982
  --> https://bugzilla.gnome.org/attachment.cgi?id=368982=edit
'gtk-fontconfig-timestamp' for Wayland (dbus's org.gtk.Settings interface)

Third version: uses new dbus interface org.gtk.Settings and mute warning on
dbus connection opening failure.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

Bastien Nocera  changed:

   What|Removed |Added

 CC||bugzi...@hadess.net

--- Comment #6 from Bastien Nocera  ---
The gnome-settings-daemon patch is all done.

The D-Bus service will be on "org.gtk.Settings":

  



  


And those values will be updated at run-time, as needed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

Bastien Nocera  changed:

   What|Removed |Added

 Attachment #368197|accepted-commit_now |needs-work
 status||

--- Comment #5 from Bastien Nocera  ---
Review of attachment 368197:

I don't think this is a good enough interface for GTK+ to use, but best discuss
it in a location where it will be implemented (in gnome-settings-daemon)

::: gdk/wayland/gdkscreen-wayland.c
@@ +919,3 @@
+  if (proxy == NULL)
+{
+  if (error != NULL)

This will warn for every GTK+ app running under a non-GNOME Wayland server.

@@ +946,3 @@
+screen_wayland->fc_settings.timestamp = (guint)timestamp;
+  else if (timestamp > G_MAXUINT)
+g_warning ("Could not handle fontconfig init: timestamp too long");

too long?

@@ +975,3 @@
+G_DBUS_PROXY_FLAGS_NONE,
+NULL,
+GSD_DBUS_FONTCONFIG_NAME,

This is highly GNOME specific.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

Matthias Clasen  changed:

   What|Removed |Added

 Attachment #368197|none|accepted-commit_now
 status||

--- Comment #4 from Matthias Clasen  ---
Review of attachment 368197:

This looks good to me now.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

Martin Blanchard  changed:

   What|Removed |Added

 Attachment #365989|0   |1
is obsolete||

--- Comment #3 from Martin Blanchard  ---
Created attachment 368197
  --> https://bugzilla.gnome.org/attachment.cgi?id=368197=edit
'gtk-fontconfig-timestamp' for Wayland (timestamp as int64)

Second version: handle timestamp as int64, inline with new
gnome-settings-daemon patch.

Comments and feedbacks welcome.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

--- Comment #2 from Matthias Clasen  ---
Thanks for working on this!

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 786693] wayland: fix fontconfig monitoring

[gtk+]

https://bugzilla.gnome.org/show_bug.cgi?id=786693

--- Comment #1 from Martin Blanchard  ---
Created attachment 365989
  --> https://bugzilla.gnome.org/attachment.cgi?id=365989=edit
'gtk-fontconfig-timestamp' for Wayland

Replies on new org.gnome.SettingsDaemon.FontConfig. See Bug #786693.

Comments and feedbacks welcome.

-- 
You are receiving this mail because:
You are on the CC list for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs