D15279: Store all annotation color attributes as ARGB string

[Tobias Deiminger]

This revision was automatically updated to reflect the committed changes.
Closed by commit R223:d61cef693d96: Store all annotation color attributes as 
ARGB string (authored by dileepsankhla, committed by tobiasdeiminger).

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15279?vs=41008=41163

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

AFFECTED FILES
  conf/editannottooldialog.cpp
  core/annotations.cpp
  ui/data/tools.xml

To: tobiasdeiminger, sander
Cc: dileepsankhla, sander, okular-devel, ngraham, aacid

D15279: Store all annotation color attributes as ARGB string

[Dileep Sankhla]

dileepsankhla added a comment.


  Thank you Tobias for making it landable. Please go ahead :)

REPOSITORY
  R223 Okular

BRANCH
  annotation_rgba

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

To: tobiasdeiminger, sander
Cc: dileepsankhla, sander, okular-devel, ngraham, aacid

D15279: Store all annotation color attributes as ARGB string

[Tobias Deiminger]

tobiasdeiminger added a subscriber: dileepsankhla.
tobiasdeiminger added a comment.


  I'm going to push, and want to do it on behalf of @dileepsankhla who did the 
original work. I.e. my Phab revision, but git commit author should be him.
  Has anybody tried with `arc land`? Will this work?
  
$ git commit --amend --author="Original Author "
$ arc land

REPOSITORY
  R223 Okular

BRANCH
  annotation_rgba

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

To: tobiasdeiminger, sander
Cc: dileepsankhla, sander, okular-devel, ngraham, aacid

D15279: Store all annotation color attributes as ARGB string

[Tobias Deiminger]

tobiasdeiminger edited the summary of this revision.

REPOSITORY
  R223 Okular

BRANCH
  annotation_rgba

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

To: tobiasdeiminger, sander
Cc: sander, okular-devel, ngraham, aacid

D15279: Store all annotation color attributes as ARGB string

[Oliver Sander]

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


  Okay, my difficulties in reproducing the described behavior are explained 
now: I had an old file `okularpartrc` in `.kde/share/config/`, and the 
`Kdelibs4ConfigMigrator` kept migrating from that old file every time is 
started Okular without an `okularpartrc`.  With that old file removed 
everything works as intended.

REPOSITORY
  R223 Okular

BRANCH
  annotation_rgba

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

To: tobiasdeiminger, sander
Cc: sander, okular-devel, ngraham, aacid

D15279: Store all annotation color attributes as ARGB string

[Oliver Sander]

sander added a comment.


  It really does take the file `tools.xml` from my development installation 
(tested with `strace`).  Indeed, I deliberately purged my distro's okular from 
my computer a while ago, to avoid confusions like this.

REPOSITORY
  R223 Okular

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

To: tobiasdeiminger
Cc: sander, okular-devel, ngraham, aacid

D15279: Store all annotation color attributes as ARGB string

[Tobias Deiminger]

tobiasdeiminger added a comment.


  > So what now?  The patch seems to do something reasonable, but it does not 
do what you claim it does.
  
  It must work, else we couldn't go on. I've got sneaking suspicion, a wrong 
tools.xml is loaded in your setup. Can you watch okular in strace
  
$ strace -e openat /devroot/usr/bin/okular 2>&1 | grep -F tools.xml
openat(AT_FDCWD, "/devroot/usr/share/okular/tools.xml", O_RDONLY|O_CLOEXEC) 
= 10
  
  Does okular really take the new patched tools.xml from your development 
installation? Or does it maybe load that one from your distro, which would be 
wrong and explain the misbehavior.

REPOSITORY
  R223 Okular

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

To: tobiasdeiminger
Cc: sander, okular-devel, ngraham, aacid

D15279: Store all annotation color attributes as ARGB string

[Oliver Sander]

sander added a comment.


  
  
  > When is your new okularpartrc created exactly? Afaikt, it should not be 
created right on startup, but only when you modify some tool in the config 
dialog.
  
  No. It is created right at program startup.  I don't even have to open a 
document.
  
  > Are you sure you looked at , and not at ? The first one is what matters.
  
  Yes.
  
  > Existing annotations don't consider alpha channel, so you don't notice it 
for now. But it would have negative consequence for typewriter.
  
  I wanted to check that the new ARGB code handles an existing okularpartrc 
with RGB colors correctly.  And it does.
  
  So what now?  The patch seems to do something reasonable, but it does not do 
what you claim it does.

REPOSITORY
  R223 Okular

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

To: tobiasdeiminger
Cc: sander, okular-devel, ngraham, aacid

D15279: Store all annotation color attributes as ARGB string

[Tobias Deiminger]

tobiasdeiminger added a comment.


  In D15279#320991 , @sander wrote:
  
  > I cannot reproduce Part 1 of your test plan.  When I delete my 
`okularpartrc` and start Okular (with the patch), then the freshly created 
`okularpartrc` file
  
  
  When is your new okularpartrc created exactly? Afaikt, it should not be 
created right on startup, but only when you modify some tool in the config 
dialog.
  
  > contains all colors as RGB, not ARGB.
  
  Strange, for me it works. New okularpartrc looks like this
  
AnnotationTools=1 [...]
  
  Are you sure you looked at , and not at ? The first one is what matters.
  
  > This doesn't seem to have any negative consequences, however.
  
  Existing annotations don't consider alpha channel, so you don't notice it for 
now. But it would have negative consequence for typewriter.

REPOSITORY
  R223 Okular

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

To: tobiasdeiminger
Cc: sander, okular-devel, ngraham, aacid

D15279: Store all annotation color attributes as ARGB string

[Oliver Sander]

sander added a comment.


  I cannot reproduce Part 1 of your test plan.  When I delete my `okularpartrc` 
and start Okular (with the patch), then the freshly created `okularpartrc` file 
contains all colors as RGB, not ARGB.  This doesn't seem to have any negative 
consequences, however.
  
  Part 2 seems to work alright.

REPOSITORY
  R223 Okular

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

To: tobiasdeiminger
Cc: sander, okular-devel, ngraham, aacid

D15279: Store all annotation color attributes as ARGB string

[Tobias Deiminger]

tobiasdeiminger added a dependent revision: D15204: Add typewriter annotation 
tool.

REPOSITORY
  R223 Okular

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

To: tobiasdeiminger
Cc: okular-devel, ngraham, aacid

D15279: Store all annotation color attributes as ARGB string

[Tobias Deiminger]

tobiasdeiminger created this revision.
Herald added a project: Okular.
Herald added a subscriber: okular-devel.
tobiasdeiminger requested review of this revision.

REVISION SUMMARY
  This is mainly preparation for D15204  
(typewriter), where storing RGB won't be sufficient any longer.
  Typewriter will need transparent background (alpha=0x00), which can only be 
expressed as ARGB string.
  
  Current code handles name format identical for all annotations. It doesn't 
hurt to store all annotations in ARGB format, so instead of introducing special 
handling for typewriter, let's store all annotation color attributes as ARGB 
string.

TEST PLAN
  - when [Reviews] section in okularpartrc is initially generated, all 
annotation color attributes are in #AARRGGBB format
  - saving into archive stores color with alpha channel (#AARRGGBB), for all 
kind of annotations

REPOSITORY
  R223 Okular

BRANCH
  annotation_rgba

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

AFFECTED FILES
  conf/editannottooldialog.cpp
  core/annotations.cpp
  ui/data/tools.xml

To: tobiasdeiminger
Cc: okular-devel, ngraham, aacid