Bug 70982

Created attachment 88729 [details] [review]
glib-demo: Simplify annotations list and its properties

Updated patch to match with changes in https://bugs.freedesktop.org/attachment.cgi?id=88161 (https://bugs.freedesktop.org/show_bug.cgi?id=70901)

Created attachment 88801 [details] [review]
glib-demo: Simplify annotations list and its properties

Created attachment 89306 [details] [review]
glib-demo: Simplify annotations list and its properties

Updated patch

Created attachment 89392 [details] [review]
glib-demo: Simplify annotations list and its properties

Updated patch. Rebased to make it fit against patches in
https://bugs.freedesktop.org/show_bug.cgi?id=70981

Comment on attachment 89392 [details] [review]
glib-demo: Simplify annotations list and its properties

Review of attachment 89392 [details] [review]:
-----------------------------------------------------------------

I'm not sure about this, I find it a bit confusing. This is what I see with my spanish locale:

Coords: (152,0,556,0) (152,0,556,0)

Maybe we could use something like:

Coords: X1: aa,bb X2: aa,bb Y1: aa,bb Y2: aa,bb

Which is what we currently have in the list but moved to a label in the properties.
The coords information is mainly useful when debugging, but now that we have the view also in the annots demo, it would be really useful to paint a focus ring around the currently selected annot. But that could be done as a follow up patch.

(In reply to comment #5)
> Comment on attachment 89392 [details] [review] [review]
> glib-demo: Simplify annotations list and its properties
> 
> Review of attachment 89392 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure about this, I find it a bit confusing. This is what I see with
> my spanish locale:
> 
> Coords: (152,0,556,0) (152,0,556,0)
> 
> Maybe we could use something like:
> 
> Coords: X1: aa,bb X2: aa,bb Y1: aa,bb Y2: aa,bb
> 
> Which is what we currently have in the list but moved to a label in the
> properties.

I was trying to save space, so no need to resize the sidebar pane and leaving space to the rendering page.  I had forgotten about the locale thing, though.

Maybe we can use semicolon instead a comma, something like:
     (152,0;556,0) (152,0;556,0)
or a dash:
     (152,0-556,0) (152,0-556,0)

FWIW, I was also inclined to removed the decimals, but the PDF has decimals (for the annotations added with other software). The ones created with cairo, are integers.

> The coords information is mainly useful when debugging, but now that we have
> the view also in the annots demo, it would be really useful to paint a focus
> ring around the currently selected annot. But that could be done as a follow
> up patch.

Indeed, the reason to save space and avoid the scrollbars in the side pane was precisely because it was becoming a pain to debug the coordinates when I was testing it :-)

The focus ring would be good. IIRC, I had it done in an Evince patch I have.

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 89392 [details] [review] [review] [review]
> > glib-demo: Simplify annotations list and its properties
> > 
> > Review of attachment 89392 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm not sure about this, I find it a bit confusing. This is what I see with
> > my spanish locale:
> > 
> > Coords: (152,0,556,0) (152,0,556,0)
> > 
> > Maybe we could use something like:
> > 
> > Coords: X1: aa,bb X2: aa,bb Y1: aa,bb Y2: aa,bb
> > 
> > Which is what we currently have in the list but moved to a label in the
> > properties.
> 
> I was trying to save space, so no need to resize the sidebar pane and
> leaving space to the rendering page.  I had forgotten about the locale
> thing, though.
> 
> Maybe we can use semicolon instead a comma, something like:
>      (152,0;556,0) (152,0;556,0)
> or a dash:
>      (152,0-556,0) (152,0-556,0)
> 
> FWIW, I was also inclined to removed the decimals, but the PDF has decimals
> (for the annotations added with other software). The ones created with
> cairo, are integers.

Yes, I prefer to keep two decimals, fwiw

> > The coords information is mainly useful when debugging, but now that we have
> > the view also in the annots demo, it would be really useful to paint a focus
> > ring around the currently selected annot. But that could be done as a follow
> > up patch.
> 
> Indeed, the reason to save space and avoid the scrollbars in the side pane
> was precisely because it was becoming a pain to debug the coordinates when I
> was testing it :-)
> 
> The focus ring would be good. IIRC, I had it done in an Evince patch I have.

Current evince already does that.

Created attachment 89498 [details] [review]
glib-demo: Simplify annotations list and its properties

Updated patch.

This patch also makes a fixed room for annotation properties.  When the annotation list is long, the properties jump ups and down.

I changed to 2 decimals and semicolon.

Something I forgot to mention: the format (x,y) is also more convenient for quadrilaterals (which can be a lot), so it helps to get a quicker idea of their coordinates.  However, those are not seen up to this patch yet.

What do you think?

Comment on attachment 89498 [details] [review]
glib-demo: Simplify annotations list and its properties

Review of attachment 89498 [details] [review]:
-----------------------------------------------------------------

Ok, pushed, thanks!