After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 792665 - GdTwoLinesRenderer ignores its vertical alignment
GdTwoLinesRenderer ignores its vertical alignment
Status: RESOLVED FIXED
Product: libgd
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: libgd maintainer(s)
libgd maintainer(s)
Depends on:
Blocks: 723843 728480
 
 
Reported: 2018-01-19 01:24 UTC by Isaque Galdino
Modified: 2018-02-27 12:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
two-line-renderer: Center title (1.38 KB, patch)
2018-01-19 15:07 UTC, Isaque Galdino
none Details | Review
two-lines-renderer: Simplify code (1.75 KB, patch)
2018-02-01 18:26 UTC, Debarshi Ray
committed Details | Review
two-lines-renderer: Don't ignore the aligned area's y-offset (901 bytes, patch)
2018-02-01 18:27 UTC, Debarshi Ray
committed Details | Review
two-lines-renderer: Don't ignore the y-offset when rendering (1.42 KB, patch)
2018-02-01 18:27 UTC, Debarshi Ray
committed Details | Review
main-icon-view: Explicitly specify the vertical alignment of the text (1.37 KB, patch)
2018-02-01 18:56 UTC, Debarshi Ray
committed Details | Review

Description Isaque Galdino 2018-01-19 01:24:21 UTC
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 I wonder if GdMainView could, when there is no line-two, to have an option to centralize it vertically.

I'm not sure if this is related to [0], but in our case, because we have other attributes in the same line, aligned at the middle of the line, it seems odd [1].

[0] https://bugzilla.gnome.org/show_bug.cgi?id=754737
[1] https://bugzilla.gnome.org/show_bug.cgi?id=762127
Comment 1 Isaque Galdino 2018-01-19 15:07:48 UTC
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.
Comment 2 Debarshi Ray 2018-02-01 13:12:56 UTC
(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.
Comment 3 Debarshi Ray 2018-02-01 13:47:55 UTC
(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.
Comment 4 Isaque Galdino 2018-02-01 14:09:11 UTC
(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.
Comment 5 Debarshi Ray 2018-02-01 14:18:59 UTC
(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.
Comment 6 Debarshi Ray 2018-02-01 15:07:33 UTC
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.
Comment 7 Debarshi Ray 2018-02-01 17:50:04 UTC
I am playing with some fixes in libgd:wip/rishi/two-lines
Comment 8 Debarshi Ray 2018-02-01 18:26:51 UTC
Created attachment 367765 [details] [review]
two-lines-renderer: Simplify code
Comment 9 Debarshi Ray 2018-02-01 18:27:05 UTC
Created attachment 367766 [details] [review]
two-lines-renderer: Don't ignore the aligned area's y-offset
Comment 10 Debarshi Ray 2018-02-01 18:27:32 UTC
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.
Comment 11 Debarshi Ray 2018-02-01 18:29:23 UTC
Dropping the duplicate bug from the blocker list.
Comment 12 Debarshi Ray 2018-02-01 18:30:33 UTC
This fixes both Bijiben and Documents for me.
Comment 13 Cosimo Cecchi 2018-02-01 18:54:39 UTC
Review of attachment 367765 [details] [review]:

LGTM
Comment 14 Debarshi Ray 2018-02-01 18:56:12 UTC
Created attachment 367770 [details] [review]
main-icon-view: Explicitly specify the vertical alignment of the text
Comment 15 Cosimo Cecchi 2018-02-01 18:56:23 UTC
Review of attachment 367766 [details] [review]:

Can't remember this code off hand, but I trust that you tested this.
Comment 16 Cosimo Cecchi 2018-02-01 18:57:14 UTC
Review of attachment 367767 [details] [review]:

++
Comment 17 Cosimo Cecchi 2018-02-01 18:57:53 UTC
Review of attachment 367770 [details] [review]:

OK
Comment 18 Debarshi Ray 2018-02-01 19:03:51 UTC
(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?
Comment 19 Debarshi Ray 2018-02-01 19:10:26 UTC
Cosimo: Oh, and thanks for the reviews!
Isaque: Thanks for finally getting this fixed. Good luck with Bijiben.
Comment 20 Debarshi Ray 2018-02-27 12:39:04 UTC
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.