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 696965 - [PATCH] gtkcellareabox: Fix renderer not always drawn when visible
[PATCH] gtkcellareabox: Fix renderer not always drawn when visible
Status: RESOLVED OBSOLETE
Product: gtk+
Classification: Platform
Component: Widget: GtkTreeView
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtktreeview-bugs
gtktreeview-bugs
Depends on:
Blocks:
 
 
Reported: 2013-03-31 13:28 UTC by Olivier Brunel (jjacky)
Modified: 2018-04-15 00:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gtkcellareabox: Fix renderer not always drawn when visible (1.83 KB, patch)
2013-03-31 13:28 UTC, Olivier Brunel (jjacky)
none Details | Review
gtkcellareabox: Fix renderer not always drawn when visible (1.92 KB, patch)
2013-10-15 15:39 UTC, Olivier Brunel (jjacky)
needs-work Details | Review
example (2.62 KB, text/x-c)
2013-10-24 12:14 UTC, Olivier Brunel (jjacky)
  Details
treeview: Fix possible column rendering issue (3.17 KB, patch)
2013-10-25 22:34 UTC, Olivier Brunel (jjacky)
none Details | Review
treeview: Fix possible column rendering issue (2.17 KB, patch)
2013-11-06 12:56 UTC, Olivier Brunel (jjacky)
none Details | Review

Description Olivier Brunel (jjacky) 2013-03-31 13:28:59 UTC
Created attachment 240204 [details] [review]
gtkcellareabox: Fix renderer not always drawn when visible

When a renderer was not originally visible, its natural size would be set to 0.
After that if would not be drawn even if visible, because of it.

Recalculing sizes for visible groups with no natural size will ensure we have
the correct size, most likely making sure all visible renderers are drawn as
expected.
Comment 1 Olivier Brunel (jjacky) 2013-10-15 15:39:49 UTC
Created attachment 257363 [details] [review]
gtkcellareabox: Fix renderer not always drawn when visible

Updated patch.
Comment 2 Tristan Van Berkom 2013-10-23 14:56:14 UTC
Thanks for the effort, this is a bit complicated tricky code so 
let me try to explain in detail what should be going on.

First of all, your patch here tries to conditionally
recalculate state in the allocation phase. This is just
wrong, if a cell renderer can become visible at this point
then there is no turning back... the implementing widget
(treeview or icon view) can not request space to fit the
new renderer here.

Here's how the code is supposed to run:

If the "visible" property is controlled by a treemodel column
then normally the view widget will ensure to request the width
of the row which changed (when the view receives a "row-changed"
signal from the GtkTreeModel), causing the right state changes
to occur in the GtkCellAreaContext.

If the user code manually calls g_object_set() on the renderer
to show a hidden renderer, then perhaps something like
gtk_tree_view_column_queue_resize() should be called as well
on the view widget.

When the width of a row might have changed, and the view can "only grow",
then gtk_cell_area_get_preferred_width() should be called.

When the view width should always be recalculated (it can also shrink),
then gtk_cell_area_context_reset() needs to be called and all
displayed rows need to be requested again (treeview would at this
point start requesting all rows all over again in a timeout).

I suspect that either you are using g_object_set() manually,
(in which case the cell area is never informed that the
visibility of a cell changed)... or... if there is a bug in
the normal code paths (i.e. the model attribute controls the
"visible" property), then probably some state variable is not
being set properly in the request phase.
Comment 3 Olivier Brunel (jjacky) 2013-10-23 17:11:15 UTC
Thanks for the review/explanation.

Indeed I use g_object_set() on the renderer, though this is in fact done in the "rendering function" (GtkTreeCellDataFunc) used instead of mapping columns in the model with properties on the renderer, so I don't think it makes any difference (since that function is called right after the mapping, which does pretty much the same thing).

So I believe there is a bug where making a renderer visible doesn't always result in said renderer being drawn. (Which, as I said, only manifests itself if the renderer wasn't originally visible, and there's no resizing of the column of course.)

That said, I can see now that I did put the code in a completely wrong place, yes. Looking at the code, after apply_cell_attributes() (in gtkcellarea.c) is when renderers could be made (non) visible, but this isn't a place to do this either (Besides, as a comment explains: "Whether a row expands or is presently expanded can only be provided by the view (as these states can vary across views accessing the same model).")

I'm still not exactly familiar with the whole drawing process of the treeview, which does have its complexity, but from what I can see, I'm now thinking maybe gtk_tree_view_column_cell_get_size() (which itself calls gtk_cell_area_context_get_preferred_width()) should be called in gtk_tree_view_bin_draw() right after the call to gtk_tree_view_column_cell_set_cell_data(), to ensure every renderer of every column has the right size calculated if (now) visible.

Would that be correct?
Comment 4 Tristan Van Berkom 2013-10-23 18:57:27 UTC
I think the correct approach is to manually recalculate the size of a
treeview column from the application at this point, whenever the
value of "visible" can change for a row.

This can be done with, as I mentioned before, gtk_tree_view_column_queue_resize().

Then, I would start with improving GtkTreeView documentation,
the function gtk_cell_layout_set_cell_data_func() doesn't say
anything at all about what the implications are in that call.

And unfortunately, it seems to work well enough most of the
time, so we have these undefined corner cases creeping through
because it's just not broken badly enough to fix :-/

The problem is that:

  a.) The treeview needs to know when the value of a row
      changes, that's when it might get rendered differently
      as a result

  b.) The treeview (or icon view or any view) receives these
      notifications for rows which change... on a row by row
      basis from GtkTreeModel

      This is how treeview can make an optimized decision on
      whether we need to recalculate everything right now
      or just require a bit more width because "this row"
      got a bit wider

  c.) The gtk_cell_area_apply_attributes() is what calls
      the cell data function, and is also what defaults
      to calling g_object_set_value() on the renderers
      based on model values.

      So the view widget can never know that a user
      used g_object_set(), it may have done so in the
      context of a cell data func, or outside of that
      context, or it could be done by the default
      function of gtk_cell_area_apply_attributes()

When data changes in such a way that it might be rendered differently,
the cell area needs to know... and it needs to recalculate the
size of the row which changed at the time that the value changed.

The rendering code needs to know this really at the time the
data changed, and not at the last moment before rendering.

The only way I can see that an application can do this, without
using the normal model attributes, is to explicitly call
gtk_tree_view_column_queue_resize() at the time which "visible"
can change for a given row (causing the treeview to internally
re-request size... well in advance of displaying it).
Comment 5 Olivier Brunel (jjacky) 2013-10-23 19:32:19 UTC
I understand what you're saying, but I think there's some confusion.

Yes, the treeview might be aware of when a gtk_tree_view_column_queue_resize() is due and call it, to (auto-)resize the columns. This would be an application issue.

But this isn't the issue here, this isn't about resizing: I do not expect/want any column to be resized here, but drawn as they should be. The issue is about the fact that a renderer isn't drawn even though it was set visible and, therefore, should have been drawn by GTK.

(It also has no link to calling g_object_set() since if I used a boolean value in the model w/ mapping, the result would be the same.)

Say I have a column, with two renderers: an icon, and a text. Only the text is visible originally, no icon. At some point, the icon becomes visible (and therefore the renderer's property "visible" is now set to TRUE, regardless of how).

When drawing this column, the icon *should* be drawn, always. If the size of the column doesn't allow to put both icon & text together, then the text will probably be cut, it might even be ellipsed, but within the allocated space for the column, both renderers are set to be visible and therefore should be, visible.

Currently, it's possible for this not to be the case, and to have the first renderer not drawn at all. It is a bug, that has nothing to do with (re)sizing issue, but the fact that when the *first calculations* were done, it wasn't visible and as a result it *now* still gets "excluded/skipped" even though it now is (set) visible and should be drawn.

As I said, if that renderer had been originaly visible, then hidden, and then made visible again, it would all work as expected (e.g. the text gets ellipsed when the icon is shown).

The only reason this doesn't happen is that the renderer wasn't visible originally, so apparently its CellGroup was not visible and so had a natural size of 0, and now that it is visible it remains still with the original natural size of 0, which is obviously wrong and leads to it not being drawn when it should. This is what I'm trying to fix.

This is why my original patch was located where it was actually, it wasn't about resizing/recalculating the (preferred) size for the area in order to have the column's size updated by the view, but making sure all groups' sizes are correct, and all drawing will be done properly (within the (unchanged) allocated space).
Comment 6 Tristan Van Berkom 2013-10-23 20:59:01 UTC
(In reply to comment #5)
> I understand what you're saying, but I think there's some confusion.
> 
> Yes, the treeview might be aware of when a gtk_tree_view_column_queue_resize()
> is due and call it, to (auto-)resize the columns. This would be an application
> issue.
> 
> But this isn't the issue here, this isn't about resizing: I do not expect/want
> any column to be resized here, but drawn as they should be. The issue is about
> the fact that a renderer isn't drawn even though it was set visible and,
> therefore, should have been drawn by GTK.

In fact it is about resizing.

The state of the cell renderer, when it has all attributes applied
(either by a cell data function or by column attributes) is what is
used to calculate the size of the row.

The size of the row depends on whether "visible" is TRUE of FALSE
for any given renderer.

If your renderer is suddenly visible at allocation/display time, but
was not visible at size request time... the content might very well
not fit. Having text renderers which "cut" at the end is just
not acceptable.

The treeview must know exactly how much width it requires before
allocating and drawing.

> 
> (It also has no link to calling g_object_set() since if I used a boolean value
> in the model w/ mapping, the result would be the same.)

How do you know that ?

Did you try it ?

> 
> Say I have a column, with two renderers: an icon, and a text. Only the text is
> visible originally, no icon. At some point, the icon becomes visible (and
> therefore the renderer's property "visible" is now set to TRUE, regardless of
> how).
> 
> When drawing this column, the icon *should* be drawn, always. If the size of
> the column doesn't allow to put both icon & text together, then the text will
> probably be cut, it might even be ellipsed, but within the allocated space for
> the column, both renderers are set to be visible and therefore should be,
> visible.

Right, see above, "it might be ellipsized" is just not enough. In order to
ensure the minimum characters required by the text renderer (if one is
configured) and rendering the ellipses at the right position, all require
that we know before hand exactly how much space will be required in advance.

If at a given point, your application would set "visible" to a different
value for a given row... then at that moment would be a good time to
call gtk_tree_model_row_changed() on that given row even.

The cell data function by itself cannot be relied upon to inform the
treeview/iconview widget that size needs to be recalculated for a given
row.

The fact that some internally cached data is not re-evaluated at
allocation time is orthogonal to the problem.

You might see the problem as:

  "When rendering rows... the cached data is incorrect and
   skips renderers which are supposed to be visible"

Sure, but the real problem is:

  "The cache data was never properly updated at the right
   time, even if we force render the visible renderers,
   we would be rendering widgets unpredictably into a space
   for which we have not properly negotiated size".

> 
> Currently, it's possible for this not to be the case, and to have the first
> renderer not drawn at all. It is a bug, that has nothing to do with (re)sizing
> issue, but the fact that when the *first calculations* were done, it wasn't
> visible and as a result it *now* still gets "excluded/skipped" even though it
> now is (set) visible and should be drawn.

Right, and when you say *first calculations*, you are referring to the
moment where size was requested for the given row.

If you invalidate the row, by calling gtk_tree_model_row_changed()
on the row for which you intend to start setting a new value
for "visible" in your cell data function, and you still have problems,
that would be a problem, that would mean that the view widgets are
not updating properly.

Right now the view widgets are displaying something different than
what they requested space for, so the results can not be defined
or trusted anyway.

> As I said, if that renderer had been originaly visible, then hidden, and then
> made visible again, it would all work as expected (e.g. the text gets ellipsed
> when the icon is shown).

If there was never a "row-changed" signal emitted for that row,
the one for which the renderer became visible, hidden, and visible again,
then anyway the results are pretty undefined.

> 
> The only reason this doesn't happen is that the renderer wasn't visible
> originally, so apparently its CellGroup was not visible and so had a natural
> size of 0, and now that it is visible it remains still with the original
> natural size of 0, which is obviously wrong and leads to it not being drawn
> when it should. This is what I'm trying to fix.

Ok.

Let's put it yet another way:

  a.) Debug gtk_cell_area_get_preferred_width(), check what
      value you are setting for the "visible" property when
      get_preferred_width() is called (the cell data func
      will be called implicitly).

  b.) Debug gtk_cell_area_render(), now chech again what
      value you are setting for the "visible" property when
      get_preferred_width() is called.

If the value of "visible" for A and B differ, the
underlying code cannot be expected to function.

If the value of "visible" for A and B are the same at
request and render time, then there is a bug in the
GtkCellAreaBox and GtkCellAreaBoxContext classes.

I.e. if they are getting the correct input but giving
you the incorrect output, then there is a bug in that
area of code to fix.

Otherwise, you are looking at something that is broken
by design, the fact that you can configure a row differently
in request phase than in the render phase, is just an
invitation to write broken code (which is why I mentioned
that fixing the documentation would be a good *start* towards
fixing this and improving the policies here).
Comment 7 Tristan Van Berkom 2013-10-23 21:15:04 UTC
(In reply to comment #6)
> (In reply to comment #5)
[...]
> Let's put it yet another way:
> 
>   a.) Debug gtk_cell_area_get_preferred_width(), check what
>       value you are setting for the "visible" property when
>       get_preferred_width() is called (the cell data func
>       will be called implicitly).
> 
>   b.) Debug gtk_cell_area_render(), now chech again what
>       value you are setting for the "visible" property when
>       get_preferred_width() is called.

Typo... I meant:


  a.) Debug gtk_cell_area_get_preferred_width(), check what
      value you are setting for the "visible" property when
      your cell data function is called.
 
  b.) Debug gtk_cell_area_render(), now chech again what
      value you are setting for the "visible" property when
      your cell data function is called.

In any case, the cell data function is called in both places
for each row that is requested or rendered, so it should be
pretty easy to find out if renderers are being configured
unpredictably (i.e. differently in request and render phases).
Comment 8 Olivier Brunel (jjacky) 2013-10-23 23:00:10 UTC
Alright, so I get what you're saying, but don't agree with everything.

The thing is that I did actually used to do a gtk_tree_view_column_queue_resize() when changing the visibility of the renderer at first, but I thought this was to workaround a bug (hence...) as I believed this shouldn't have to be needed since I only wanted renderers to be drawn on the same size as requested.

That is, because the size of the column doesn't change (no column autosize), only how things in that space are rendered, I didn't think a new size request was required. And I still think that is the case.

Now, I do in fact emit a row-changed on the model for the concerned row. I realize now I probably should have mention that earlier, sorry about that.

But yes, a row-changed is emitted on the model for that row, and this, as you said, should be enough. Also, this doesn't trigger a new size request from the treeview, actually. It only does so if column autosize is enabled, since in that case we want the allocated size to be updated (if needed).
Again, I'm not using column autosize here, so no such thing, and no need for a size request.

So I believe you are correct in saying that there's no new size request performed, but not in that it is actually needed/explains the problem.

Seems to me the problem remains that because a renderer/CellGroup wasn't visible, no calculations for it were done (in an effort to save time) and its natural size was set to zero. But now that it has become visible, it remains "excluded" and isn't being drawn, as it should be. Which is why my patch recalculated sizes for visible groups with no natural size, because that was most likely such an "error" of invalid cached data.

Now you might be right in that this shouldn't be done there/that way, but I'm not sure where it should be done then.
Comment 9 Olivier Brunel (jjacky) 2013-10-24 12:14:07 UTC
Created attachment 258011 [details]
example

Alright, so here's a little example I wrote, matching what I said before: it shows 2 treeviews which are identical; they have one column, made of 2 renderers: an icon, and a text.

The only difference is that the one of the left starts with the icon hidden, the one on the right the icon visible. Clicking the button will toggle the icon's visibility on both treeviews; This is done using attribute mapping (so row-changed is obviously emitted as expected).

You'd expect to see the icon "jump" from one treeview to the other, but instead only the icon on the right does toggle visibility; you never see the icon on the left, even when you should/it is visible. That's the bug.

Writing this example I realized something, in order for the bug to occur you also need to use the fixed height mode on the treeview (and therefore fixing sizing on the column).

Hope this can be helpful.
Comment 10 Tristan Van Berkom 2013-10-25 20:38:46 UTC
(In reply to comment #9)
> Created an attachment (id=258011) [details]
> example
> 
> Alright, so here's a little example I wrote, matching what I said before: it
> shows 2 treeviews which are identical; they have one column, made of 2
> renderers: an icon, and a text.
> 
> The only difference is that the one of the left starts with the icon hidden,
> the one on the right the icon visible. Clicking the button will toggle the
> icon's visibility on both treeviews; This is done using attribute mapping (so
> row-changed is obviously emitted as expected).
> 
> You'd expect to see the icon "jump" from one treeview to the other, but instead
> only the icon on the right does toggle visibility; you never see the icon on
> the left, even when you should/it is visible. That's the bug.
> 
> Writing this example I realized something, in order for the bug to occur you
> also need to use the fixed height mode on the treeview (and therefore fixing
> sizing on the column).
> 
> Hope this can be helpful.

Yes this is very helpful, unfortunately it's not that easy to fix.

Basically this proves that there is breakage even when the row-changed
signal is emitted for a given row whos visibility will change.

Also, the fact that GtkTreeView neglects to tell the GtkTreeViewColumn
about row changes (with fixed height mode ON and autosize OFF I guess)...
just because the GtkTreeView will not allocate any new sizes for the
GtkTreeViewColumn... is most probably to blame.

The treeview column & cell area need to be notified and requested
at least... not reset but at least requested for that one row
which changed (this is really not very expensive, and should
not cause the whole view to be recalculated / resized).

NOTE: I tried compiling your program against GTK+-2 and it
is broken in exactly the same way... it would seem the
same bug existed in GtkTreeView code before we replaced
the GtkTreeViewColumn code with the GtkCellArea stuff.
Comment 11 Olivier Brunel (jjacky) 2013-10-25 22:34:09 UTC
Created attachment 258151 [details] [review]
treeview: Fix possible column rendering issue

Alright, so here's a new attempt to fix this.

If I understand correctly, the bug is in the treeview after all, which doesn't properly trigger a size request for a row that asked for it (via row-changed signal).

So on the row-changed handler, in fix row height mode and if the row is visible, we call gtk_tree_view_column_cell_get_size() which will trigger the size request on the cell area.
Comment 12 Tristan Van Berkom 2013-11-06 06:02:15 UTC
(In reply to comment #11)
> Created an attachment (id=258151) [details] [review]
> treeview: Fix possible column rendering issue
> 
> Alright, so here's a new attempt to fix this.
> 
> If I understand correctly, the bug is in the treeview after all, which doesn't
> properly trigger a size request for a row that asked for it (via row-changed
> signal).
> 
> So on the row-changed handler, in fix row height mode and if the row is
> visible, we call gtk_tree_view_column_cell_get_size() which will trigger the
> size request on the cell area.

Olivier.

Your fix looks much more on target this time.

Actually reading through the code it looks just about exactly what I think 
should be done.

  a.) It does not reset the cell area contexts for the case of a row change
      when it should not (i.e. it should not slow down the fixed height mode
      or non-autosizing columns).

  b.) It causes only that single row to be recalculated which should update
      the cell area cache and cause the renderer to show up.

I don't like exactly how it's done, i.e. the 'fixed_refresh' variable implies
that the code is a "bug fix"

The code is not repaired so that the bug doesnt exist, instead the patchwork
is totally visible... I understand that you may have saved lines of code this
way but it doesnt really contribute to the readability of that function.

Also, to be honest, as this is really sensitive code, and Kristian is not
maintaining it anymore, I would like to try to get a second opinion before
pushing this.

For my review, as I said, it looks like it does exactly the right thing,
but does so in a way that doesnt improve readability of the function.
Comment 13 Olivier Brunel (jjacky) 2013-11-06 12:55:18 UTC
(In reply to comment #12)
> I don't like exactly how it's done, i.e. the 'fixed_refresh' variable implies
> that the code is a "bug fix"

Oh, I see what you mean. I have to say, I didn't intend it that way, in my mind the 'fixed_refresh' name wasn't about "fixing" but a reference to fixed row height mode, though I can see how poorly that was named.

I've been thinking about this patch though, and I think it was wrong to use node_is_visible() to see whether to trigger the size_request or not, because even if the row isn't visible the request should still be done. Else, and though the row was invalidated/will be redrawn, the original bug would still occur when the row is brought into view.

I'll add a revised patch.
Comment 14 Olivier Brunel (jjacky) 2013-11-06 12:56:03 UTC
Created attachment 259068 [details] [review]
treeview: Fix possible column rendering issue
Comment 15 Matthias Clasen 2018-02-10 05:24:57 UTC
We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.
Comment 16 Matthias Clasen 2018-04-15 00:33:13 UTC
As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla.

If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab:

https://gitlab.gnome.org/GNOME/gtk/issues/new