GNOME Bugzilla – Bug 745256
Update font size automatically when Xft resolution changes
Last modified: 2015-03-13 17:05:53 UTC
In devhelp from master (currently 3.15.91), 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 devhelp and navigate to any topic 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 devhelp 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 devhelp 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: devhelp 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 (pending on review) and Yelp's bug 744921 (already committed), 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.
Created attachment 298038 [details] [review] Patch Proposal See attached implementing a similar idea to the one proposed for yelp: use GtkSettings's gtk-xft-dpi property both to find the Xft resolution on demand and to keep track of changes when they happen. In this case I've also added some additional bits to account for potential screen changes (as in GdkScreen changes), which I could not do in Yelp due to not having access to the actual WebView. Please review, thanks!
Review of attachment 298038 [details] [review]: See the comments. ::: src/dh-window.c @@ +59,2 @@ guint fonts_changed_id; + guint screen_changed_id; this should be gulong no? @@ +562,3 @@ + + /* now store the new GtkSettings and (re)connect the signals if needed */ +{ better check priv->gtk_settings == NULL @@ +677,2 @@ g_clear_object (&priv->settings); + g_clear_object (&priv->gtk_settings); gtk_settings_get_default is transfer none, don't you get an assertion here?
Thanks for the review, Nacho. I'll address the issues pointed out in a follow up patch soon. (In reply to Ignacio Casal Quinteiro (nacho) from comment #2) > Review of attachment 298038 [details] [review] [review]: > > See the comments. > > ::: src/dh-window.c > @@ +59,2 @@ > guint fonts_changed_id; > + guint screen_changed_id; > > this should be gulong no? Yes, sorry for the mistake. > @@ +562,3 @@ > + > + /* now store the new GtkSettings and (re)connect the signals if > needed */ > +{ > > better check priv->gtk_settings == NULL Ok > @@ +677,2 @@ > g_clear_object (&priv->settings); > + g_clear_object (&priv->gtk_settings); > > gtk_settings_get_default is transfer none, don't you get an assertion here? Ooops! I definitely should not clear it, sorry about that.
Created attachment 298099 [details] [review] Patch Proposal Patch updated, incorporating those suggestions as well as two calls to g_free() that were missing in the previous patch.
Review of attachment 298099 [details] [review]: Minor comments. Ask aleksander or fred to give an ack. ::: src/dh-window.c @@ +545,3 @@ +screen_changed_cb (GtkWidget *window, + GdkScreen *previous_screen, + } wrong align @@ +564,3 @@ + + /* now store the new GtkSettings and (re)connect the signals if needed */ + gpointer user_data) here you check for == NULL while in other places you do if (settings), either do it explicitly everywhere or nowhere. Check the other code to be sure what do do .
Created attachment 298315 [details] [review] Patch Proposal (In reply to Ignacio Casal Quinteiro (nacho) from comment #5) > Review of attachment 298099 [details] [review] [review]: > > Minor comments. Ask aleksander or fred to give an ack. > Thanks, will do. > ::: src/dh-window.c > @@ +545,3 @@ > +screen_changed_cb (GtkWidget *window, > + GdkScreen *previous_screen, > + } > > wrong align > Fixed > @@ +564,3 @@ > + > + /* now store the new GtkSettings and (re)connect the signals if > needed */ > + gpointer user_data) > > here you check for == NULL while in other places you do if (settings), > either do it explicitly everywhere or nowhere. Check the other code to be > sure what do do . I added this before following your suggestion, but it's true I forgot to be consistent and check what the right thing to do it is in other places. However, it's unclear to me what the right coding style is wrt that, so I'm just changing if (settings) into if (settings != NULL) in my patch for consistency with the whole changeset.
Comment on attachment 298315 [details] [review] Patch Proposal yeah, go push it.
(In reply to Frederic Peters from comment #7) > Comment on attachment 298315 [details] [review] [review] > Patch Proposal > > yeah, go push it. Done, thanks
OK, we need to fix this in WebKit, by deprecating the existing font size settings if necessary. We shouldn't have to do this in every application separately.