[Okular-devel] [PATCHES] Customizable annotation tools

Mon May 20 15:07:14 UTC 2013

El Diumenge, 19 de maig de 2013, a les 01:03:49, Fabio D'Urso va escriure:
> Hi,
> 
> As I said in IRC, I'd like to include customizable annotation tools in KDE
> 4.11, so I'm resurrecting this old thread.
> 
> Since the original post, I've created a configurable-review-tools branch and
> pushed almost(*) all patches there.
> They have slightly been modified since the OP, but the overall structure is
> the same:
>  - Tools can be configured from the configuration panel
>  - Tools are still described through XML data
>  - Configuration is saved in XML format in okularpartrc via kcfg
> 
> I've merged master again, therefore it's possible to see a big dump of all
> changes with:
>   git diff origin/master origin/configurable-review-tools
> 
> A (imho) better way to see changes is:
>   gitk origin/configurable-review-tools ^origin/master --no-merges
> which gives some sort of changelog.
> 
> In the first patches, you can safely skip analysis of code that directly
> creates XML strings, as it is replaced by safer QDom methods in
> 3dcaa585587984a0d34daa41915dfe4d4d995d75.
> 
> If you prefer a single big review request instead, please let me know.

I'd appreciate if you reviewboard it, it's easier to leave comments in there.

Anyhow a first batch of comments, you have a few

slotXYZ (bool )

where then you don't use the bool, just remove it if you don't need it

You have slotRowChanged that just calls updateButtons, why not change the 
connection to call updateButtons directly?

I think i miss an "Edit" button in the settings dialog, double clicking is ok 
but not really discoverable unless you know you can do it.

Overall it looks good :-)

Cheers,
  Albert

> 
> * = What's still missing is support for a new "polyline" tool, which I had
> initially posted, but I've noticed it needs some corrections (my first
> implementation ended the line on double-click, but kolourpaint uses right-
> click, and I'd like to mirror it for consistency).
> 
> Thanks,
> Fabio
> 
> On Wednesday, August 08, 2012 12:19:03 AM Albert Astals Cid wrote:
> > El Dimecres, 1 d'agost de 2012, a les 03:04:31, Fabio D'Urso va escriure:
> > > On Wednesday, August 01, 2012 12:32:06 AM Albert Astals Cid wrote:
> > > > Can we see screenshots of the other GUI changes you made for lazy
> > > > people?
> > > 
> > > Annotation widget changes (in all images old is on the left):
> > > - Do not show the font selection field in pop-up annotations (cmp-popup)
> > > 
> > >    I guess it was a forgotten if, because the same widget is used for
> > >    inplace text annotations, where configuring the font makes sense.
> > > 
> > > - Configurable line width in InkAnnotations (cmp-ink)
> > > - Configurable inner color in polygons (cmp-polygon)
> > > 
> > >    Note that even tough straight lines and polylines use the same
> > >    widget,
> > >    the inner color field is not shown in those annotations. In other
> > >    words,
> > >    their properties dialog were not changed.
> > > 
> > > - Text alignment in inplace text annotations (cmp-inplace)
> > > 
> > >    This one can be done better (eg with icons) in future patches. I had
> > >    had
> > > 
> > > a quick look at koffice source and I remember that there were different
> > > cases for LTR and RTL languages, that's why I went the easy path.
> > > 
> > > The Identity configuration page is replaced by an Annotations page,
> > > which
> > > contains the configurable tool list. You can see it in config-panel-new.
> > > Note that the privacy notice now appears as tooltip for text box.
> > > 
> > > In config-panel-new, you can also see the dialog to create and edit
> > > tools.
> > > As I said in the previous post, it uses the same widgets used by the
> > > annotation properties dialog.
> > > The caption is "Edit annotation tool" if the user is editing an existing
> > > tool (like in the picture), or "Create annotation tool" if the user is
> > > creating a new tool.
> > > If the user hasn't set a customized name, the Name text box shows the
> > > default tool name in gray (placeholder text).
> > > The combo lets you choose the tool type. Changing its value also changes
> > > the type of properties that can be configured. Available choices are:
> > > Pop-up Note, Inline Note, Freehand Line, Straight Line, Polygon,
> > > Polyline, Text markup, Geometrical shape, Stamp.
> > > 
> > > In this dialog, I used the term "Freehand Line" instead of "Ink", and
> > > "Straight Line", "Polygon" and "Polyline" instead of the generic "Line"
> > > which is used in the annotation properties dialog, because they sound
> > > clearer to me. If you agree, I'd make another patch to make the
> > > annotation
> > > properties dialog show these names too.
> > > 
> > > > It might be good adding some contents to
> > > > PageViewAnnotator::defaultToolName
> > > > since e.g. "Underline" can be read as a verb or as a noun.
> > > 
> > > Yes, I see the issue. Currently we have Highlighter, but also Polygon,
> > > Stamp, Underlining and Ellipse, that are all nouns. I'll review that
> > > part.
> > > 
> > > > What happens when there are lots of items in the toolbar, do you get a
> > > > double line of icons?
> > > 
> > > Yes. I didn't change it and the existing code can do that
> > > (toolbar-wrap).
> > > 
> > > P.S.: I'd like to change the Polygon icon too, because it's not a
> > > polygon
> > > right now.
> > 
> > Works for me (as in "I agree to everything you say")
> > 
> > Cheers,
> > 
> >   Albert
> >   
> > > Fabio
> 
> _______________________________________________
> Okular-devel mailing list
> Okular-devel at kde.org
> https://mail.kde.org/mailman/listinfo/okular-devel