Summary: | WebKit GTK+ should use Font info from GtkStyle, and aliases, by default | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rodney Dawes <dobey> | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||
Severity: | Normal | CC: | christian, mrobinson, mrowe, zl29ah | ||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||
Version: | 523.x (Safari 3) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Rodney Dawes
2007-10-05 05:23:14 PDT
Created attachment 16543 [details]
Patch to update default font settings in GTK+
Created attachment 16544 [details]
Updated patch to match style guidelines better
Here's an updated version of the patch to match the style guidelines better, as mentioned by bdash on irc.
Created attachment 16545 [details]
Updated with inline declarations
Changed the defsize and family variables to be declared inline, as suggested by bdash on irc.
Comment on attachment 16545 [details] Updated with inline declarations + int defsize = pango_font_description_get_size(GTK_WIDGET(page)->style->font_desc); Does GtkStyle store a separate default font size for fixed vs. variable width fonts? "defsize" should be renamed "defaultSize" to adhere to our coding style guidelines <http://webkit.org/coding/coding-style.html>. + defsize /= 1024; Can we get this constant from a Pango header? If not, this constant should be a static const int at the top of the file. You'll need to add a ChangeLog entry before this can be landed. See <http://webkit.org/coding/contributing.html> for instructions. (In reply to comment #4) > Does GtkStyle store a separate default font size for fixed vs. variable width > fonts? It only provides one PangoFontDescription, so unfortunately no, it doesn't provide preferred monospace font information. I would certainly prefer to access the font settings from GNOME, but these are only available in gconf. > "defsize" should be renamed "defaultSize" to adhere to our coding style > guidelines <http://webkit.org/coding/coding-style.html>. > > + defsize /= 1024; > > Can we get this constant from a Pango header? If not, this constant should be a > static const int at the top of the file. This constant does appear to be defined in pango-types.h. I'm about to upload a patch which fixes this and the variable name. > You'll need to add a ChangeLog entry before this can be landed. See > <http://webkit.org/coding/contributing.html> for instructions. Also in updated patch that I'm about to attach. Created attachment 16615 [details]
Updated patch to resolve the issues in Adam's review
Comment on attachment 16615 [details]
Updated patch to resolve the issues in Adam's review
r=me
I'm still not convinced this patch is a good thing. Other platforms all default to the hard-coded values, but allow the user to explicitly override them. WebKit on Windows, for instance, does not default to the OSs default UI font. It defaults to the standard web fonts. (In reply to comment #8) > I'm still not convinced this patch is a good thing. Other platforms all > default to the hard-coded values, but allow the user to explicitly override > them. WebKit on Windows, for instance, does not default to the OSs default UI > font. It defaults to the standard web fonts. I'm not convinced as well: - You should not change the font names. Fontconfig is supposed to take care of that - Changing the default sizes is probably not good as well - You do not listen to the style change signal. If you would do you would in some situations overwrite users changes to the Settings. So if you still want to work on it feel free to implement webkitsettings (using properties) and show how to react on changing the style using GtkLauncher. Comment on attachment 16615 [details]
Updated patch to resolve the issues in Adam's review
I'm going to mark this r- based on Holger's comments.
In my opinion it is preferrable to use the user's font and the aliases respectively, as this is what the user expects with other widgets/ programs and trying to use fonts which are not installed feels a bit ugly in the first place. The widget must definitely react on style changes unless the (unimplemented) font property has been changed by client code. As for the implementation, this might not be the right way, but I'd like to see this done eventually. I'm not too comfortable with this just yet -- it might make sense to hold back here, but to keep the bug open. #15608 should probably be implemented first before we worry about this one, anyway. I think we want to shoot for compatibility with the web here. This patch is also many years out of date. We can address this issue in a fresh bug if it comes up. Thanks for the patch! |