D13203: Add Typewriter annotation tool in Okular

[Tobias Deiminger]

tobiasdeiminger added inline comments.

INLINE COMMENTS

> dileepsankhla wrote in editannottooldialog.cpp:128
> There is no any problem in the newer Okular version as what I have found is that it respects both #RRGGBB and #AARRGGBB formats. I don't know about the older version but it should be both forward and backward compatible. And I agree there should be consistency in the color formats.
> So if we adopt the new color format in the default tools in tools.xml, do we need to alter all the color entries? Or is it okay to have two color formats?

> I don't know about the older version but it should be both forward and backward compatible.

Test for forwards compatibility (def: <https://en.wikipedia.org/wiki/Forward_compatibility> "allows a system to accept input intended for a later version of itself") is easy.

- open a test.txt file in Okular with D13203 <https://phabricator.kde.org/D13203> applied
- add some annotations (inline note, line, ...)
- save document to test.okular document archive

Because tools.xml was not yet patched to #AARRGGBB for all annotations, just let's simulate this patch:

- unpack the okular archive

  $ unzip test.okular

- edit metadata.xml and change all <annotation>...color=...</annotation> attributes to #AARRGGBB format.

  $ vi metadata.xml # do :%s/color="#/color="#00/g

- pack the archive again

  $ zip test_aarrggbb.okular content.xml test.txt metadata.xml



- Open test_aarrggbb.okular in older Okular (i.e. compiled from master branch, or just a recent Ubuntu installation)

  $ okular test_aarrggbb.okular

- Check if annotations are painted with colors as expected.

Just tried it, it works.

Can you please come up with similar test steps for backwards compatiblity (def: <https://en.wikipedia.org/wiki/Backward_compatibility> "allows for interoperability with an older legacy system") and perform the test?

Reconsidering it, I'm not sure if it's worth the while to open a new Differential patch for the tools.xml alpha channel patch. Probably it's ok to incorporate it in D13203 <https://phabricator.kde.org/D13203>.

But let's at least write both the forwards and backwards compatibility test steps down in the Test Plan <https://secure.phabricator.com/book/phabricator/article/differential_test_plans> field of D13203 <https://phabricator.kde.org/D13203>. It's yet empty. May you please do this?

> dileepsankhla wrote in editannottooldialog.cpp:128
> There is no any problem in the newer Okular version as what I have found is that it respects both #RRGGBB and #AARRGGBB formats. I don't know about the older version but it should be both forward and backward compatible. And I agree there should be consistency in the color formats.
> So if we adopt the new color format in the default tools in tools.xml, do we need to alter all the color entries? Or is it okay to have two color formats?

> So if we adopt the new color format in the default tools in tools.xml, do we need to alter all the color entries? Or is it okay to have two color formats?

I'm not sure if I understand this question. Of course not all color entries that appear somewhere in okular source code 😃 We shall just be consistent with

  const QString color = m_stubann->style().color().name(QColor::HexArgb);

So adapt everything to #AARRGGBB that will affect Okular::Annotation::style()->color(). Afaikt this are the color=... attributes of <annotation> elements in tools.xml. But not the color=... attributes of <engine> elements, because they don't affect Okular::Annotation::style()->color(). Are there other places?

REPOSITORY
  R223 Okular

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

To: dileepsankhla, tobiasdeiminger
Cc: ltoscano, ngraham, tobiasdeiminger, aacid, okular-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20180613/7d599e7f/attachment.html>