GNOME Bugzilla – Bug 754842
Use system document font
Last modified: 2015-10-19 16:46:06 UTC
Change the entry text font in Almanah Diary main window and use the configured system document font because Cantarell is well defined for UI but not for large texts documents, like diary entries. Other option would be to include or own font, like some kind of Serif.
Created attachment 313129 [details] [review] UI: Using system document font The GtkTextView now uses the configured system document font updating the font if the user changes the setting. The width of the GtkTextView is calculated with a random 15 words sentences which can be translated in to adjust the size for different languages.
Attachment 313129 [details] pushed as be98cf8 - UI: Using system document font
Review of attachment 313129 [details] [review]: ::: src/main-window.c @@ +165,3 @@ gtk_header_bar_set_show_close_button (GTK_HEADER_BAR (self->priv->header_bar), TRUE); + + self->priv->desktop_interface_settings; What’s that for? @@ +1380,3 @@ + font_name = g_settings_get_string (self->priv->desktop_interface_settings, ALMANAH_MAIN_WINDOW_DOCUMENT_FONT_KEY_NAME); + css_font = g_strdup_printf (".almanah-mw-entry-view { font: %s; }", font_name); + css_provider = gtk_css_provider_get_default (); If you load into the default CSS provider, won’t you potentially overwrite existing CSS coming from other sources? I think you want to create a new GtkCssProvider. @@ +1391,3 @@ + gtk_widget_set_size_request(GTK_WIDGET (self->priv->entry_scrolled), + fixed_width + gtk_widget_get_margin_start (GTK_WIDGET (self->priv->entry_view)) + gtk_widget_get_margin_end (GTK_WIDGET (self->priv->entry_view)), + -1); I don’t think it should be necessary to set the size request for the scrolled window, because it should have its size recalculated when the entry_view size request is set. @@ +1400,3 @@ + +int +mw_get_font_width (GtkWidget *widget, const gchar *font_name) Should be static gint. @@ +1416,3 @@ + pango_layout_get_pixel_size (layout, &width, &height); + + g_object_unref (layout); @desc is leaked at the end of this function. @@ +1423,3 @@ + +void +mw_desktop_interface_settings_changed (__attribute__ ((unused)) GSettings *settings, gchar *key, gpointer user_data) Should be static. Use G_GNUC_UNUSED instead of __attribute__((unused)) — it’s more portable. @key should be const gchar*. @@ +1425,3 @@ +mw_desktop_interface_settings_changed (__attribute__ ((unused)) GSettings *settings, gchar *key, gpointer user_data) +{ + if (g_ascii_strcasecmp (ALMANAH_MAIN_WINDOW_DOCUMENT_FONT_KEY_NAME, key) != 0) Why strcasecmp()? The case of the string you receive from GSettings should always be consistent with the GSettings schema it comes from.
(In reply to Philip Withnall from comment #3) > Review of attachment 313129 [details] [review] [review]: > > ::: src/main-window.c > @@ +165,3 @@ > gtk_header_bar_set_show_close_button (GTK_HEADER_BAR > (self->priv->header_bar), TRUE); > + > + self->priv->desktop_interface_settings; > > What’s that for? Just forgotten the "= NULL;" > > @@ +1380,3 @@ > + font_name = g_settings_get_string (self->priv->desktop_interface_settings, > ALMANAH_MAIN_WINDOW_DOCUMENT_FONT_KEY_NAME); > + css_font = g_strdup_printf (".almanah-mw-entry-view { font: %s; }", > font_name); > + css_provider = gtk_css_provider_get_default (); > > If you load into the default CSS provider, won’t you potentially overwrite > existing CSS coming from other sources? I think you want to create a new > GtkCssProvider. Well, you're right if some widget has added a style named "almanah-mw-entry-view"... Do you think it's better to create a new provider? The code has been copied from some gnome core app, but if you think that it's better I'll change it... > > @@ +1391,3 @@ > + gtk_widget_set_size_request(GTK_WIDGET (self->priv->entry_scrolled), > + fixed_width + gtk_widget_get_margin_start (GTK_WIDGET > (self->priv->entry_view)) + gtk_widget_get_margin_end (GTK_WIDGET > (self->priv->entry_view)), > + -1); > > I don’t think it should be necessary to set the size request for the > scrolled window, because it should have its size recalculated when the > entry_view size request is set. Perhaps I'm doing something wrong, but If you don't set the scrolled window size, the application window can be resized with a minor width that the entry width... > > @@ +1400,3 @@ > + > +int > +mw_get_font_width (GtkWidget *widget, const gchar *font_name) > > Should be static gint. Yep. > > @@ +1416,3 @@ > + pango_layout_get_pixel_size (layout, &width, &height); > + > + g_object_unref (layout); > > @desc is leaked at the end of this function. Ouch!, yes. > > @@ +1423,3 @@ > + > +void > +mw_desktop_interface_settings_changed (__attribute__ ((unused)) GSettings > *settings, gchar *key, gpointer user_data) > > Should be static. Use G_GNUC_UNUSED instead of __attribute__((unused)) — > it’s more portable. @key should be const gchar*. Thanks, good points. > > @@ +1425,3 @@ > +mw_desktop_interface_settings_changed (__attribute__ ((unused)) GSettings > *settings, gchar *key, gpointer user_data) > +{ > + if (g_ascii_strcasecmp (ALMANAH_MAIN_WINDOW_DOCUMENT_FONT_KEY_NAME, key) > != 0) > > Why strcasecmp()? The case of the string you receive from GSettings should > always be consistent with the GSettings schema it comes from. eeehhh, Yes, moving to strcmp.
Created attachment 313549 [details] [review] UI: Fixed some issues
(In reply to Álvaro Peña from comment #4) > (In reply to Philip Withnall from comment #3) > > > > @@ +1416,3 @@ > > + pango_layout_get_pixel_size (layout, &width, &height); > > + > > + g_object_unref (layout); > > > > @desc is leaked at the end of this function. > > Ouch!, yes. > Not really, because it's the new description for the text view pango layout (with the free we get a core dump, just tested it bad before to attach the patch)
Created attachment 313550 [details] [review] UI: Fixed some issues
Created attachment 313551 [details] [review] UI: Fixed some issues Discovered pango_font_description_free :P
(In reply to Álvaro Peña from comment #6) > (In reply to Álvaro Peña from comment #4) > > (In reply to Philip Withnall from comment #3) > > > > > > @@ +1416,3 @@ > > > + pango_layout_get_pixel_size (layout, &width, &height); > > > + > > > + g_object_unref (layout); > > > > > > @desc is leaked at the end of this function. > > > > Ouch!, yes. > > > > Not really, because it's the new description for the text view pango layout > (with the free we get a core dump, just tested it bad before to attach the > patch) Must read the docs...
Review of attachment 313551 [details] [review]: Looks good to me. :-)
(In reply to Álvaro Peña from comment #4) > > @@ +1380,3 @@ > > + font_name = g_settings_get_string (self->priv->desktop_interface_settings, > > ALMANAH_MAIN_WINDOW_DOCUMENT_FONT_KEY_NAME); > > + css_font = g_strdup_printf (".almanah-mw-entry-view { font: %s; }", > > font_name); > > + css_provider = gtk_css_provider_get_default (); > > > > If you load into the default CSS provider, won’t you potentially overwrite > > existing CSS coming from other sources? I think you want to create a new > > GtkCssProvider. > > Well, you're right if some widget has added a style named > "almanah-mw-entry-view"... Do you think it's better to create a new > provider? The code has been copied from some gnome core app, but if you > think that it's better I'll change it... I think it would probably not make a difference in practice, but changing it would result in cleaner code using a better approach. > > > > @@ +1391,3 @@ > > + gtk_widget_set_size_request(GTK_WIDGET (self->priv->entry_scrolled), > > + fixed_width + gtk_widget_get_margin_start (GTK_WIDGET > > (self->priv->entry_view)) + gtk_widget_get_margin_end (GTK_WIDGET > > (self->priv->entry_view)), > > + -1); > > > > I don’t think it should be necessary to set the size request for the > > scrolled window, because it should have its size recalculated when the > > entry_view size request is set. > > Perhaps I'm doing something wrong, but If you don't set the scrolled window > size, the application window can be resized with a minor width that the > entry width... Ah, I see. That makes sense, because the scrolled window is set to allow horizontal scrolling. What about if you set its vscrollbar-policy to GTK_POLICY_NEVER? (Assuming that fits with the rest of the UI design — it should cause lines to wrap in the entry_view, rather than start scrolling horizontally.)
Created attachment 313615 [details] [review] UI: Fixed some issues Followed Philip suggestions
(In reply to Philip Withnall from comment #11) > (In reply to Álvaro Peña from comment #4) > > > @@ +1380,3 @@ > > > + font_name = g_settings_get_string (self->priv->desktop_interface_settings, > > > ALMANAH_MAIN_WINDOW_DOCUMENT_FONT_KEY_NAME); > > > + css_font = g_strdup_printf (".almanah-mw-entry-view { font: %s; }", > > > font_name); > > > + css_provider = gtk_css_provider_get_default (); > > > > > > If you load into the default CSS provider, won’t you potentially overwrite > > > existing CSS coming from other sources? I think you want to create a new > > > GtkCssProvider. > > > > Well, you're right if some widget has added a style named > > "almanah-mw-entry-view"... Do you think it's better to create a new > > provider? The code has been copied from some gnome core app, but if you > > think that it's better I'll change it... > > I think it would probably not make a difference in practice, but changing it > would result in cleaner code using a better approach. Ok. > > > > > > > @@ +1391,3 @@ > > > + gtk_widget_set_size_request(GTK_WIDGET (self->priv->entry_scrolled), > > > + fixed_width + gtk_widget_get_margin_start (GTK_WIDGET > > > (self->priv->entry_view)) + gtk_widget_get_margin_end (GTK_WIDGET > > > (self->priv->entry_view)), > > > + -1); > > > > > > I don’t think it should be necessary to set the size request for the > > > scrolled window, because it should have its size recalculated when the > > > entry_view size request is set. > > > > Perhaps I'm doing something wrong, but If you don't set the scrolled window > > size, the application window can be resized with a minor width that the > > entry width... > > Ah, I see. That makes sense, because the scrolled window is set to allow > horizontal scrolling. What about if you set its vscrollbar-policy to > GTK_POLICY_NEVER? (Assuming that fits with the rest of the UI design — it > should cause lines to wrap in the entry_view, rather than start scrolling > horizontally.) Yes, you're right.
Review of attachment 313615 [details] [review]: ::: src/main-window.c @@ +1384,3 @@ + GtkStyleContext *style_context; + + self->priv->css_provider = gtk_css_provider_get_default (); You're still using the default CSS provider, rather than a new one?
Created attachment 313622 [details] [review] UI: Fixed some issues
(In reply to Philip Withnall from comment #14) > Review of attachment 313615 [details] [review] [review]: > > ::: src/main-window.c > @@ +1384,3 @@ > + GtkStyleContext *style_context; > + > + self->priv->css_provider = gtk_css_provider_get_default (); > > You're still using the default CSS provider, rather than a new one? My mistake.
Review of attachment 313622 [details] [review]: Looks good to me. :-)
Attachment 313622 [details] pushed as 43cd522 - UI: Fixed some issues