Use RTF default color as default Qt format
ClosedPublic

Authored by pvuorela on Dec 3 2019, 3:06 PM.

Details

Summary

RTF spec says a color entry without anything defined is a "default
color", but doesn't specify more what that is. In practice seems to
work nicer now when such a default color is e.g. used to clear a
previously set text background color. Earlier version resulted in
black background and black text.

Diff Detail

Repository
R8 Calligra
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
pvuorela created this revision.Dec 3 2019, 3:06 PM
Restricted Application added a project: Calligra: 3.0. · View Herald TranscriptDec 3 2019, 3:06 PM
Restricted Application added a subscriber: Calligra-Devel-list. · View Herald Transcript
pvuorela requested review of this revision.Dec 3 2019, 3:06 PM

Just a suggestion to avoid adding a variable which meaning may be redundant with the state of an already existing one. Otherwise LGTM.

filters/words/rtf/import/3rdparty/rtf-qt/src/ColorTableDestination.cpp
26

We let the initialisation by default m_currentColor(QColor())

37

Before setting the red value, we add:

if (! m_currentColor.isValid())
  m_currentColor = QColor::Black;

To avoid uninitialised values in other channels.

39

The same for blue and green.

54

Here we pass m_currentColor, inconditionaly, since the invalid status is equivalent to the flag being false.

63–64

And finally, we reset to QColor(), instead of black.

filters/words/rtf/import/3rdparty/rtf-qt/src/ColorTableDestination.h
45

I'm wondering, if we could use the invalid status of QColor as this flag ?

See later…

pvuorela added inline comments.Dec 3 2019, 4:26 PM
filters/words/rtf/import/3rdparty/rtf-qt/src/ColorTableDestination.h
45

Should work, but I'm not sure would it be much better. Saves one boolean instance during loading a file, but then feels a bit more complex.

@pvuorela as you prefer, anyway, it's a question of taste, I find it myself easier to follow if the information is on one variable only. But your solution is good also, the file is small and is kept small so easy to follow.

filters/words/rtf/import/3rdparty/rtf-qt/src/ColorTableDestination.cpp
44

It looks like there may be a distinction to be made between an empty control word and an unhandled control word. In the case of an unhandled control word, clearing the colour seems sensible, but then in the case of an empty control word, the debug output isn't needed.

pvuorela added inline comments.Dec 31 2019, 1:22 PM
filters/words/rtf/import/3rdparty/rtf-qt/src/ColorTableDestination.cpp
44

Empty control word? Don't think that should be happening?

pvuorela added inline comments.Dec 31 2019, 1:28 PM
filters/words/rtf/import/3rdparty/rtf-qt/src/ColorTableDestination.cpp
44

Or well, the code might return such, but think the file in that case is broken and as such ok to complain about on debug. Also a bit separate from the changes here.

This looks good to me, and certainly gave better results for me using files with the 'auto' colour.

filters/words/rtf/import/3rdparty/rtf-qt/src/ColorTableDestination.cpp
44

Yeah, this was my misunderstanding of the spec. I don't see a problem with what you're doing here.

This revision is now accepted and ready to land.Dec 31 2019, 1:55 PM
This revision was automatically updated to reflect the committed changes.