[Okular-devel] Review Request 111681: TextDocumentGenerator: Use black as default text color

[Jaydeep Solanki]

> On Sept. 26, 2013, 2:10 a.m., Albert Astals Cid wrote:
> > I'm again confused, isn't the idea that we respect the colors specified on the file? Or not? Jaydeep, what's the final decision we took?
> 
> Christoph Feck wrote:
>     Albert, the issue is that some documents do not indicate text color, they just assume.
> 
> Albert Astals Cid wrote:
>     Right, so if they assume and we follow the color scheme, all is nice, no? You'll get your weird black on red as defined on your color scheme? Or that's something we don't like and we want it to be black on white not obeying your color scheme?
> 
> Christoph Feck wrote:
>     Yes, you can follow the color scheme, but then you have to do it for both background and foreground. But right now, Okular uses a white background.
>     
>     In the description I explained why I think following the color scheme might be bad: If you follow the color scheme, then you can break (i.e. render invisible) documents that _do_ set text colors, especially for users that use a dark color scheme.
>     
>     Of course, a third option would be to ignore all colors in the document, and force both background and foreground to use the color scheme, but then "colorful" documents would look boring.
> 
> Albert Astals Cid wrote:
>     Why would someone set the text color and not the background color? Does that happen?

Okay, here's what we need :
i)  if text color is explicitly specified in html/css use that, else use black.
ii) if background color is specified use that, else use white.
iii)link color should be independent of the color scheme ( i.e. it should be blue )

What your patch does,
i)  if no color is specified, it uses black for text, but if a color is specified it uses that color ( I checked ).
ii) Link color still depends on the color scheme. ( I saw you have included a line to set the link color to blue, but it follows the theme color )

It would be nice if you can do something about he background color too, because the implementation that we have in epubs takes care of that (see branch 'epub-qtextdoc').


- Jaydeep


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111681/#review40816
-----------------------------------------------------------


On Aug. 20, 2013, 4:10 p.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, 4:10 p.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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20130926/4c8b31b2/attachment.html>