D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz closed this revision.

REPOSITORY
  R223 Okular

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  +1, I say shipit!

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz added a comment.


  Final version of the new UI components:
  
  F7092260: Screenshot_20190724_215313.png 

  
  F7092263: Screenshot_20190724_215331.png 

  
  If there are not further comments, I am going to land this tomorrow evening.

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz updated this revision to Diff 62500.
simgunz added a comment.


  - Fix messages and i18nc

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22064?vs=62454=62500

BRANCH
  improve-stamp-annotation

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

AFFECTED FILES
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/guiutils.cpp
  ui/guiutils.h
  ui/pagepainter.cpp
  ui/pageviewannotator.cpp

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Nathaniel Graham]

ngraham added a comment.


  In D22064#501601 , @simgunz wrote:
  
  > In D22064#501557 , @ngraham 
wrote:
  >
  > > Exactly what I was imagining!I might also add at the beginning something 
like: "Warning:  feature is considered experimental."
  >
  >
  > Is it ok like this?
  >  F7091319: Screenshot_20190724_182254.png 

  >
  > The longer sentence would wrap given the dialog default size
  >  F7091324: Screenshot_20190724_182457.png 

  >
  > Should  'Experimental' after the column be capitalzed or lowercase?
  >
  > In the k18n guide ( tag), it is capitalized, but maybe it is just 
an example. Does the HIG say anything about this?
  >  https://api.kde.org/frameworks/ki18n/html/prg_guide.html
  
  
  The HIG doesn't have specifically anything to say about this, but the "W" in 
"Warning" should definitely be capizalized. As for the other question, the 
rules of American English grammar dictate that you only capitalize the first 
letter of the first word after a colon if it's a full sentence, and even then 
it's optional. So in the first image, the "E" in "Experimental sould definitely 
be un-capitalized. In the second image, the "T" in "This" could be capitalized 
only if you feel like it.
  
  I think the shorter version is better.

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz added a comment.


  In D22064#501557 , @ngraham wrote:
  
  > Exactly what I was imagining!I might also add at the beginning something 
like: "Warning:  feature is considered experimental."
  
  
  Is it ok like this?
  F7091319: Screenshot_20190724_182254.png 

  
  The longer sentence would wrap given the dialog default size
  F7091324: Screenshot_20190724_182457.png 

  
  Should  'Experimental' after the column be capitalzed or lowercase?
  
  In the k18n guide ( tag), it is capitalized, but maybe it is just an 
example. Does the HIG say anything about this?
  https://api.kde.org/frameworks/ki18n/html/prg_guide.html

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  Looks good to go from my perspective. My vote goes to shipping it after you 
tweak the string in the way I suggested. Nice work! 

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Nathaniel Graham]

ngraham edited the summary of this revision.
ngraham edited the test plan for this revision.

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Nathaniel Graham]

ngraham added a comment.


  Exactly what I was imagining!I might also add at the beginning something 
like: "Warning: this feature is considered experimental."

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz added a comment.


  F7089222: Screenshot_20190724_085941.png 


REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz updated this revision to Diff 62454.
simgunz added a comment.


  - Add warning regarding broken stamp support in PDF

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22064?vs=62335=62454

BRANCH
  improve-stamp-annotation

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

AFFECTED FILES
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/guiutils.cpp
  ui/guiutils.h
  ui/pagepainter.cpp
  ui/pageviewannotator.cpp

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Nathaniel Graham]

ngraham added a comment.


  In D22064#500378 , @simgunz wrote:
  
  > I agree with @ngraham. Showing a warning seems the best solution for now. 
Any suggestions on the best way to display it? Is a string at the bottom of the 
appearence widget enough?
  
  
  How about putting a KMessageWidget at the top using the Warning style?

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz marked an inline comment as done.
simgunz added a comment.


  In D22064#498964 , @aacid wrote:
  
  > Small code improvements that can be ignored if you want.
  
  
  Fixed, and thanks for pointing out my bad style practices. I also like clean 
code.
  
  > The main concern i have is that we're improving a broken feature, stamps 
are not saved correctly into the pdf files (because of poppler) so I'm a bit 
unconvinced of making it easier to select random files since that'll create 
more broken PDF files out there.
  
  I agree with @ngraham. Showing a warning seems the best solution for now. Any 
suggestions on the best way to display it? Is a string at the bottom of the 
appearence widget enough?
  
  In the new annotation toolbar, we could disable the stamp annotation button 
when the loaded file is a pdf, what do you think?

INLINE COMMENTS

> pino wrote in annotationwidgets.cpp:166
> The dialog title must be i18n'ed; while you are there, please use the 
> `@title:window` semantic context with i18nc()

I have also tried to add  and  tags as suggested in the i18n 
semantics tutorial, but they seem not to be processed given that the tags 
appears among the text when the dialog is shown.

`i18nc("@info", "Could not load the file 
%1", customStampFile )`

https://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics#Phrase_tags

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz updated this revision to Diff 62335.
simgunz marked 7 inline comments as done.
simgunz added a comment.


  - Move to init list
  - Add translation context
  - Use new connect syntax
  - Add tooltip to custom stamp button

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22064?vs=60660=62335

BRANCH
  improve-stamp-annotation

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

AFFECTED FILES
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/guiutils.cpp
  ui/guiutils.h
  ui/pagepainter.cpp
  ui/pageviewannotator.cpp

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> annotationwidgets.cpp:53
> +toplay->addWidget( m_comboItems );
> +m_stampPushButton = new QPushButton(QIcon::fromTheme( "document-open" ), 
> "", this );
> +m_stampPushButton->setVisible( false );

maybe add a tooltip to this button, just to hint users what's for

> annotationwidgets.cpp:166
> +if( pixmap.isNull() ) {
> +KMessageBox::sorry( this, i18n( "Could not load the file %1", 
> customStampFile ), "Invalid file" );
> +} else {

The dialog title must be i18n'ed; while you are there, please use the 
`@title:window` semantic context with i18nc()

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: pino, aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, 
joaonetto, kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Albert Astals Cid]

aacid added a comment.


  Small code improvements that can be ignored if you want.
  
  The main concern i have is that we're improving a broken feature, stamps are 
not saved correctly into the pdf files (because of poppler) so I'm a bit 
unconvinced of making it easier to select random files since that'll create 
more broken PDF files out there.
  
  On the other hand this isn't really PDF specific, so i guess it could go in?
  
  What do others think?

INLINE COMMENTS

> annotationwidgets.cpp:45
>  {
> -QHBoxLayout * mainlay = new QHBoxLayout( this );
> +m_previewPosition = position;
> +QVBoxLayout * mainlay = new QVBoxLayout( this );

move this up to the initializer list, i.e.

, m_previewPosition( position )

after QWidget( parent )

> annotationwidgets.cpp:53
> +toplay->addWidget( m_comboItems );
> +m_stampPushButton = new QPushButton(QIcon::fromTheme( "document-open" ), 
> "", this );
> +m_stampPushButton->setVisible( false );

"" -> QString()

> annotationwidgets.cpp:77
>  connect( m_comboItems, ::editTextChanged, this, 
> ::iconComboChanged );
> +connect( m_stampPushButton, SIGNAL(clicked()), this, 
> SLOT(selectCustomStamp()) );
>  }

Any reason not to use the "new" connect syntax based on function pointers?

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: aacid, yurchor, ngraham, okular-devel, maguirre, fbampaloukas, joaonetto, 
kezik, tfella, darcyshen

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz added a subscriber: aacid.
simgunz added a comment.


  @aacid Can you also have a look at this revision when you have the time?

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: aacid, yurchor, ngraham, okular-devel, fbampaloukas, joaonetto, tfella, 
darcyshen

D22064: General improvements to stamp annotation

[Nathaniel Graham]

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


  Looks good to me, but I'd like a review from an #okular 
 person, too.

INLINE COMMENTS

> simgunz wrote in annotationwidgets.cpp:153
> I have copied the code from KIconLoader. I guess that "Icon Files" can be 
> translated. Any suggestion to improve it?
> 
> https://api.kde.org/frameworks/kiconthemes/html/kicondialog_8cpp_source.html#l00605

Ehh, looks like this is actually fine.

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

To: simgunz, #okular, ngraham
Cc: yurchor, ngraham, okular-devel, fbampaloukas, joaonetto, tfella, darcyshen, 
aacid

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz edited the summary of this revision.

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: yurchor, ngraham, okular-devel, fbampaloukas, joaonetto, tfella, darcyshen, 
aacid

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz marked 2 inline comments as done.
simgunz added a comment.


  Added the enum, much better now.
  
  @yurchor Great.

INLINE COMMENTS

> ngraham wrote in annotationwidgets.cpp:153
> Are you sure the list of file formats should be localized?

I have copied the code from KIconLoader. I guess that "Icon Files" can be 
translated. Any suggestion to improve it?

https://api.kde.org/frameworks/kiconthemes/html/kicondialog_8cpp_source.html#l00605

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: yurchor, ngraham, okular-devel, fbampaloukas, joaonetto, tfella, darcyshen, 
aacid

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz updated this revision to Diff 60660.
simgunz added a comment.


  - Define enum for preview position

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22064?vs=60553=60660

BRANCH
  improve-stamp-annotation

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

AFFECTED FILES
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/guiutils.cpp
  ui/guiutils.h
  ui/pagepainter.cpp
  ui/pageviewannotator.cpp

To: simgunz, #okular
Cc: yurchor, ngraham, okular-devel, fbampaloukas, joaonetto, tfella, darcyshen, 
aacid

D22064: General improvements to stamp annotation

[Yuri Chornoivan]

yurchor added a comment.


  In D22064#486358 , @simgunz wrote:
  
  > @yurchor Are you willing to update the documentation regarding this 
modification after we merge this? (I saw you changed the doc regarding the 
refactor of the annotation tools config dialogs, and it seems you have 
experience with the documentation aspect).
  
  
  Sure.

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: yurchor, ngraham, okular-devel, fbampaloukas, joaonetto, tfella, darcyshen, 
aacid

D22064: General improvements to stamp annotation

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> annotationwidgets.cpp:153
> +const QString customStampFile = QFileDialog::getOpenFileName(this, 
> i18n("Select custom stamp symbol"),
> +QString(), i18n("*.ico *.png *.xpm *.svg *.svgz | Icon Files (*.ico 
> *.png *.xpm *.svg *.svgz)") );
> +if ( !customStampFile.isEmpty() )

Are you sure the list of file formats should be localized?

> annotationwidgets.cpp:425
>  
> -m_pixmapSelector = new PixmapPreviewSelector( widget );
> +m_pixmapSelector = new PixmapPreviewSelector( widget, true /* 
> previewBelow */ );
>  formlayout->addRow( i18n( "Stamp symbol:" ), m_pixmapSelector );

If this was an enum, you wouldn't need to add an inline comment explaining what 
it does :)

> annotationwidgets.h:35
>  public:
> -explicit PixmapPreviewSelector( QWidget * parent = nullptr );
> +explicit PixmapPreviewSelector( QWidget * parent = nullptr, bool 
> previewBelow = false );
>  virtual ~PixmapPreviewSelector();

Instead of adding a new bool argument, how about an enum instead for clarity so 
the code is easy to read?

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: yurchor, ngraham, okular-devel, fbampaloukas, joaonetto, tfella, darcyshen, 
aacid

D22064: General improvements to stamp annotation

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: yurchor, ngraham, okular-devel, fbampaloukas, joaonetto, tfella, darcyshen, 
aacid

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz added a subscriber: yurchor.
simgunz added a comment.


  @yurchor Are you willing to update the documentation regarding this 
modification after we merge this? (I saw you changed the doc regarding the 
refactor of the annotation tools config dialogs, and it seems you have 
experience with the documentation aspect).

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: yurchor, ngraham, okular-devel, fbampaloukas, joaonetto, tfella, darcyshen, 
aacid

D22064: General improvements to stamp annotation

[Nathaniel Graham]

ngraham added a comment.


  This is fantastic! I tested it out and it works perfectly. Huge improvement.

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: ngraham, okular-devel, fbampaloukas, joaonetto, tfella, darcyshen, aacid

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz added a comment.


  F6929689: Screenshot_20190624_081508.png 

  Okular embedded stamp
  
  F6929691: Screenshot_20190624_081534.png 

  File chooser filters
  
  F6929694: Screenshot_20190624_081738.png 

  Horizontal image
  
  F6929701: Screenshot_20190624_082102.png 

  Vertical image
  
  F6929707: Screenshot_20190624_082253.png 

  Icon name
  
  F6929697: Screenshot_20190624_081913.png 

  Error message

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: okular-devel, fbampaloukas, joaonetto, tfella, ngraham, darcyshen, aacid

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz added a task: T8074: Make custom stamp image support more discoverable.

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: okular-devel, fbampaloukas, joaonetto, tfella, ngraham, darcyshen, aacid

D22064: General improvements to stamp annotation

[Simone Gaiarin]

simgunz edited the summary of this revision.
simgunz edited the test plan for this revision.

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: okular-devel, fbampaloukas, joaonetto, tfella, ngraham, darcyshen, aacid

D22064: General improvements to stamp annotation

[Simone Gaiarin]

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

REVISION SUMMARY
  Configuration:
  
  - Add push button to select custom stamp image
  - Check if loaded image is usable as stamp or throw error
  - Keep image proportions in previewer
  - Move previewer below the combobox to display larger preview
  
  Annotation tool:
  
  - Keep stamp image proportion in annotation preview (while left mouse button 
is down)
  - Adding the annotation with one-click (without holding the left mouse button 
and dragging) adds the stamp with original proportions
  
  TODO:
  
  [ ] Check if filters in file chooser make sense / propose better alternative
  [ ] Update doc

TEST PLAN
  From stamp annotation configuration dialog:
  
  - Click push button next to combo box opens a file selector
  - Selecting a corrupted image file should throw an error
  - Selecting a good image file shows the preview of the image
  - Select a horizontal image shows a large clear preview
  - Select a vertical image file shows a smaller preview without messing up the 
visual of the config dialog
  
  From page view, select the stamp annotation with horizontal image file (not 
squared):
  
  - Click and hold. The preview maintains proportions
  - Single click. The stamp image in the pdf maintains proportions and has the 
same size of the click and hold preview.
  - Add an annotation of the Okular custom stamps (internal SVG so treated 
slightly differently) do not create problems

REPOSITORY
  R223 Okular

BRANCH
  improve-stamp-annotation

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

AFFECTED FILES
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/guiutils.cpp
  ui/guiutils.h
  ui/pagepainter.cpp
  ui/pageviewannotator.cpp

To: simgunz, #okular
Cc: okular-devel, fbampaloukas, joaonetto, tfella, ngraham, darcyshen, aacid