After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 645433 - gnome-shell's panel ignores font settings
gnome-shell's panel ignores font settings
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2011-03-21 18:15 UTC by Marek
Modified: 2017-11-17 20:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gnome-shell panel vs application's top bar (19.20 KB, image/png)
2011-03-21 18:21 UTC, Marek
  Details
screenshot of not respected font settings (275.77 KB, image/png)
2011-06-02 00:09 UTC, Marek
  Details
Support subpixel font rendering (3.32 KB, patch)
2011-11-10 18:34 UTC, Pierre Ossman
rejected Details | Review
screenshot of g-s 3.2.2.1 with the subpixel rendering patch (145.12 KB, image/png)
2012-02-25 23:04 UTC, Ilja Sekler
  Details
screenshot of eog showing the screenshot above 4x magnified (81.10 KB, image/png)
2012-02-25 23:07 UTC, Ilja Sekler
  Details
Support subpixel font rendering (3.4) (2.91 KB, patch)
2012-11-02 13:58 UTC, Pierre Ossman
none Details | Review
Subpixel font rendering and more for Wayland shells (8.85 KB, patch)
2017-10-09 11:36 UTC, Daniel van Vugt
none Details | Review
0001-eglnative-Use-gnome-settings-daemon-font-settings.patch (8.76 KB, patch)
2017-10-10 04:30 UTC, Daniel van Vugt
none Details | Review
fix-645433-v3.patch (8.76 KB, patch)
2017-10-10 07:26 UTC, Daniel van Vugt
none Details | Review
fix-645433-v4.patch (8.99 KB, patch)
2017-10-25 10:05 UTC, Daniel van Vugt
none Details | Review
fix-645433-v5.patch (8.36 KB, patch)
2017-10-26 09:33 UTC, Daniel van Vugt
none Details | Review
fix-645433-v6.patch (8.22 KB, patch)
2017-10-27 08:04 UTC, Daniel van Vugt
committed Details | Review

Description Marek 2011-03-21 18:15:48 UTC
After looking on the internet I've managed to change default font settings using dconf-editor.

As fully hinted fonts don't work for me, I've changed them to RGBA anitaliasing with slight hinting, then I've changed desktop font to "Droid Sans" - I find this Cantarell font very distracting, that's why I changed it.

But, unfortunately, none of those changes were applied to gnome-shell's top panel and other elements - I would expect it do so. I use laptop, so I would like to see fonts correctly antialiased and properly hinted.

I am using Fedora 15 Alpha on my laptop, package:

$ rpm -qa | grep gnome-shell
gnome-shell-2.91.91-2.fc15.x86_64
Comment 1 Marek 2011-03-21 18:21:17 UTC
Created attachment 183971 [details]
gnome-shell panel vs application's top bar

As you can see on the image - hinting is correctly applied to the application's top bar, but not to the gnome-shell's panel.
Comment 2 Florian Müllner 2011-06-01 10:59:40 UTC
What exactly did you do? The shell font currently cannot be changed (lest editing the stylesheet), but as of bug 599713 hinting and antialiasing should be picked up correctly (a quick test doesn't suggest any regressions).
Comment 3 Marek 2011-06-01 15:44:15 UTC
I did change font rendering settings using dconf-editor... and took a screenshot.

Note, my bug report was created in March, while the fix has been pushed to live at the beginning of May :)

I'll test it and post my results here.
Comment 4 Florian Müllner 2011-06-01 15:58:42 UTC
(In reply to comment #3)
> Note, my bug report was created in March, while the fix has been pushed to live
> at the beginning of May :)

Nope, this bug report is from March *2011*, the fix was pushed in May *2010* - so that's not it ...
Comment 5 Marek 2011-06-02 00:08:22 UTC
I am writing this post using Fedora 15 Live CD, running gnome-shell and Gnome 3.0.

I've installed dconf-editor and gnome-tweak-tool, I've changed font rendering settings from greyscale to rgba, hinting from medium to slight... although I can see font rendenring has changed - it's more fussy now, but not rgba, still greyscalled. see screenshot attached.
Comment 6 Marek 2011-06-02 00:09:28 UTC
Created attachment 189053 [details]
screenshot of not respected font settings

just to confirm - I did restart gnome shell to make sure, changes were applied.
Comment 7 Pierre Ossman 2011-11-10 17:38:32 UTC
FYI, your latest screen shot doesn't have rgb hinting for the applications either.

I am seeing the same issue here though. Everything respects the hinting settings except for gnome-shell. It is consistently ignored though, not just some elements.

This is on Fedora 16, so gnome-shell 3.2.

Should bug 599713 be reopened?
Comment 8 Pierre Ossman 2011-11-10 17:43:32 UTC
Looking at the patch for bug 599713, it seems like that patch was simply bad. I only considers no antialias and gray, not rgb. So more work is needed.
Comment 9 Pierre Ossman 2011-11-10 18:34:28 UTC
Created attachment 201169 [details] [review]
Support subpixel font rendering

Attached patch works like a charm here. The comment about mutter not supporting it no longer applies I guess.

Please review and apply. :)
Comment 10 Pierre Ossman 2012-02-25 15:34:44 UTC
Could we get this patch applied? Or comments on what's preventing it from getting applied?
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-02-25 18:57:45 UTC
Review of attachment 201169 [details] [review]:

As far as I can tell, the limitations in Clutter still exist, so I don't think you're actually seeing subpixel rendering here...
Comment 12 Ilja Sekler 2012-02-25 23:04:14 UTC
Created attachment 208424 [details]
screenshot of g-s 3.2.2.1 with the subpixel rendering patch

This is how the calendar of a patched g-s looks on Fedora 16 with subpixel-enabled freetype, slight hinting, lcddefault lcd filter setting and DejaVuSans font instead of Cantarell.
Comment 13 Ilja Sekler 2012-02-25 23:07:49 UTC
Created attachment 208425 [details]
screenshot of eog showing the screenshot above 4x magnified
Comment 14 Pierre Ossman 2012-02-26 10:12:46 UTC
That should settle it then. :)

As Ilja alluded to, Fedora's freetype doesn't support subpixel rendering. So you need to test this on a system with a fully featured one. There is a replacement one for Fedora available in rpmfusion. No idea what the state of the other major distributions is.
Comment 15 Ilja Sekler 2012-02-26 10:50:21 UTC
(In reply to comment #14)

> As Ilja alluded to, Fedora's freetype doesn't support subpixel rendering. 
> So you need to test this on a system with a fully featured one. [...]
> No idea what the state of the other major distributions is.

AFAIK all except of openSUSE ship subpixel-enabled freetype packages, though only Ubuntu and its derivatives ship with subpixel rendering + slight hinting + lcdfilter=default as default setting.

How subpixel rendering can be enabled in freetype on Fedora is mentioned on line 2 of <http://pkgs.fedoraproject.org/gitweb/?p=freetype.git;a=blob;f=freetype.spec>
Comment 16 Owen Taylor 2012-02-27 15:13:51 UTC
(In reply to comment #10)
> Could we get this patch applied? Or comments on what's preventing it from
> getting applied?

The code to do sub-pixel AA in clutter is incorrect - the math is simply wrong. Beyond that, at a theoretical level, there's a second problem which is that you cannot do sub-pixel AA correctly when a temporary buffer is involved - that would require a 6 channel temporary buffer. Un some cases clutter automatically will use such a temporary buffer. That problem may possibly be ignorable.
Comment 17 Pierre Ossman 2012-02-27 17:43:30 UTC
Do you have any idea when this will go wrong? And what the result will be? I haven't checked every portion of the shell, but what I've seen has rendered correctly. So it seems to work well in practice.
Comment 18 Alessandro Crismani 2012-02-28 11:10:32 UTC
The patch works incredibly well on my system and makes a huge difference. Thanks Pierre. I hope that problems preventing its inclusion may be sorted out, since the Shell looks much better after applying it.
Comment 19 Jasper St. Pierre (not reading bugmail) 2012-04-24 18:30:13 UTC
I just want to point out that Clutter already has similar code. This will be fixed if the patches from bug #672807 land.
Comment 20 Owen Taylor 2012-04-24 19:25:12 UTC
(In reply to comment #19)
> I just want to point out that Clutter already has similar code. This will be
> fixed if the patches from bug #672807 land.

well, if by fixed, you mean, "rendering an incorrect result that looks mostly similar to what the user requested"
Comment 21 Jasper St. Pierre (not reading bugmail) 2012-04-24 19:26:43 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > I just want to point out that Clutter already has similar code. This will be
> > fixed if the patches from bug #672807 land.
> 
> well, if by fixed, you mean, "rendering an incorrect result that looks mostly
> similar to what the user requested"

What else can we give them?
Comment 22 Owen Taylor 2012-04-24 20:02:12 UTC
There are two errors involved:

 A) The incorrect compositing math in Cogl. This is fixable as long as Cogl is rendering directly to the screen.

 B) The inherent error from temporary surfaces. Let me explain this in detail - consider drawing white glyphs onto a  temporary surface with a transparent background and then compositing that down against a solid background. If the solid background is black, then we should have blue fringes on the right and red fringes on the left for a vertical stem.

 rgb|rgb|rgb
    xxxxxxx

If the background is white, we should have no fringes at all. If we choose early - when we still have a transparent background, we don't know, so we risk having white glyphs outlined with ghostly fringes.

So, what would be useful is making mockups of the correct result and the result we're getting now for, say, the scrolled application list in the overview. What I think we should make sure that we aren't giving people fringes, any fringes, but giving them fringes that make things look better.
Comment 23 Jasper St. Pierre (not reading bugmail) 2012-05-18 21:11:35 UTC
Review of attachment 201169 [details] [review]:

We now respect ClutterSettings, instead of rolling our own font options. This patch is obsolete. Cogl still has to be fixed.
Comment 24 Jasper St. Pierre (not reading bugmail) 2012-05-18 21:13:41 UTC
Owen, do you know of a bug for CoglPango/Cogl/Clutter re: wrong math?
Comment 25 Xavier Claessens 2012-10-21 09:04:25 UTC
I'm not sure my case is the same as described on this bug:

I've connected my PC on the TV, but with 1920x1080 resolution (needed to watch HD movies) text are too small to read. I've changed font size in gnome-tweak-tool which makes all apps much easier to use, but gnome-shell does not respect that setting. The panel, password request dialog, etc, are all still too small to read.
Comment 26 Pierre Ossman 2012-11-02 12:09:14 UTC
Which version of Gnome Shell is required for this? I upgraded to 3.4 (would have been 3.6 if Fedora could get F18 in order) and that seems to be insufficient.
Comment 27 Pierre Ossman 2012-11-02 13:58:15 UTC
Created attachment 227889 [details] [review]
Support subpixel font rendering (3.4)

Checked the code, and 3.4 is apparently too old. Attached an updated patch for those also running 3.4.
Comment 28 Jasper St. Pierre (not reading bugmail) 2012-11-02 15:08:54 UTC
It was landed in 3.6.

http://git.gnome.org/browse/gnome-shell/commit/src/shell-global.c?h=gnome-3-6&id=dc3d3acb3b6ce16303ee1cd08ff075817f923243

If anything, we should just cherry-pick that for gnome-3-4. I think the Clutter there is new enough.
Comment 29 Pierre Ossman 2012-11-29 09:11:20 UTC
Confirmed working (modulo Owen's gotchas) on 3.6.
Comment 30 Jasper St. Pierre (not reading bugmail) 2013-03-03 21:38:18 UTC
Let's move this to cogl to let them fix it.
Comment 31 Emmanuele Bassi (:ebassi) 2013-03-04 11:32:49 UTC
a new bug would have been better.

Owen: can you clarify comment 22?
Comment 32 Owen Taylor 2013-03-04 15:26:20 UTC
(In reply to comment #31)
> a new bug would have been better.
> 
> Owen: can you clarify comment 22?

Can you be more specific about what aspects of comment 22 are unclear?
Comment 33 Emmanuele Bassi (:ebassi) 2013-03-04 15:35:27 UTC
point A:

> A) The incorrect compositing math in Cogl. This is fixable as long as Cogl is
> rendering directly to the screen.

what would be the correct compositing math? is it a bug in the Cogl Pango renderer when submitting geometry, or is it inside the glyph cache when generating the glyph masks?
Comment 34 Owen Taylor 2013-03-04 17:08:34 UTC
The basic idea is that compositing subpixel glyphs requires specific math, if I remember correctly:

 dest_a = source_a * mask_a + (1 - source_a * mask_a) * dest_a
 dest_r = source_r * mask_r + (1 - source_a * mask_r) * dest_r
 dest_g = source_g * mask_g + (1 - source_a * mask_g) * dest_g
 dest_b = source_b * mask_b + (1 - source_a * mask_b) * dest_b

Instead of this, the way that blending is done in the cogl-pango is:

 dest_a = source_a * mask_a + (1 - source_a * mask_a) * dest_a
 dest_r = source_r * mask_r + (1 - source_a * mask_a) * dest_r
 dest_g = source_g * mask_g + (1 - source_a * mask_a) * dest_g
 dest_b = source_b * mask_b + (1 - source_a * mask_a) * dest_b

(The comment in cogl-pango-pipeline-cache.c:get_base_texture_alpha_pipeline() mentions what the default combine mode is. get_base_texture_rgba_pipeline() just uses that, which gives us a fragment of source_a * mask_a, source_r * mask_r, source_g * mask_g, source_b * mask_b. Then we just leave the default blending mode in place, giving the above equations.)

So the result is correct if the background is black, but not otherwise. In some cases you can do it in a single pass with a blend factor of 1 - SRC_COLOR - I think this is the case where source_a == source_r == source_g == source_b - white, black, or gray - but check my math. In other cases, two passes are needed.

See also the comment for exaTryMagicTwoPassCompositeHelper() in http://cgit.freedesktop.org/xorg/xserver/tree/exa/exa_render.c.

If you have an intermediate texture - if you, are, say, applying an effect to a Cogl actor - then this doesn't work out at all, because we no longer have a separate source color and 4-channel mask, we just have a combined argb value.
Comment 35 Daniel van Vugt 2017-10-06 05:27:59 UTC
This is still an open issue for Wayland sessions:
  https://bugs.launchpad.net/ubuntu/+source/gnome-shell/+bug/1714459

Should we start a new bug?
Comment 36 Emmanuele Bassi (:ebassi) 2017-10-06 09:15:42 UTC
Well, if it's a problem in GNOME Shell then this issue should be re-assigned to Mutter, otherwise the Shell developers won't ever be aware of it.
Comment 37 Daniel van Vugt 2017-10-06 09:43:51 UTC
I'm trying to learn the mutter code and fix this right now so you could assign it to me. Still wouldn't mind if someone else fixed it in the mean time.
Comment 38 Daniel van Vugt 2017-10-09 11:36:44 UTC
Created attachment 361177 [details] [review]
Subpixel font rendering and more for Wayland shells

A patch to try and close this bug, finally.
Comment 39 Daniel van Vugt 2017-10-10 04:30:21 UTC
Created attachment 361212 [details] [review]
0001-eglnative-Use-gnome-settings-daemon-font-settings.patch

Patch version 2.
Comment 40 Daniel van Vugt 2017-10-10 07:26:51 UTC
Created attachment 361215 [details] [review]
fix-645433-v3.patch

Version 3: Fix typo and other documentation.
Comment 41 Marco Trevisan (Treviño) 2017-10-25 00:46:38 UTC
Review of attachment 361215 [details] [review]:

This is a good improvement, I'm not in love of the usage of this gsettings key as you know, but... If we don't move it now (I would), this is fine.

Unfortunately the RGB subpixel isn't available for wayland anyway, but it's in a different level.

Some thoughts on code:

::: clutter/clutter/egl/clutter-backend-eglnative.c
@@ +64,3 @@
 
+  g_signal_handlers_disconnect_by_data (backend_egl_native->xsettings,
+                                        backend_egl_native);

This is pointless here, since you'll clear the object anyway, so no risk that it will emit any signal.

Being a backend it's probably not needed either if you clear, while you can instead use connect_object. Just the clear is fine for me.

@@ +145,3 @@
+
+  hinting = g_settings_get_enum (xsettings, "hinting");
+  if (hinting >= 0 && hinting < G_N_ELEMENTS (hintings))

G_N_ELEMENTS is defined using sizeof, so an unsigned, you might need to cast this.

@@ +152,3 @@
+  else
+    {
+      output->cairo_hint_style = CAIRO_HINT_STYLE_NONE;

use CAIRO_HINT_STYLE_DEFAULT

@@ +153,3 @@
+    {
+      output->cairo_hint_style = CAIRO_HINT_STYLE_NONE;
+      output->clutter_font_hint_style = "hintnone";

Then in this case there's no need to set any clutter hint style I think

@@ +164,3 @@
+  else
+    {
+      output->cairo_antialias = CAIRO_ANTIALIAS_GRAY;

Use CAIRO_ANTIALIAS_DEFAULT instead

@@ +176,3 @@
+  else
+    {
+      output->cairo_subpixel_order = CAIRO_SUBPIXEL_ORDER_RGB;

And here too CAIRO_SUBPIXEL_ORDER_DEFAULT

@@ +197,3 @@
+  options = cairo_font_options_create ();
+  if (!options)
+    return;

g_return_if_fail probably here is wanted as I guess it's something we expect to have, no?

@@ +203,3 @@
+  cairo_font_options_set_antialias (options, current.cairo_antialias);
+  cairo_font_options_set_subpixel_order (options, current.cairo_subpixel_order);
+  clutter_backend_set_font_options (backend, options);

I didn't check, but this is also initializing the ClutterSettings properties, right?

@@ +225,3 @@
+  ClutterSettings *csettings = clutter_settings_get_default ();
+  GValue value = G_VALUE_INIT;
+  FontSettings current;

maybe just fs? Current isn't really explicative here.

@@ +247,3 @@
+  g_value_set_string (&value, current.clutter_font_subpixel_order);
+  clutter_settings_set_property_internal (csettings, "font-subpixel-order", &value);
+  g_value_unset (&value);

I would remove the hassle of gvalue here and just use g_object_set:

  g_object_set (csettings,
                "font-hinting", 1,
                "font-antialias", current.clutter_font_antialias,
                "font-hint-style", current.clutter_font_hint_style,
                "font-subpixel-order", current.clutter_font_subpixel_order,
                NULL);

Using g_intern_string in case, but I don't see the point.

@@ +257,3 @@
   backend_egl_native->event_timer = g_timer_new ();
+
+  backend_egl_native->xsettings = g_settings_new ("org.gnome.settings-daemon.plugins.xsettings");

I'd put this in a define

@@ +259,3 @@
+  backend_egl_native->xsettings = g_settings_new ("org.gnome.settings-daemon.plugins.xsettings");
+
+  if (backend_egl_native->xsettings)

This won't actually ensure that the settings exist, but if they don't it will assert badly.

So, it's probably saner to use g_settings_schema_source_lookup with this source, and if if found you can create the gsettings using g_settings_new_full.

Maybe a gwarning stating that we miss this could be useful too.

@@ +262,3 @@
+    {
+      clutter_backend_egl_native_init_font_options (backend_egl_native);
+      g_signal_connect (backend_egl_native->xsettings, "change-event",

Why not "changed"?

And in the callback you could even ignore changes to the keys we don't need, or instead add 3 connections with changed::key-name (probably not the nicest anyway).
Comment 42 Daniel van Vugt 2017-10-25 10:05:57 UTC
Created attachment 362243 [details] [review]
fix-645433-v4.patch

Patch version 4.
Comment 43 Marco Trevisan (Treviño) 2017-10-25 13:17:55 UTC
Review of attachment 362243 [details] [review]:

::: clutter/clutter/egl/clutter-backend-eglnative.c
@@ +138,3 @@
+  };
+
+  guint hinting, antialiasing, rgba_order;

I find these a bit hard to read... as values[value].. 

Since you don't need to preserve the vars, you can just initialize a guint value and use it for every enum.

@@ +152,3 @@
+       */
+      output->cairo_hint_style = CAIRO_HINT_STYLE_NONE;
+      output->clutter_font_hint_style = "hintnone";

What I was suggesting was to not sending such info to clutter at all in case, but in any case I think you can just add support for such values in settings_update_font_options too, as it will reconvert these in cairo settings again, so there's no point of changing it since we've all the codepath in control.

@@ +193,3 @@
+  FontSettings fs;
+
+  g_return_if_fail (xsettings = backend_egl_native->xsettings);

I preferred just an "if (!xsettings) here", since this will cause a warning which we already have triggered on _init.

@@ +194,3 @@
+
+  g_return_if_fail (xsettings = backend_egl_native->xsettings);
+  g_return_if_fail (options = cairo_font_options_create ());

I don't think this could fail either, but if you want to add the g_return, do the initialization first, as doing it in there is quite confusing.

@@ +259,3 @@
+      if (!backend_egl_native->xsettings)
+        {
+          g_warning ("Failed to open settings for %s", xsettings_path);

Honestly I don't think this can happen, if we've the schema, well get something here.

No need to over-check imho.
Comment 44 Daniel van Vugt 2017-10-26 09:33:26 UTC
Created attachment 362324 [details] [review]
fix-645433-v5.patch

Patch version 5.

Still pretty much unchanged logic for the last few versions. Only formatting changes for Marco.
Comment 45 Marco Trevisan (Treviño) 2017-10-26 11:27:06 UTC
Review of attachment 362324 [details] [review]:

Ok, looks good now

::: clutter/clutter/egl/clutter-backend-eglnative.c
@@ +137,3 @@
+    /* vbgr=4 */ {CAIRO_SUBPIXEL_ORDER_VBGR, "vbgr"},
+  };
+  guint i;

Ehehe, you went minimal for real now :).
Comment 46 Jonas Ådahl 2017-10-26 13:02:42 UTC
Review of attachment 362324 [details] [review]:

Functionality wise it looks good. My comments are regarding readability and coding style.

::: clutter/clutter/egl/clutter-backend-eglnative.c
@@ +94,3 @@
+
+static void
+get_font_gsettings (GSettings *xsettings, FontSettings *output)

Function arguments goes on their own lines, with the argument variable names aligned

@@ +183,3 @@
+{
+  GSettings *xsettings = backend_egl_native->xsettings;
+  cairo_font_options_t *options = NULL;

There is no need to initialize this.

@@ +187,3 @@
+
+  if (!xsettings)
+    return FALSE;

You only ever call this after already having checked this, so why check it again?

@@ +189,3 @@
+    return FALSE;
+
+  options = cairo_font_options_create ();

This only happens on OOM, so no need to handle it. It's not handled in other places in clutter, so lets keep things consistent.

@@ +204,3 @@
+  cairo_font_options_destroy (options);
+
+  return TRUE;

Given the two above comments, you can stop making this function look like it might fail when it will never.
Comment 47 Marco Trevisan (Treviño) 2017-10-27 00:29:23 UTC
Review of attachment 362324 [details] [review]:

::: clutter/clutter/egl/clutter-backend-eglnative.c
@@ +183,3 @@
+{
+  GSettings *xsettings = backend_egl_native->xsettings;
+  cairo_font_options_t *options = NULL;

Agree.

@@ +187,3 @@
+
+  if (!xsettings)
+    return FALSE;

I was about to say the same, but then I noticed it could be still the case where you have initialized but there's no such settings schema, thus here it's null, but not an error.
Comment 48 Jonas Ådahl 2017-10-27 02:31:11 UTC
(In reply to Marco Trevisan (Treviño) from comment #47)
> Review of attachment 362324 [details] [review] [review]:
>
> @@ +187,3 @@
> +
> +  if (!xsettings)
> +    return FALSE;
> 
> I was about to say the same, but then I noticed it could be still the case
> where you have initialized but there's no such settings schema, thus here
> it's null, but not an error.

The function init_font_options() is only called if the backend_egl_native->xsettings is non-NULL (in clutter_backend_egl_native_init()).
Comment 49 Marco Trevisan (Treviño) 2017-10-27 02:42:40 UTC
(In reply to Jonas Ådahl from comment #48)
> The function init_font_options() is only called if the
> backend_egl_native->xsettings is non-NULL (in
> clutter_backend_egl_native_init()).

Ah you're right, I think it wasn't like this in the previous version, thus I didn't pay attention
Comment 50 Daniel van Vugt 2017-10-27 08:04:37 UTC
Created attachment 362385 [details] [review]
fix-645433-v6.patch

Patch version 6.
Comment 51 Jonas Ådahl 2017-10-27 08:20:46 UTC
Review of attachment 362385 [details] [review]:

lgtm
Comment 52 Marco Trevisan (Treviño) 2017-10-28 03:16:19 UTC
Pushed as https://git.gnome.org/browse/mutter/commit/?id=a37956c

I don't think we need this for gnome-3-26, right?
Comment 53 Daniel van Vugt 2017-10-30 06:21:28 UTC
Well I developed it against the gnome-3-26 branch, so it definitely works there. But it's not an important fix to backport to 3.26.
Comment 54 Daniel van Vugt 2017-10-31 02:30:26 UTC
We definitely want the fix in Ubuntu 18.04 at least.

If there's a chance 18.04 will still be on Gnome 3.26 then yes this fix should be applied to gnome-3-26. That would be a bit nicer than having to distro patch it in 18.04.
Comment 55 Florian Müllner 2017-10-31 11:43:05 UTC
(In reply to Daniel van Vugt from comment #54)
> If there's a chance 18.04 will still be on Gnome 3.26 then yes this fix
> should be applied to gnome-3-26.

No. If 18.04 will still be on Gnome 3.26, then it would be more convenient for you if the fix was on the gnome-3-26 branch.

To be clear: In this particular case, pushing the patch to the gnome-3-26 branch is fine with me, and if the patch makes it in today I'll include it in the 3.26.2 release.

But please don't imply that upstream has any obligation to comply with any downstream decision Ubuntu (or any other distribution) makes. Not too long ago, this would have meant freezing private API between gnome-shell, gnome-settings-daemon and gnome-control-center, because Ubuntu was mixing core components from three stable releases ...
Comment 56 Daniel van Vugt 2017-11-01 02:55:52 UTC
Whoa, peace. I didn't mean to imply that "should" is a strong word.

- should
+ ideally would

I don't really mind which way it goes. I would just like to know so that a decision doesn't get deferred till April and we're left with an LTS without the fix.

I am happy to distro-patch this just for Ubuntu but at the moment there are no Ubuntu-specific patches on mutter. I thought it would be nice to suggest as a first preference that this not become the singular reason we deviate from the Debian debs. And now this discussion should really move back into Launchpad...
Comment 57 Marco Trevisan (Treviño) 2017-11-17 20:38:11 UTC
(In reply to Florian Müllner from comment #55)
> To be clear: In this particular case, pushing the patch to the gnome-3-26
> branch is fine with me, and if the patch makes it in today I'll include it
> in the 3.26.2 release.

Since there were no objections, I've pushed this to gnome-3-26 as https://git.gnome.org/browse/mutter/commit/?h=gnome-3-26&id=d358e, so in in any case it will hit the users using 3.26.3 (including ubuntu for sure).