General improvements to stamp annotation
ClosedPublic

Authored by simgunz on Jun 24 2019, 6:05 AM.

Details

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

BUG: 370381
BUG: 383652
CCBUG: 383651
FIXED-IN: 1.9.0

Closes T8074

TODO:

  • Check if filters in file chooser make sense / propose better alternative
  • Update doc ( @yurchor will do it after we merge this)
Test Plan

From stamp annotation configuration dialog:

  • Show a warning regarding limitations of the feature's current implementation
  • 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
  • Input a valid icon name in the combobox and the preview of the icon is shown

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

Diff Detail

Repository
R223 Okular
Branch
improve-stamp-annotation
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14385
Build 14403: arc lint + arc unit
simgunz created this revision.Jun 24 2019, 6:05 AM
Restricted Application added a project: Okular. · View Herald TranscriptJun 24 2019, 6:05 AM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
simgunz requested review of this revision.Jun 24 2019, 6:05 AM
simgunz edited the summary of this revision. (Show Details)Jun 24 2019, 6:09 AM
simgunz edited the test plan for this revision. (Show Details)


Okular embedded stamp


File chooser filters


Horizontal image


Vertical image


Icon name


Error message

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

@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).

ngraham edited the summary of this revision. (Show Details)Jun 25 2019, 7:36 AM
ngraham added inline comments.Jun 25 2019, 7:42 AM
ui/annotationwidgets.cpp
162

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

445

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

ui/annotationwidgets.h
35–37

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

@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.

simgunz updated this revision to Diff 60660.Jun 25 2019, 6:05 PM
  • Define enum for preview position
simgunz marked 2 inline comments as done.Jun 25 2019, 6:08 PM

Added the enum, much better now.

@yurchor Great.

ui/annotationwidgets.cpp
162

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

simgunz edited the summary of this revision. (Show Details)Jun 25 2019, 6:09 PM
ngraham accepted this revision.Jun 25 2019, 6:53 PM

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

ui/annotationwidgets.cpp
162

Ehh, looks like this is actually fine.

This revision is now accepted and ready to land.Jun 25 2019, 6:53 PM
simgunz added a subscriber: aacid.Jun 28 2019, 6:08 AM

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

aacid added a comment.Jul 21 2019, 8:48 AM

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?

ui/annotationwidgets.cpp
46

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

, m_previewPosition( position )

after QWidget( parent )

53

"" -> QString()

78

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

pino added a subscriber: pino.Jul 21 2019, 8:54 AM
pino added inline comments.
ui/annotationwidgets.cpp
53

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

167

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

If the feature is considered fundamentally broken, then just hiding it behind a bad UI isn't really enough because people will discover it anyway (I did). It either should be totally removed so we don't torture users until Poppler can handle it properly, or it should be branded with a big banner that says "warning, this is an experimental feature. It may not work in X, Y, or Z circumstances." Personally I would prefer the latter.

simgunz updated this revision to Diff 62335.Jul 22 2019, 5:46 PM
simgunz marked 7 inline comments as done.
  • Move to init list
  • Add translation context
  • Use new connect syntax
  • Add tooltip to custom stamp button
simgunz marked an inline comment as done.Jul 22 2019, 6:02 PM

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?

ui/annotationwidgets.cpp
167

I have also tried to add <filename> and <warning> 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", "<warning>Could not load the file <filename>%1</filename></warning>", customStampFile )

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

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?

simgunz updated this revision to Diff 62454.Jul 24 2019, 6:55 AM
  • Add warning regarding broken stamp support in PDF

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

ngraham edited the summary of this revision. (Show Details)Jul 24 2019, 3:35 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham accepted this revision.Jul 24 2019, 3:49 PM

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! 👍

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

Is it ok like this?

The longer sentence would wrap given the dialog default size

Should 'Experimental' after the column be capitalzed or lowercase?

In the k18n guide (<warning> 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

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

Is it ok like this?

The longer sentence would wrap given the dialog default size

Should 'Experimental' after the column be capitalzed or lowercase?

In the k18n guide (<warning> 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.

simgunz updated this revision to Diff 62500.Jul 24 2019, 7:52 PM
  • Fix messages and i18nc

Final version of the new UI components:

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

ngraham accepted this revision.Jul 24 2019, 8:43 PM

+1, I say shipit!

ngraham edited the summary of this revision. (Show Details)Jul 24 2019, 8:50 PM
simgunz closed this revision.Jul 25 2019, 6:09 PM