davidllewellynjones accepted this revision.
davidllewellynjones added a comment.
This revision is now accepted and ready to land.


  I've made some suggestions and have a couple of queries, but they're all very 
minor.

INLINE COMMENTS

> DocumentDestination.cpp:125
> +         m_charactersToSkip = m_unicodeSkip;
> +     } else if (controlWord == "uc" && hasValue) {
> +         m_unicodeSkip = value;

A minor issue, but the spacing around the brackets here doesn't match the 
surrounding code.

> PictDestination.cpp:46
> +         m_format = "png";
> +     } else if ( controlWord == "dibitmap" ) {
> +         qCDebug(lcRtf) << "BMP";

The spec mentions `\wbitmap` is also a Windows device-dependent bitmap. Could 
that be sent through this branch too?

(see: http://latex2rtf.sourceforge.net/rtfspec_7.html#rtfspec_24)

> PictDestination.cpp:54
>       } else if ( controlWord == "pich" ) {
>              qCDebug(lcRtf) << "pict height: " << value;
>       } else if ( controlWord == "picscalex" ) {

The `\picw` and `\pich` keywords are now ignored; is this intentional? It looks 
like  they're mandatory, whereas the `\picwgoal`, `\pichgoal`, `\picscalex` and 
`\picscaley` which seem to be used now instead, are optional.

(see: http://latex2rtf.sourceforge.net/rtfspec_7.html#rtfspec_24)

> PictDestination.cpp:110
> +            qCWarning(lcRtf) << "Embedded picture in unknown format";
> +        }
>      }

Lines 87-110 seem to use different spacing around the brackets than elsewhere.

> TextDocumentRtfOutput.cpp:68
> +    {
> +     m_cursor->insertText(str);
>      }

The tabs/spaces are all over the place in this file already, but it probably 
makes sense to stick to one or the other nevertheless (spaces by the looks of 
it).

REPOSITORY
  R8 Calligra

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24943

To: pvuorela, davidllewellynjones
Cc: davidllewellynjones, Calligra-Devel-list, dcaliste, cochise, vandenoever

Reply via email to