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 744921 - Update font size automatically when Xft resolution changes
Update font size automatically when Xft resolution changes
Status: RESOLVED FIXED
Product: yelp
Classification: Applications
Component: General
git master
Other Linux
: Normal normal
: ---
Assigned To: Yelp maintainers
Yelp maintainers
Depends on:
Blocks:
 
 
Reported: 2015-02-22 01:53 UTC by Mario Sánchez Prada
Modified: 2015-03-13 17:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch Proposal (4.46 KB, patch)
2015-02-22 02:13 UTC, Mario Sánchez Prada
accepted-commit_now Details | Review

Description Mario Sánchez Prada 2015-02-22 01:53:19 UTC
In Yelp from master (currently 3.15.90), the size of the text being rendered inside of the content pane (a WebKit webview) won't change if you suddenly change the Xft resolution in your desktop, but only after having closed and re-launched Yelp.

STEPS TO REPRODUCE:

 1. Open Yelp and navigate to any topic showing text
 2. Go to gnome-control-center > accessibility and enable "Larger Text"
 3. Observe how text from GTK UI scales accordingly, but the content doesn't 
 4. Close Yelp and restart again, with the "Larger Text" option still enabled
 5. See how text is now scaled to be bigger
 6. Disable the "Larger Text" accessibility option now
 7. Observe how only the GTK UI scales accordingly, content keeps being big
 8. Close Yelp and restart again, see how text is now small again

EXPECTED OUTCOME:

Text from the Webview showing the topic's content should be scaled accordingly whenever the accessibility "Larger Text" option is enabled or disabled, together with the rest of the UI.

Another way to cause the same effect would be to update the 'text-scaling-factor' from org.gnome.desktop.interface GSettings to some random value (a11y uses 1.25 for "Larger Text"), but I think for testing the a11y option is easier.

ACTUAL OUTCOME:

Yelp does not update the font size of the contents when the screen resolution for font handling changes, and only considers that value on startup.

OTHER COMMENTS:

This issue is similar to epiphany's bug 744796, which kind of suggests it would be better to fix this problem in WebKitGTK+ itself. Unfortunately, doing it so would probably require to change the WebKitSettings API in a way that is not likely to happen anytime soon, so the suggested approach is to take care of this details in the application side.
Comment 1 Mario Sánchez Prada 2015-02-22 02:13:00 UTC
Created attachment 297545 [details] [review]
Patch Proposal

See attached implementing a similar idea to the one proposed for epiphany: use GtkSettings's gtk-xft-dpi property both to find the Xft resolution on demand and to keep track of changes when they happen.

Please review, thanks!
Comment 2 David King 2015-02-25 10:24:48 UTC
Review of attachment 297545 [details] [review]:

Looks good, just a question about gtk_settings_get_default().

::: libyelp/yelp-view.c
@@ +255,3 @@
     priv->prevstate = priv->state = YELP_VIEW_STATE_BLANK;
 
+    priv->gtk_settings = gtk_settings_get_default ();

As a view is associated with a GdkScreen, is it better to use gtk_settings_get_for_screen() here? I suppose that the DPI setting is per-screen. On the other hand, I do not know if it is common to have different GdkScreens nowadays.

@@ +1881,3 @@
+  gdouble dpi = 96;
+
+  settings = gtk_settings_get_default ();

Same comment as above about tying the settings object to a GdkScreen.
Comment 3 Mario Sánchez Prada 2015-02-25 11:14:05 UTC
(In reply to David King from comment #2)
> Review of attachment 297545 [details] [review] [review]:
> 
> Looks good, just a question about gtk_settings_get_default().

Thanks for the review, I've inlined some comments below too...

> ::: libyelp/yelp-view.c
> @@ +255,3 @@
>      priv->prevstate = priv->state = YELP_VIEW_STATE_BLANK;
>  
> +    priv->gtk_settings = gtk_settings_get_default ();
> 
> As a view is associated with a GdkScreen, is it better to use
> gtk_settings_get_for_screen() here?

Yes, the problem I see is that at the moment I'm connecting here for the signal is not really possible to know the correct GdkScreen for the YelpView widget, as it has not been added to any container widget yet.

So, I resolved to use the default screen since that's what epiphany does too, and also because it seems that there should be only one GdkScreen anyway (see my comment below).

> I suppose that the DPI setting is
> per-screen. On the other hand, I do not know if it is common to have
> different GdkScreens nowadays.

Exactly, that's what I thought too :-). As per the GdkScreen documentation:

"X originally identified screens with physical screens, but nowadays it is more common to have a single GdkScreen which combines several physical monitors (see gdk_screen_get_n_monitors())."

But I agree that's an oversimplification and perhaps not the safest approach, and actually epiphany seems to acknowledge a similar issue too. From ephy-embed-prefx.c:

"
  [...]
  /* FIXME: We should use the view screen instead of the detault one
   * but we don't have access to the view here.
   */
  screen = gdk_screen_get_default ();
  [...]
"

> @@ +1881,3 @@
> +  gdouble dpi = 96;
> +
> +  settings = gtk_settings_get_default ();
> 
> Same comment as above about tying the settings object to a GdkScreen.

This one is actually a bit trickier, because we can't know the GdkScreen from here: 

This function (normalize_font_size()) gets called from settings_set_fonts(), which is a callback for the YelpSettings::fonts-changed signal. And the problem is that such a callback is connected globally from the class_init(), so we have no access to a specific YelpView when normalize_font_size() is executed, thus we can't get the specific GdkScreen associated to it.

I think a possible solution to this problem, so that we can stop using gtk_settings_get_default() or gdk_screen_get_default() could be something like this:

 * Connect to YelpSettings::fonts-changed from yelp_view_init() instead, passing the current YelpView as the user data, so that we have access to the YelpView from settings_set_fonts() when executed
 * Pass the YelpView to normalize_font_size(), and use it internally to get the right GdkScreen for the view, so that we also get the right GtkSettings
 * Also from yelp_view_init(), connect to GtkWidget::screen-changed, so that we can handle both the initial case, and further screen changes (should they happen)
 * From the screen_changed() callback, simply call to settings_set_fonts() passing the default YelpSettings, which should do the right thing
 * Disconnect handlers for those two signal connections newly added from yelp_view_init, from yelp_view_finalize

How does it sound?
Comment 4 David King 2015-02-25 11:21:58 UTC
Review of attachment 297545 [details] [review]:

Thanks for the reply, and I am in agreement. I think that for the moment it is fine to add a comment about fetching the GtkSettings for the GdkScreen associated with the view in the future, so please push once you have added that. The font handling changes a bit with WebKit2 (see https://git.gnome.org/browse/yelp/commit/?h=wip/amigadave/webkit2-port&id=894bbcb9310f7eb93ed6db133a97c8439acefbe5) so we can revisit later.
Comment 5 Mario Sánchez Prada 2015-02-25 11:35:14 UTC
Ok. I will add a FIXME comment like the one in epiphany and push it today then.

About your WIP branch for WebKit2, I see you are actually already doing a similar thing in there: connecting to fonts-changed from outside of class_init, so that you have access to the YelpView, which I find it particularly nice because it kind of validates my suggestion above. Also, I see there you are already taking the right screen into account, so properly fixing this issue with changing Xft resolutions in the WebKit2 version of Yelp should not be a problem, cool :)

Actually, your link made me realize we probably need a similar fix in devhelp too, since I've just checked and it's not responding to changes in the Xft resolution either, but that's should be separate bug anyway.
Comment 6 Mario Sánchez Prada 2015-02-25 11:42:14 UTC
This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.