The config dialog of each annotation tool is now a form layout without group boxes. Everything is aligned.
Details
- Reviewers
ngraham aacid - Group Reviewers
Okular VDG - Maniphest Tasks
- T8076: Fix design of annotation toolbar
- Commits
- R223:a33cb321df12: Improve layout of annotation configuration dialogs
Diff Detail
- Repository
- R223 Okular
- Branch
- fix-annot-config-dialog
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 12241 Build 12259: arc lint + arc unit
Would be easier to test this change if it is rebased against current master, as many changes landed in annotationwidgets.{h,cpp} recently.
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.
ui/annotationwidgets.cpp | ||
---|---|---|
762–763 | return at the end of a function screams really bad at me, please fix :) |
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
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.
- 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 | ||
---|---|---|
206 | 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? |
Thanks! Much simpler indeed.
ui/annotationwidgets.cpp | ||
---|---|---|
206 | 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. |
ui/annotationwidgets.cpp | ||
---|---|---|
206 | 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.)
Looks ok to me.
ui/annotationwidgets.cpp | ||
---|---|---|
378 | Capitalize symbol? | |
379 | 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’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.
- 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?
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.
- Add color button and opacity spin manually in each annotation
- Add vertical space to separate groups of settings
Looks and behaves well to me from a UI perspective! Very nice work.
conf/dlgannotationsbase.ui | ||
---|---|---|
55 ↗ | (On Diff #58946) | Is this change intentional? If so, can we just move the ampersand around so that it still has an accelerator? |
Do you mean “Brush Sizes”? Krita and Gimp don’t work with lines AFAIK. Inkscape uses Width for line thickness.
conf/dlgannotationsbase.ui | ||
---|---|---|
71 ↗ | (On Diff #58946) | I think this could be: 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 | ||
379 | Read in the user documentatio that it theoretically works. |
Yes I meant the brushes (that behave as the freehand annotation). For me setting it to Width is fine.
conf/dlgannotationsbase.ui | ||
---|---|---|
55 ↗ | (On Diff #58946) | 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 | ||
378 | KDE HIG prescribe sentence style capitalization for labels in a form layout: https://hig.kde.org/style/writing/capitalization.html | |
379 | 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. |
ui/annotationwidgets.cpp | ||
---|---|---|
379 | Once you do that, it will fix https://bugs.kde.org/show_bug.cgi?id=383652 :) |
conf/dlgannotationsbase.ui | ||
---|---|---|
71 ↗ | (On Diff #58946) | I have not understood the reason. This message is only displayed in the configuration dialog. In any case we could change it for me. |
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).
conf/dlgannotationsbase.ui | ||
---|---|---|
71 ↗ | (On Diff #58946) | I suggested two sentences to change.
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? |
conf/dlgannotationsbase.ui | ||
---|---|---|
55 ↗ | (On Diff #58946) | 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 | ||
534 ↗ | (On Diff #58732) | you're going to have conflicts, AFAICS this "only for PDF documents" is gone. |
ui/annotationwidgets.cpp | ||
---|---|---|
838 ↗ | (On Diff #58732) | Why not qobject_cast? |
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 :)
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?
I had the same problem yesterday and got around it by moving to https://invent.kde.org/kde/okular .