Improve layout of annotation configuration dialogs
ClosedPublic

Authored by simgunz on Feb 26 2018, 12:38 PM.

Details

Summary

The config dialog of each annotation tool is now a form layout without group boxes. Everything is aligned.

Diff Detail

Repository
R223 Okular
Branch
fix-annot-config-dialog
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12654
Build 12672: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

Would be easier to test this change if it is rebased against current master, as many changes landed in annotationwidgets.{h,cpp} recently.

Restricted Application added a subscriber: okular-devel. · View Herald TranscriptMay 25 2019, 9:24 AM

This diff was on hold because of the work I am doing on the new annotation toolbar, that may make all the annotation configuration dialogs obsolete. But I now think those dialogs may still be useful, so that we can move this review forward. I am going to rebase it and possibly fix the layout of the newly added fields in a few days.

aacid added a subscriber: aacid.May 26 2019, 6:03 PM
aacid added inline comments.
ui/annotationwidgets.cpp
829–830

return at the end of a function screams really bad at me, please fix :)

simgunz updated this revision to Diff 58732.May 27 2019, 4:28 PM

Rebase to master, fix layout of FileAttachment and Caret

  • Pass gridLayout pointer to createStyleWidget
  • Fix design of text annotation tools
  • Fix layout of freehand tool
  • Fix layout of highlighter
  • Fix layout of geometrical shape
  • Fix layout of straight line and polygon
  • Position spacer at the bottom of the grid layout
  • Simplify code
  • Fix layout of stamp
  • Fix line endings
  • Fix font capitalization
  • Remove unuseful return
  • Add icon label
  • Fix indentation
  • Fix layout of caret annotation
  • Remove unused code
  • Fix layout of file attachment
  • Rename layout > gridLayout for clarity
  • Make row variable constant
  • Set buddy
  • Mark variable unused
  • Fix layout of extraWidget of FileAttachment
  • Fix layout of General Info

Here a visual list of all the modifications:

^^ This is common to all the annotation

"Shape fill" maybe can be renamed "Fill color".

Big improvement. Code looks sane, but I think we can go even further! I don't see the need for a top level grid layout here; we can use a FormLayout for each annotation page's own layout. For any layout where there's a thing on the right that consists of more than one item, you can put them in a QHBoxLayout, and then put that in the FormLayout.

So in createAppearanceWidget() we create a QFormLayout (instead that a QGridLayout) and then this is passed down to all the createStyleWidget methods, is that ok?

I think it needs to be passed down, instead of having independents QFormLayouts for each annotation in order to have the Color and Opacity elements correctly aligned with the elements below ( which I think won't happen if we have two separate QFormLayouts on the top and on the bottom).

Yep, that makes sense. I know in Kirigami we have a way to align two FormLayouts but I couldn't find a way for the QWidgets version. So I guess you'll have to pass around the same one.

simgunz updated this revision to Diff 58817.May 29 2019, 7:45 AM
  • Use form layout instead of grid layout
  • Set label alignment and field growth policy
  • Resort code of line annotation
  • Fix wrong widget set as parent
  • Move line termination styles before leaders style
  • Align combobox to top

Using the QFormLayout simplified the code a quite a bit!

Regarding the stamp annotation tool, I have aligned the combobox to the top, because I have not found a way to vertically align the label. Though I will soon open a new review with some changes for that annotation tool in particular, where I plan to move the preview beneath the combobox. (or maybe I can do it directly in this review?)

ui/annotationwidgets.cpp
213

Is it correct to set this growth policy? I have not found any guideline on the HIG. Compared to the grid layout where label and widget take the same amount of horizontal space, now the widgets take way more space making it a little ugly for some annotation tools. Any idea?

Tested, LGTM.

Thanks! Much simpler indeed.

ui/annotationwidgets.cpp
213

For this one, FieldsStayAtSizeHint might be better, and then we just set sensible sizehints for anything that winds up too small. That's basically what we're doing for all our QML software. In general we're trying to move towards making the controls optimally sized in the first place and then static in size when the window grows or shrinks. We often manually set the width of spinboxes, comboboxes, and text fields to make sure they're all the same width.

aacid added inline comments.May 29 2019, 6:07 PM
ui/annotationwidgets.cpp
213

Let's not have static sizes, this is not a phone screen where you can't resize windows.

Just tried this. Maybe you can bring back the section headings, which were group box labels before? Straight Line got many options recently.

Not working buddys (probably a problem with PixmapPreviewSelector):
Popup-Note -> Icon (Alt + i)
Stamp -> Stamp symbol (Alt + s)
(Another not working accelerator: Configure Okular -> Annotations: Author and Add both have Alt + a.)

Regarding the stamp annotation tool, I have aligned the combobox to the top, because I have not found a way to vertically align the label.

Looks ok to me.

ui/annotationwidgets.cpp
389

Capitalize symbol?

394

Why is this editable? Entering a path to an image does not work.

I don't think the section headings really added much. Most of them were group boxes with one item, which is pointless. When using a FormLayout style, in general you want to make the labels self-explanatory as much as possible so you don't need section headers. To separate groups of related settings, you can add some whitespace that has the effect of grouping things together in a very natural-looking way. We do this extensively in other FormLayouts and people generally think it looks good:


I don't think the section headings really added much. Most of them were group boxes with one item, which is pointless. When using a FormLayout style, in general you want to make the labels self-explanatory as much as possible so you don't need section headers. To separate groups of related settings, you can add some whitespace that has the effect of grouping things together in a very natural-looking way. We do this extensively in other FormLayouts and people generally think it looks good:

I’m not sure whether to agree.

Your screenshots show vertical whitespace, but between differently shaped widgets. That makes the space appear a bit random. Then I looked in System Settings, which uses mostly group boxes, and sometimes space, section labels, and horizontal lines.

But in this dialog, whitespace will probably look good, because all widgets have the same shape.

simgunz updated this revision to Diff 58928.May 30 2019, 8:08 PM
  • Fix label buddy not working of PixmapPreviewSelector
  • Fix keyboard shortcut of Author conflicts with Add (now selected automatically)
  • Same order of Size and Fill color for GeomShape and Polygon
  • Make annotation type the first element before color

I am also in favor of the white space. Lines or group boxes seem to me too heavy for this type of configuration (and it is what I wanted to remove in the first place). Any idea on how to add the vertical white space in a QFormLayout?

Shape fill and Size are now in the same order for GeomShape and Polygon.

For Highlighter and Geometrical shape I moved the annotation type to the top, before the color/size settings. What do you think?

I am also in favor of the white space. Lines or group boxes seem to me too heavy for this type of configuration (and it is what I wanted to remove in the first place). Any idea on how to add the vertical white space in a QFormLayout?

Check out how Dolphin does it in its settings window.

For Highlighter and Geometrical shape I moved the annotation type to the top, before the color/size settings. What do you think?

Makes sense, most important setting first. :) Maybe this makes squiggle and strikeout a bit more discoverable.

Straight line has a property called “Size”, which sounds a bit too universal to me. How about Width or Thickness?

Not only straight line, but also freehand line, geometrical shape and polygon (with the same meaning). I agree with you, but on the other hand in drawing software like Krita or Gimp it is called Size so maybe the users are used to it. I have not a strong preference on this matter.

simgunz updated this revision to Diff 58945.May 31 2019, 12:20 PM
  • Add color button and opacity spin manually in each annotation
  • Add vertical space to separate groups of settings
simgunz updated this revision to Diff 58946.May 31 2019, 12:24 PM
  • Add opacity spin to file attachment annotation
ngraham retitled this revision from Improve layout of annotation configuraton dialogs to Improve layout of annotation configuration dialogs.May 31 2019, 12:50 PM
ngraham accepted this revision.May 31 2019, 1:10 PM

Looks and behaves well to me from a UI perspective! Very nice work.

conf/dlgannotationsbase.ui
55

Is this change intentional? If so, can we just move the ampersand around so that it still has an accelerator?

This revision is now accepted and ready to land.May 31 2019, 1:10 PM

Not only straight line, but also freehand line, geometrical shape and polygon (with the same meaning). I agree with you, but on the other hand in drawing software like Krita or Gimp it is called Size so maybe the users are used to it. I have not a strong preference on this matter.

Do you mean “Brush Sizes”? Krita and Gimp don’t work with lines AFAIK. Inkscape uses Width for line thickness.

conf/dlgannotationsbase.ui
71

I think this could be:
Note: The information here is used only for annotations.
Information inserted here will be saved in your documents, but will not be transmitted automatically.

Reason: When annotating a document without having the side panel open, then saving and distributing the document, the user might not know that this information is transmitted, because her/his name was never displayed.

ui/annotationwidgets.cpp
394

Read in the user documentatio that it theoretically works.

simgunz marked 3 inline comments as done.May 31 2019, 3:32 PM

Yes I meant the brushes (that behave as the freehand annotation). For me setting it to Width is fine.

conf/dlgannotationsbase.ui
55

Yes.

Both Author and Add had 'a' as accelerator. This was actually working. Pressing 'a' twice switch between the two.

Nonetheless I changed it to make the accelerators unique.

Initially I set 'u' as accelerator for Author but this was conflicting with 'Move Up' and the accelerator of author was automatically changed to 't' once running Okular. (not sure why this did not happen also for 'a').

Then I noticed that not setting the accelerator let Qt set one automatically, so I decided to not set it. Mainly because I though that it make sense to set it only if it has a logical sense like 'u' for Move Up or 'd' for Move Down, but given that 't' is a random letter of author I decided to let it be automatic.

Maybe it is better to set one explicitly?

ui/annotationwidgets.cpp
389

KDE HIG prescribe sentence style capitalization for labels in a form layout: https://hig.kde.org/style/writing/capitalization.html

394

Sometimes works. :-) Try with a path to an icon. In any case I'll improve the Stamp tool soon, adding a 'select file' button. I have some code ready that I'll share soon.

ngraham added inline comments.May 31 2019, 4:04 PM
ui/annotationwidgets.cpp
394

Once you do that, it will fix https://bugs.kde.org/show_bug.cgi?id=383652 :)

simgunz marked 7 inline comments as done.May 31 2019, 5:53 PM
simgunz added inline comments.
conf/dlgannotationsbase.ui
71

I have not understood the reason. This message is only displayed in the configuration dialog. In any case we could change it for me.

simgunz updated this revision to Diff 58965.May 31 2019, 5:54 PM
  • Rename "Size" to "Width"
  • Set better accelerators

What should we do regarding the message at line 71 of conf/dlgannotationsbase.ui ? I think is the last thing to possibly change.

After that I think this review is ready. Once I get the ok from @aacid I can land this.

(The code for the stamp annotation is ready, I will push it once we land this one).

davidhurka added inline comments.Jun 1 2019, 6:00 PM
conf/dlgannotationsbase.ui
71

What should we do regarding the message at line 71 of conf/dlgannotationsbase.ui ? I think is the last thing to possibly change.

I suggested two sentences to change.

  1. Replace “comments and reviews” by “annotations”. I think that is more clear. The tools are called Annotation, and comments and reviews are only possible use cases for them, along notes and the Typewriter hack for defective forms.
  2. Don’t say that the information is not transmitted without knowledge, but provide this knowledge: The information is saved in annotated documents, and so will be transmitted together with the document.

Currently, the message is indicating in two ways, that the information is not transmitted when the user fills forms using Typewriter and sends the document to someone else.

Is this now explained better?

Replace “comments and reviews” by “annotations”.

+1

simgunz updated this revision to Diff 59000.Jun 2 2019, 7:16 AM
  • Change string that inform the user about how Identity info are used
aacid added inline comments.Jun 2 2019, 9:39 PM
conf/dlgannotationsbase.ui
55

It's not Qt. It's kxmlgui that does the autoaccelerator magic. In principle it's always better to set it manually, this way translators know there's an accelrator and can try to set one that makes sense themselves and not leave it to "randomness" of the autoaccelerator magic.

ui/annotationwidgets.cpp
465

you're going to have conflicts, AFAICS this "only for PDF documents" is gone.

simgunz updated this revision to Diff 59532.Jun 10 2019, 4:51 PM
simgunz marked 3 inline comments as done.

Fix author accelerator, remove 'for pdf only' tooltips

shubham added inline comments.
ui/annotationwidgets.cpp
818

Why not qobject_cast?

simgunz updated this revision to Diff 59533.Jun 10 2019, 5:00 PM
simgunz marked 3 inline comments as done.
  • Properly cast QObject
simgunz marked an inline comment as done.Jun 10 2019, 5:02 PM
simgunz updated this revision to Diff 59534.Jun 10 2019, 5:07 PM
  • Fix code formatting

Is this waiting on something or should it be commited?

Just waiting for your OK.

aacid added a comment.Jun 22 2019, 1:04 PM

had a quick look at the code and don't see anything obviosuly wrong, i'd say let's go for it

Side note: i usually don't go over things that have been approved by someone else, so if you feel you really need my review when something has been approved please ping me directly :)

aacid accepted this revision.Jun 22 2019, 1:04 PM

Perfect. I can land this tomorrow evening.

simgunz edited the summary of this revision. (Show Details)Jun 23 2019, 6:35 PM

Tried to land this, but I got this error message:

remote: FATAL: W refs/heads/master okular gaiarin DENIED by refs/.*
remote: error: hook declined to update refs/heads/master
To git.kde.org:okular
 ! [remote rejected]     6ee24b04e911da577f23a139c19e8862aaab10f3 -> master (hook declined)
error: failed to push some refs to 'git@git.kde.org:okular'
Usage Exception: Push failed! Fix the error and run "arc land" again.

My .git/config

[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
[remote "origin"]
        url = kde:okular
        fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
        remote = origin
        merge = refs/heads/master
        rebase = true
[branch]
        autosetuprebase = always
[url "git://anongit.kde.org/"]
    insteadOf = kde:
[url "git@git.kde.org:"]
    pushInsteadOf = kde:

Are there restrictions to push to the Okular repo?

sander added a subscriber: sander.Jun 23 2019, 7:25 PM

I had the same problem yesterday and got around it by moving to https://invent.kde.org/kde/okular .

shubham removed a subscriber: shubham.Jun 24 2019, 4:35 AM
This revision was automatically updated to reflect the committed changes.

@sander Thanks. It worked.