<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/111681/">http://git.reviewboard.kde.org/r/111681/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 16th, 2013, 8:58 p.m. UTC, <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On August 16th, 2013, 9:17 p.m. UTC, <b>Christoph Feck</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On August 16th, 2013, 10:21 p.m. UTC, <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Christoph</p>
<br />
<p>On August 16th, 2013, 8:58 p.m. UTC, Christoph Feck wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Okular.</div>
<div>By Christoph Feck.</div>
<p style="color: grey;"><i>Updated Aug. 16, 2013, 8:58 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I tested the document from bug 322547 comment #3.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=253583">253583</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=322547">322547</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>core/textdocumentgenerator.cpp <span style="color: grey">(b260b3f)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111681/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>