Created attachment 367087 [details] [review]
two-line-renderer: Center title

When using GdMainListView widget and there is only one line (no
subtitle), title is aligned vertically to the top of the widget instead
of being aligned to the center of it.

That makes applications like Bijiben, which uses only one line, to look
ugly.

This patch fix that, checking when GdTwoLineRenderer is rendering
GdMainListView widget only, so other widgets like GdMainIconView, are
not affected.

(In reply to Isaque Galdino from comment #0)
> Bijiben uses GdMainview and in list-view, the title is aligned at the top of
> line, instead of being aligned in the middle, vertically, as it used to be
> in the past (I'm not sure how long it was that way).

So, the last part of your sentence has been bothering me. I don't think that it ever worked in the past. :) Or, if it worked then it was not GdTwoLinesRenderer, but something else, that was responsible for making it work. For example, see the Documents screenshot (attachment 268394 [details]) from 2014 in bug 723843.

(In reply to Isaque Galdino from comment #0)
> I'm not sure if this is related to [0]
>
> [...]
> 
> [0] https://bugzilla.gnome.org/show_bug.cgi?id=754737

For the record, the absence of the "list" style class is probably not the issue. I tried adding it with the Inspector and it didn't matter.

(In reply to Debarshi Ray from comment #2)
> (In reply to Isaque Galdino from comment #0)
> > Bijiben uses GdMainview and in list-view, the title is aligned at the top of
> > line, instead of being aligned in the middle, vertically, as it used to be
> > in the past (I'm not sure how long it was that way).
> 
> So, the last part of your sentence has been bothering me. I don't think that
> it ever worked in the past. :) Or, if it worked then it was not
> GdTwoLinesRenderer, but something else, that was responsible for making it
> work. For example, see the Documents screenshot (attachment 268394 [details]
> [details]) from 2014 in bug 723843.

As I said, Bijiben uses GdMainView, not GdTwoLinesRenderer directly. I don't have the history of when or why this changed. Per this bug history: https://bugzilla.gnome.org/show_bug.cgi?id=762127 Pierre-Yves Luyten (former Bijiben maintainer) though there was a change in libgd.

Even so, I believe the presented patch makes sense for applications that doesn't use a subtitle, like Bijiben.

I appreciate if that's applied for our next release next week.

Thanks.

(In reply to Isaque Galdino from comment #4)
> (In reply to Debarshi Ray from comment #2)
> > I don't think that
> > it ever worked in the past. :) Or, if it worked then it was not
> > GdTwoLinesRenderer, but something else, that was responsible for making it
> > work. For example, see the Documents screenshot (attachment 268394 [details]
> > [details]) from 2014 in bug 723843.
> 
> As I said, Bijiben uses GdMainView, not GdTwoLinesRenderer directly. I don't
> have the history of when or why this changed. Per this bug history:
> https://bugzilla.gnome.org/show_bug.cgi?id=762127 Pierre-Yves Luyten (former
> Bijiben maintainer) though there was a change in libgd.

If it ever worked in the past, then the current breakage would be a regression, which puts things in a slightly different light. eg., did somebody want to fix something else or add a new feature that broke this. But after digging through the libgd code and Git history, and from my memory of the Documents bug tracker, I don't think it ever worked in the past.

Anyway, this needs to be fixed.

Review of attachment 367087 [details] [review]:

I haven't finished cross-checking everything, but it appears to me that the co-ordinate calculation might be off in a number of places.

gd_two_lines_renderer_get_size seems to take a lot of trouble to generate the right offset values that take into consideration whether there's a second line or not, the paddings and the alignments. Even then we don't always use those values. Not sure why.

First, why aren't we adding y_offset to aligned_area->y in gd_two_lines_renderer_get_aligned_area?

In gd_two_lines_renderer_render, we are again recalculating values that seems like the same ones that we should have gotten from gd_two_lines_renderer_get_size. Why is that?

(Also, one might think that we could even just chain up to the parent GtkCellRendererText for cases where there is no subtitle. However, that's prevented by the lack of the text-lines property in the parent.)

::: libgd/gd-two-lines-renderer.c
@@ +307,3 @@
   render_area = area;
   render_area.x += x_offset_1 - layout_rect.x;
+  if (GD_IS_MAIN_LIST_VIEW (widget) && layout_two == NULL)

Checking the parent widget's type in the cell renderer is a bit hacky. :) Is it really necessary?

Do we really need the "layout_two == NULL" check? gd_two_lines_renderer_get_size takes care to return a y_offset that matches the presence of the line-two text.

I think that answering the above questions will lead us to the right solution.

I am playing with some fixes in libgd:wip/rishi/two-lines

Created attachment 367765 [details] [review]
two-lines-renderer: Simplify code

Created attachment 367766 [details] [review]
two-lines-renderer: Don't ignore the aligned area's y-offset

Created attachment 367767 [details] [review]
two-lines-renderer: Don't ignore the y-offset when rendering

I took the liberty to fix up your patch.

Dropping the duplicate bug from the blocker list.

This fixes both Bijiben and Documents for me.

Review of attachment 367765 [details] [review]:

LGTM

Created attachment 367770 [details] [review]
main-icon-view: Explicitly specify the vertical alignment of the text

Review of attachment 367766 [details] [review]:

Can't remember this code off hand, but I trust that you tested this.

Review of attachment 367767 [details] [review]:

++

Review of attachment 367770 [details] [review]:

OK

(In reply to Cosimo Cecchi from comment #15)
> Review of attachment 367766 [details] [review] [review]:
> 
> Can't remember this code off hand, but I trust that you tested this.

As far as I can tell, get_aligned_area implementation isn't related to this bug. However, this patch doesn't cause any obvious breakage and seems to match the overall logic and what GtkCellRendererText does.

By the way, while reading gtkcellrenderertext.c I noticed that gtk+'s get_layout is subtly different from libgd's create_layout_with_attrs with respect to setting the PangoLayout's width when the "wrap-width" is and isn't set. I haven't been able to assign blame to it for any breakage, but I am still curious why this is so. Do you remember?

Cosimo: Oh, and thanks for the reviews!
Isaque: Thanks for finally getting this fixed. Good luck with Bijiben.

It turns out that it negatively affected Music's search view because it wants to continue having the text aligned against the top margin. I'd expect that to be fixed by having the application set the CellRenderer's yalign.