> 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? > > Christoph Feck wrote: > 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. > > Jaydeep Solanki wrote: > Christoph, I guess Albert is talking about a scenario when the color > scheme has a non blue color for links. Like in my case, I previously used > "Krita-dark" color scheme, which has links in white. In such cases too Okular > needs to display links in blue.
Oh, you are right, for links it still uses the scheme color (where I also used dark blue). Sorry for the confusion. QTextCursor::insertHtml() creates CSS styles using the global palette for links, so changing the palette later has no effect. In other words, to change the color of links, we still need to change the CSS manually. Unless epub is the only format offering links, I would suggest to move the CSS fixup code from the epub generator to TextDocumentGenerator, so that other generators benefit from that as well. - 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
