GNOME Bugzilla – Bug 631924
patch to fix subpixel font rendering with openoffice
Last modified: 2011-02-25 14:31:10 UTC
Created attachment 172138 [details] [review] patch from ubuntu OpenOffice, when run within GNOME, pays no attention to fontconfig's lcdfilter setting. Fonts are lcdfiltered nicely in all apps except OpenOffice. OpenOffice doesn't entirely respect fontconfig. It still uses the legacy X resource manager for some font settings (see `xrdb -query`). Gnome-settings-daemon kindly exports settings like Xft.hinting and Xft.hintstyle to Xrm, but not Xft.lcdfilter. Ubuntu has a simple patch, 40_xres_lcddefault.patch, which works for me. https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/271283
Comment on attachment 172138 [details] [review] patch from ubuntu Please mark patches as such in bugzilla.
Review of attachment 172138 [details] [review]: That looks like a good way of solving the problem, but the patch needs to be updated to match current code.
Created attachment 172143 [details] [review] patch for git
Review of attachment 172143 [details] [review]: I'd feel more comfortable if you reused the existing variables that track RGBA usage. ::: plugins/xsettings/gsd-xsettings-manager.c @@ +288,3 @@ + + /* priv helper for OOO lcdfilter */ + gboolean use_rgba; Why do you need that when you could check the value of ->rgba already?
(In reply to comment #4) > Review of attachment 172143 [details] [review]: > > I'd feel more comfortable if you reused the existing variables that track RGBA > usage. > > ::: plugins/xsettings/gsd-xsettings-manager.c > @@ +288,3 @@ > + > + /* priv helper for OOO lcdfilter */ > + gboolean use_rgba; > > Why do you need that when you could check the value of ->rgba already? I don't know why it was done that way. I didn't write the patch. I only tweaked it to fit the code in git.
Created attachment 172460 [details] [review] revised patch for git You're right, it's nicer this way.
Review of attachment 172460 [details] [review]: Looks alright apart from the strcmp() usage. Could you please replace that with "g_str_equal()" instead?
Created attachment 172471 [details] [review] revised patch that uses g_str_equal() instead of strcmp()
Fixed some indentation/spacing problems with the patch. Please provide git-formatted patches in the future, so that authorship is maintained.
And I thought I did alright. Obviously I don't do this often. I don't write software for a living. I just report bugs occasionally. I've used git maybe two or three times. I did make an effort to make the indentation match the existing code. The adjacent code used spaces not tabs, so I replaced my tabs with spaces to align them. What went wrong? And what do you mean by git-formatted? I made the patch with `git diff`. I think "git-formatted" must mean something special. And how is authorship maintained? Too many questions. You're just going to tell me to RTFM. But please answer me. It'll quickly fill some gaps in my knowledge and maybe inspire me to RTFM.
(In reply to comment #10) > And I thought I did alright. Obviously I don't do this often. I don't write > software for a living. I just report bugs occasionally. I've used git maybe two > or three times. > > I did make an effort to make the indentation match the existing code. The > adjacent code used spaces not tabs, so I replaced my tabs with spaces to align > them. What went wrong? > > And what do you mean by git-formatted? I made the patch with `git diff`. I > think "git-formatted" must mean something special. And how is authorship > maintained? > > Too many questions. You're just going to tell me to RTFM. But please answer me. > It'll quickly fill some gaps in my knowledge and maybe inspire me to RTFM. Nevermind, I read TFM.
(In reply to comment #10) > And I thought I did alright. Obviously I don't do this often. I don't write > software for a living. I just report bugs occasionally. I've used git maybe two > or three times. > > I did make an effort to make the indentation match the existing code. The > adjacent code used spaces not tabs, so I replaced my tabs with spaces to align > them. What went wrong? Spaces between function name and opening bracket, lining up the second line of a function (though the old code didn't do that), that sort of thing. > And what do you mean by git-formatted? I made the patch with `git diff`. I > think "git-formatted" must mean something special. And how is authorship > maintained? First you'd need to commit the patch (with a short log message), and a longer explanation. With git properly configured this would show you as the author of the patch. Then you'd use "git format-patch" to generate a stand-alone patch. > Too many questions. You're just going to tell me to RTFM. But please answer me. > It'll quickly fill some gaps in my knowledge and maybe inspire me to RTFM. See: http://live.gnome.org/Git/Developers for some details
In my opinion this patch does The Wrong Thing(tm). Exporting Xft.lcdfilter overrides fontconfig, thus making lcdfilter unconfigurable[1]. Which LCD filter works best is a personal choice and ought to remain configurable, even if it means editing fontconfig files. Perhaps, since g-s-d is already linking against fc here the patch could be updated to try and query fc for what it knows is the lcdfilter option. That way it remains both consistent and configurable. I argue that it is a personal choice because both lcddefault a lcdlight are too blurry for my eyes. http://www.spasche.net/files/lcdfiltering/ has a good comparison. Note that the patents on the bytecode interpreter in freetype have expired and that the autohinter is now a depreciated feature according to the freetype sources. [1] Mostly unconfigurable. With the existing behaviour lcdfilter can be overridden by defining it in the .Xsettings file which the xrdb backend will load. However, it isn't loaded until after some applications have started, like the panel and nautilus in some occasions. The loading of the setting does not propagate it too these currently running applications.
*** Bug 640133 has been marked as a duplicate of this bug. ***
Bug 640133 comment 4 has explanations on why this is a bad idea. Reverted in master. commit d52ead7817612dc74cd2f8dcecedb8d11b8d900c Author: Bastien Nocera <hadess@hadess.net> Date: Wed Feb 23 16:58:02 2011 +0000 Revert "xsettings: Export Xft.lcdfilter for OO.o's benefit" This reverts commit 6cf315249ab27d4396b0f5b5edb1e689a5cafc68. See https://bugzilla.gnome.org/show_bug.cgi?id=640133#c4 for details.