dcaliste added a comment.
Just a suggestion to avoid adding a variable which meaning may be redundant
with the state of an already existing one. Otherwise LGTM.
INLINE COMMENTS
> ColorTableDestination.cpp:26
> ColorTableDestination::ColorTableDestination( Reader *reader,
> AbstractRtfOutput *output, const QString &name ) :
> - Destination( reader, output, name )
> + Destination( reader, output, name ), m_currentColor(Qt::black),
> m_colorSet(false)
> {
We let the initialisation by default m_currentColor(QColor())
> ColorTableDestination.cpp:37
> if ( controlWord == "red" ) {
> m_currentColor.setRed( value );
> } else if (controlWord == "green" ) {
Before setting the red value, we add:
if (! m_currentColor.isValid())
m_currentColor = QColor::Black;
To avoid uninitialised values in other channels.
> ColorTableDestination.cpp:39
> } else if (controlWord == "green" ) {
> m_currentColor.setGreen( value );
> } else if (controlWord == "blue" ) {
The same for blue and green.
> ColorTableDestination.cpp:54
> if ( plainText == ";" ) {
> - m_output->appendToColourTable( m_currentColor );
> + m_output->appendToColourTable( m_colorSet ? m_currentColor :
> QColor() );
> resetCurrentColor();
Here we pass m_currentColor, inconditionaly, since the invalid status is
equivalent to the flag being false.
> ColorTableDestination.cpp:63
> {
> m_currentColor = Qt::black;
> + m_colorSet = false;
And finally, we reset to QColor(), instead of black.
> ColorTableDestination.h:45
> QColor m_currentColor;
> + bool m_colorSet;
> };
I'm wondering, if we could use the invalid status of QColor as this flag ?
See later…
REPOSITORY
R8 Calligra
REVISION DETAIL
https://phabricator.kde.org/D25715
To: pvuorela, davidllewellynjones, dcaliste
Cc: Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, cochise,
vandenoever
