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.
Details
Diff Detail
- Repository
- R8 Calligra
- Branch
- rtf_default_color
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 19440 Build 19458: arc lint + arc unit
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 | 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… |
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. |
filters/words/rtf/import/3rdparty/rtf-qt/src/ColorTableDestination.cpp | ||
---|---|---|
44 | Empty control word? Don't think that should be happening? |
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. |