> On Aug. 16, 2013, 8:58 p.m., Albert Astals Cid wrote: > > To be honest i'm a bit confused by all the different patches trying to fix > > the same thing, there's this one, the other one that tries to use > > kcolorscheme, the other one that tries to let the user choose. > > > > And what I don't understand is why we need these patches. I would > > understand that if an epub sets the background color it will also set the > > text color (so all is fine, none of "our" colors is used) and if no color > > is set, i would undertand that the color scheme by the user provides > > acceptable colors. > > > > So where is the problem? > > Christoph Feck wrote: > The problem is not documents that set colors, but those that set no > colors (for example, plain text files). > > In the rendering code, the pixmap is pre-filled with hard-coded white > color, but the text is actually rendered using the system's foreground color. > This leads to "white on white" with dark color schemes (which usually use > white text). > > There are, as you see, several ways to fix it. Why I believe this one > makes more sense that using the system's background color: See my rationale > at "Description" above. > > Albert Astals Cid wrote: > Ok, can you try Jaydeep's branch epub-qtextdoc according to > https://www.dropbox.com/s/mgf778ug6yjie2l/GSoC%20patches.odt the color issues > are fixed there. Can you confirm? Is the solution acceptable for you? > > Christoph Feck wrote: > From quickly checking commit 1823cf9998555e22c6f3d8701728882dc34b244b > (which is documented to fix the color issue), I see that Jaydeep injects CSS > code to change QTextDocument's default color to Qt::black. While this might > have the same result, my approach is simpler and uses less resources. > > Additionally, it looks like his patch is limited to epub, while my patch > works for all generators that use textdocumentgenerator.cpp? > > Jaydeep might clarify, if I am wrong. > > Christoph Feck wrote: > (Let me add that I wasn't aware about his work while I wrote the patch. I > have no intention to disturb his work.) > > Jaydeep Solanki wrote: > For sure, you patch addresses a bigger issue by fixing this for all > generators that use TextDocumentGenerator. > I'd be happy to remove my patch for this patch, but I'm still not finding > the links in blue color. > Albert can you please confirm, if the links show up in blue. > > Jaydeep Solanki wrote: > Okay, it seems that my color scheme was the culprit for not showing links > in blue. I changed color scheme and now it works. > Albert, IMHO this patch seems more appropriate. The other one with > KColorScheme doesn't seem so good, as it changes the background color of the > document which is not necessary. > > Albert Astals Cid wrote: > Hmmm, well, but you're saying that the links in blue still depend on the > color scheme, no? That's still not good. Christoph any idea why that may > happen?
I updated the patch to use the (brighter) blue color, instead of darkBlue. It should now match the color Jaydeep uses. I prefer the darker version, though, because it makes text with many links easier to read. - Christoph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/#review37994 ----------------------------------------------------------- On Aug. 20, 2013, 10:40 a.m., Christoph Feck wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111681/ > ----------------------------------------------------------- > > (Updated Aug. 20, 2013, 10:40 a.m.) > > > Review request for Okular. > > > Description > ------- > > As indicated in bug 322547, some documents do not specify a text color, and > probably assume the default text color to be black. QTextDocument, however, > defaults to using the system text color. > > This patch changes the default text color to Qt::black. It should affect > epub, fb2, odt, and plain text generators. > > I think it is better to use this approach instead of changing the paper color > to use the system background color (see bug 253583), because > > 1) the document might specify a text color in some places, > > 2) the user is able to change the fg/bg colors anyway using Okular's > Accessibility options, and those probably expect black on white. > > > This addresses bugs 253583 and 322547. > http://bugs.kde.org/show_bug.cgi?id=253583 > http://bugs.kde.org/show_bug.cgi?id=322547 > > > Diffs > ----- > > core/textdocumentgenerator.cpp b260b3f > > Diff: http://git.reviewboard.kde.org/r/111681/diff/ > > > Testing > ------- > > I tested the document from bug 322547 comment #3. > > > Thanks, > > Christoph Feck > >
_______________________________________________ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
