GNOME Bugzilla – Bug 744796
Update font size automatically when Xft resolution changes
Last modified: 2015-03-24 13:56:53 UTC
In epiphany from master (currently 3.15.1), the size of the text being rendered inside of a webview won't change if you suddenly change the Xft resolution in your desktop, but only after having closed and re-launched epiphany. Perhaps a clear example of why this is an issue is what happens when you are browsing the web and then activate the "Larger Text" option from the accessibility settings: I would expect the WebView's contents to scale accordingly, but instead I'm stuck with the text being rendered exactly as before, even though text in the GTK-based chrome of epiphany gets properly scaled. If I know close epiphany and open it again, the text is shown scaled, but if I now disable the "Larger Text" option I'm again needing to restart epiphany just to pick the right test size. 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.
Created attachment 297280 [details] [review] Patch Proposal See attached implementing the idea discussed yesterday on the #webkitgtk+ IRC channel: use GtkSettings's gtk-xft-dpi property both to find the screen resolution on demmand and to keep track of changes when they happen. Pleas review, thanks!
Review of attachment 297280 [details] [review]: ::: embed/ephy-embed-prefs.c @@ +212,3 @@ + GtkSettings *settings = gtk_settings_get_for_screen (screen); + + gtk_xft_dpi = -1; You already initialized this variable up above. @@ +221,2 @@ dp = hypot (gdk_screen_get_width (screen), gdk_screen_get_height (screen)); di = hypot (gdk_screen_get_width_mm (screen), gdk_screen_get_height_mm (screen)) / 25.4; I think we don't need this fallback code anymore. The GtkSetting should never be -1, correct? But I guess it doesn't hurt to keep it, since it's only a few lines. @@ +484,3 @@ +gtk_settings_xft_dpi_changed_cb (GtkSettings *gtk_settings) +{ + GSettings *gsettings = ephy_settings_get (EPHY_PREFS_WEB_SCHEMA); I guess gsettings is guaranteed to be nonnull here because ephy_settings_shutdown() is only called after gtk_application_run() completes, so this is fine.
Thanks for the quick review, Michael. I will address your comments in a follow up patch, but let's wait a bit more for now to see if we someone else can clarify some of the doubts expressed below... :) (In reply to Michael Catanzaro from comment #2) > Review of attachment 297280 [details] [review] [review]: > > ::: embed/ephy-embed-prefs.c > @@ +212,3 @@ > + GtkSettings *settings = gtk_settings_get_for_screen (screen); > + > + gtk_xft_dpi = -1; > > You already initialized this variable up above. OOps! Sorry about it. > @@ +221,2 @@ > dp = hypot (gdk_screen_get_width (screen), gdk_screen_get_height > (screen)); > di = hypot (gdk_screen_get_width_mm (screen), gdk_screen_get_height_mm > (screen)) / 25.4; > > I think we don't need this fallback code anymore. The GtkSetting should > never be -1, correct? But I guess it doesn't hurt to keep it, since it's > only a few lines. I'm not sure whether we need that or not to be honest, so I took the conservative approach of leaving it there. About the GtkSetting, -1 is actually a valid value according to the documentation, so I'd rather leave it there too, unless it's 100% unlikely to happen for some reason. > @@ +484,3 @@ > +gtk_settings_xft_dpi_changed_cb (GtkSettings *gtk_settings) > +{ > + GSettings *gsettings = ephy_settings_get (EPHY_PREFS_WEB_SCHEMA); > > I guess gsettings is guaranteed to be nonnull here because > ephy_settings_shutdown() is only called after gtk_application_run() > completes, so this is fine. Hmm.. interesting. Perhaps a null-check won't hurt either, though.
Created attachment 297540 [details] [review] Patch Proposal Just decided to upload a new patch addressing the comments by Michael plus a couple of minor issues I found myself, to ease its review
This patch works well.
Review of attachment 297540 [details] [review]: Sorry for the delay reviewing this. ::: embed/ephy-embed-prefs.c @@ +215,3 @@ + /* Returned value for gtk-xft-dpi is (dots/inch * 1024) */ + return gtk_xft_dpi / 1024.0; + } Are you sure we really need to do all this? GtkSettings calls gdk_screen_set_resolution() after getting the gtk-xft-dpi setting and also handles the case of GDK_DPI_SCALE env var. I think we could keep the current code here, getting the gdk_screen_get_resolution(), but I'm not sure. @@ +485,3 @@ +{ + GSettings *gsettings = ephy_settings_get (EPHY_PREFS_WEB_SCHEMA); + if (!gsettings) This can't be NULL unless you are passing an invalid schema, which is not the case. @@ +491,3 @@ + * right thing and call webkit_pref_callback_font_size() if needed. + */ + webkit_pref_callback_gnome_fonts (gsettings, EPHY_PREFS_WEB_USE_GNOME_FONTS, NULL); This is a bit weird, and that's why you need to add a comment explaining it. I would add a new function ephy_embed_prefs_update_font_settings (or something similar) called from here and webkit_pref_callback_gnome_fonts. Then you don't need to explain anything. @@ +553,3 @@ ephy_embed_prefs_init (gpointer user_data) { + GdkScreen *screen = NULL; You don't need to initialize this. @@ +586,3 @@ + screen = gdk_screen_get_default (); + if (screen) { + GtkSettings *gtk_settings = gtk_settings_get_for_screen (screen); gtk_settings_get_default already does this. You could simplify this by doing: gtk_settings = gtk_settings_get_default(); if (gtk_settings) { g_signal_connect.... } And you don't need the screen.
Review of attachment 297540 [details] [review]: Thanks for the review, Carlos. I will address all your comments in a follow up patch. Now see my coments below... ::: embed/ephy-embed-prefs.c @@ +215,3 @@ + /* Returned value for gtk-xft-dpi is (dots/inch * 1024) */ + return gtk_xft_dpi / 1024.0; + } You are probably right. I changed this for consistency with the rest of the patch (using xft-dpi all around), but it's of course ok to leave the previous implementation. After all, this function should keep returning what it was returning so far anyway :) @@ +485,3 @@ +{ + GSettings *gsettings = ephy_settings_get (EPHY_PREFS_WEB_SCHEMA); + if (!gsettings) Ok. Will remove that. @@ +491,3 @@ + * right thing and call webkit_pref_callback_font_size() if needed. + */ + GSettings *gsettings = ephy_settings_get (EPHY_PREFS_WEB_SCHEMA); Sounds good to me to. Will do that in a follow up patch. Thanks for pointing it out! @@ +553,3 @@ ephy_embed_prefs_init (gpointer user_data) { + GdkScreen *screen = NULL; Ok. @@ +586,3 @@ + screen = gdk_screen_get_default (); + if (screen) { + GtkSettings *gtk_settings = gtk_settings_get_for_screen (screen); That's right, and even something I think I did on the patches for yelp and/or devhelp (which I wrote after this one). Will change it
(In reply to Mario Sánchez Prada from comment #7) > (In reply to Carlos Garcia Campos from comment #6) > > Review of attachment 297540 [details] [review] [review]: > > @@ +491,3 @@ > > + * right thing and call webkit_pref_callback_font_size() if needed. > > + */ > > + webkit_pref_callback_gnome_fonts (gsettings, > > EPHY_PREFS_WEB_USE_GNOME_FONTS, NULL); > > > > This is a bit weird, and that's why you need to add a comment explaining it. > > I would add a new function ephy_embed_prefs_update_font_settings (or > > something similar) called from here and webkit_pref_callback_gnome_fonts. > > Then you don't need to explain anything. > > > Sounds good to me to. Will do that in a follow up patch. Thanks for pointing > it out! Actually, I'm not sure that's the right thing to do, since webkit_pref_callback_gnome_fonts() will end up settings a value for the WebKitSettings in a way or another depending on whether the 'use-gnome-fonts' GSetting is true or false. The difference is that in one case it will call g_object_set() directly over the WebKitSettings instance, and in the other case it will do it via webkit_pref_callback_font_size() instead. Perhaps the right thing is actually to update the comment to a less misleading one, without pointing directly to webkit_pref_callback_font_size(), and still call webkit_pref_callback_gnome_fonts(). What do you think?
Created attachment 299700 [details] [review] Patch Proposal Forget my previous comment, I think I need more coffee :) See a new patch attached (hopefully) addressing all your concerns.
Review of attachment 299700 [details] [review]: Please, reword the commit message because it's not accurate with this new version of the patch. ::: embed/ephy-embed-prefs.c @@ +484,3 @@ +{ + GSettings *gsettings = ephy_settings_get (EPHY_PREFS_WEB_SCHEMA); + ephy_embed_prefs_update_font_settings (gsettings, EPHY_PREFS_WEB_USE_GNOME_FONTS); We don't need the local variable: ephy_embed_prefs_update_font_settings (ephy_settings_get (EPHY_PREFS_WEB_SCHEMA), EPHY_PREFS_WEB_USE_GNOME_FONTS);
(In reply to Carlos Garcia Campos from comment #10) > [...] > ::: embed/ephy-embed-prefs.c > @@ +484,3 @@ > +{ > + GSettings *gsettings = ephy_settings_get (EPHY_PREFS_WEB_SCHEMA); > + ephy_embed_prefs_update_font_settings (gsettings, > EPHY_PREFS_WEB_USE_GNOME_FONTS); > > We don't need the local variable: > > ephy_embed_prefs_update_font_settings (ephy_settings_get > (EPHY_PREFS_WEB_SCHEMA), > EPHY_PREFS_WEB_USE_GNOME_FONTS); Done, thanks!
Comment on attachment 299700 [details] [review] Patch Proposal I reverted this in master because we're in hard code freeze; we can ask for a freeze exception, but TBH it's not super important, so I'd just wait until next week to commit it.
Hey Michael, sorry I did not realize of that. I say the accepted-commit_now flag and well... I "committed now" :) Anyway, I agree with you it's perhaps not super important, so I'm fine with waiting until the code freeze is lifted
(In reply to Mario Sánchez Prada from comment #13) > Hey Michael, sorry I did not realize of that. I say the accepted-commit_now > flag and well... I "committed now" :) Yes, that's what I meant indeed when I set that flag:-) > Anyway, I agree with you it's perhaps not super important, so I'm fine with > waiting until the code freeze is lifted I thought we hadn't released yet, so this was going to be a last minute change before the release and then the freeze.