a standalone testcase would be appreciated

Created attachment 242644 [details]
Example Glade file

Run the attached Glade file with:

glade-previewer -f broken-grid-test.glade

Notice that it behaves normally before the mentioned commit,
and after the mentioned commit there is no way to again obtain
the same functionality, no workaround.

What I have noticed is, this is happening because one widget which spans
the most columns is set to hexpand.

'normally' is a pretty unclear description of the expected behavior.

It seems you expect that the grid should prefer to expand the third column, because it has no non-expanding children in it ?

This is not as clear-cut as you might think. In fact, I just reverted a change that made the grid consider columns containing only spanning children as expandable because it caused problems in gnumeric, see bug 698660.

Here is what the grid currently does:

When faced with a spanning, expanding child, if none of the rows it spans is expanding due to a non-spanning expanding child, it marks all the spanned columns as expanding.

You can certainly work around this by putting some invisible child next to the buttons, and marking it as expandable.
That will give the grid the hint to make the third column expandable and leave the first two alone.

And with invisible I meant 'non-drawing', if course - e.g. an empty label

(In reply to comment #3)
> 'normally' is a pretty unclear description of the expected behavior.
> 

By 'normally' I mean, in the way it has been behaving previously, in the
way that it should work in order to not break code which has the same
expectations (which I expect to be, a lot of GtkGrid using code).

> It seems you expect that the grid should prefer to expand the third column,
> because it has no non-expanding children in it ?
> 
> This is not as clear-cut as you might think. In fact, I just reverted a change
> that made the grid consider columns containing only spanning children as
> expandable because it caused problems in gnumeric, see bug 698660.

Not at all ;-)

I expect GtkGrid allocation to be a very delicate and sensitive area to
be making modifications to, don't exactly expect that it's clear cut.

> 
> Here is what the grid currently does:
> 
> When faced with a spanning, expanding child, if none of the rows it spans is
> expanding due to a non-spanning expanding child, it marks all the spanned
> columns as expanding.

This seems to indeed be the behavioural change which caused this regression
to happen, this was not the policy before the mentioned baseline related
commit.

> 
> You can certainly work around this by putting some invisible child next to the
> buttons, and marking it as expandable.
> That will give the grid the hint to make the third column expandable and leave
> the first two alone.

Well, here's to hoping that it wont come to adding padding widgets to the UI,
would be rather sad if GTK+ necessitated that.

(In reply to comment #5)
> 
> I expect GtkGrid allocation to be a very delicate and sensitive area to
> be making modifications to, don't exactly expect that it's clear cut.
> 
> > 
> > Here is what the grid currently does:
> > 
> > When faced with a spanning, expanding child, if none of the rows it spans is
> > expanding due to a non-spanning expanding child, it marks all the spanned
> > columns as expanding.
> 
> This seems to indeed be the behavioural change which caused this regression
> to happen, this was not the policy before the mentioned baseline related
> commit.

Yes, it was.
I should know, I wrote the code :-)
As I said, I tried to change this policy in 
https://git.gnome.org/browse/gtk+/commit/?id=5e1a06d1b124f09a1a54d5ae0f38905dbdc2cc81
but had to revert this change in 
https://git.gnome.org/browse/gtk+/commit/?id=13858fde29e9626d1d959d436f87105263bced98

> Well, here's to hoping that it wont come to adding padding widgets to the UI,
> would be rather sad if GTK+ necessitated that.

If you can propose a meaningful policy that would make your empty-except-for-spanning-children column expand while keeping the empty-except-for-spanning-children rows in the 'Spanning' example of testgrid collapsed, I'm all ears.

The policy as I understand it, is that widgets don't expand unless explicitly
set to expand, which is a concept basically carried over to rows/columns
of a GtkGrid.

In the case that one widget spans multiple columns, it should not be the
deciding factor to cause *all* of it's columns to expand, but rather require
that at least one of the columns it occupies expands.

But I think I can word this into a meaningful policy, here it goes:

Once a widget has declared itself as expanding, there are 2 criteria
for expanding a given column which that widget spans:

  a.) Every other row either agrees with the expanding of the given column,
      or makes no argument for a given column.

      I.e. a column is in agreement for a given row if it explicitly 
      expands or contains no widget in that column.

  b.) Any other row disagrees completely with an expanding widget

      example: one widget expands across 3 columns,
      and another row is full of non-expanding widgets which
      span the same columns.

      In the case of 'b' where we find full disagreement between
      rows, all of the effected columns expand equally.

      Also in terms of code, it would seem that 'b' is the fallback
      case; after having checked with all rows and still not satisfying
      the 'expand' declaration of a given widget, all of the columns
      which that widget span end up expanding.

The same criteria that applies to columns/hexpand, of course applies
to rows/vexpand.

I'm pretty sure the above policy is pretty much what GtkGrid has been
doing up until now, at least it *feels* this way using GtkGrid, I
hope that the above is clear and comprehensible ;-)

so, now tell me: does an empty cell 'agree' with expanding, or not ?

Indeed it does (I think I mentioned that in 'a'), as in it does not make
any argument.

But having empty cells expand by default is what breaks the gnumeric use case.

You can try to wiggle your way out by arguing that in that case, you could make another row expand, but then you get into really hairy ordering issues regarding which rows and spanning children you check in which order.

I'd be happy to test a patch, but I'm not too hopeful that you can come up with something that works in all cases.

(In reply to comment #10)
> But having empty cells expand by default is what breaks the gnumeric use case.
> 

I never said that empty cells expand by default.

I said that empty cells do not challenge the explicit expand which was
requested for the same column in another row (I realize what I tried to
explain above is complex in a way, I tried to not use too many words
in order to describe it).

To reiterate what I said previously, in pseudo code:

foreach (expanding widget in grid) {

   bool did_expand = FALSE;

   foreach (column in columns which the widget spans) {

     foreach (row in grid) {
        if (row[column] == NULL ||
            row[column] == expanding widget) {

            this_column_expands();

            did_expand = TRUE;
        }
     }
   }

   /* Every column which an expanding widget spanned
    * had a disagreement with the expand clause
    */
   if (did_expand == FALSE)
     all_of_the_expanding_widgets_columns_expand();
}

> You can try to wiggle your way out by arguing that in that case, you could make
> another row expand, but then you get into really hairy ordering issues
> regarding which rows and spanning children you check in which order.

I don't think it's that hairy, because the fallback of distributing
expand to all columns of an widget widget kicks in as soon as there
is explicit disagreement found in all of the columns.

So long as there is at least one column which does not explicitly
disagree, you can give expand space to that column.

Of course when a widget only spans one column, it unconditionally
expands it's column (because it would hit the fallback right away).

> 
> I'd be happy to test a patch, but I'm not too hopeful that you can come up with
> something that works in all cases.

I'm really trying to be constructive here, and now I get the feeling
that I'm getting on your nerves, if that's the case well, I'm sorry
for that.

It's quite clear that something has changed in the noted commit
627685e2a2efd9be72e66dd1a1e8f49aab1d1600 which changes the original
behaviour.

From what I understand, your following commits (the first one which
tries to address the breakage but breaks gnumeric in doing so), come
after the original breakage.

Something was obviously lost in the calculation of expanding columns/rows
with original baseline related commit.

You say that you're not hopeful that a solution can be found to work
in all cases, my response to that logically has to be that:

 o Before the mentioned baseline commit, the code must have worked in all
   cases, otherwise gnumeric must have been broken in GTK+ 3.8 and before
   as well.

 o Perhaps the baseline related commit tries to innocently sneak in a
   new requirement, one which is unrealistic to try to satisfy without
   breaking things.

I doubt that the latter is the case, I don't see why baselines would
effect the expanding state of widgets, but I'm quite convinced of the
former.

(In reply to comment #10)
[...]
> You can try to wiggle your way out by arguing that in that case, you could make
> another row expand...

Also, in order to make sure we are on the same page, we are talking columns
here, not rows (talking both rows and columns certainly makes things even
more confusing).

Until now I've only been talking about the hexpand attribute of child
widgets causing columns to expand, by examining cells in the same columns
while iterating through the sibling rows... whatever algorithm which
applies to hexpand/columns, can work with vexpand/rows as well.

(In reply to comment #11)

> > 
> > I'd be happy to test a patch, but I'm not too hopeful that you can come up with
> > something that works in all cases.
> 
> I'm really trying to be constructive here, and now I get the feeling
> that I'm getting on your nerves, if that's the case well, I'm sorry
> for that.

You're not. Sorry if I gave you that impression. This is an interesting problem and I enjoy discussing it.

>  o Before the mentioned baseline commit, the code must have worked in all
>    cases, otherwise gnumeric must have been broken in GTK+ 3.8 and before
>    as well.

I've added the gnumeric testcase to testgrid now, so we can easily find out !


And just to clarify: this is a 1-dimensional problem, I'm talking about rows/columns interchangeably here. We decide their expandability independently and separately.

I wonder if I might be stumbling upon the same problem here. I have a GtkGrid like this:

 ------------------
|               |   2   |  3   |
|      1       |---------|
|               |                 |
|               |                 |
|--------|                 |
|               |      4         |
|               |                 |
|       5      |                 |
|               |                 |
|               |                 |
 ------------------

1. left=0, top=0, width=1, height=2, expand=false, align=start
2. left=1, top=0, width=1, height=1, align=start, hexpand=true, vexpand=false
3. left=2, top=0, width=1, height=1, valign=start, halign=end, hexpand=true, vexpand=false
4. left=1, top=1, width=2, height=2, halign=fill, valign=start, hexpand=true, vexpand=false
5. left=0, top=2, width=1, height=1, align=start, expand=false

The behavior I want is that the widget in cell 4, a GtkTreeView, is always aligned at the top of cell 4. The GtkTreeView should be rendered with less height if it's smaller than the contents of 1+5, and larger if larger, but always start-aligned.

A screenshot of this is here:
http://people.xmms2.org/~nano/weird-layout3.png

As you can see, the distance between the GtkTreeView and the labels above it (cells 2 and 3 in my ascii art) differs, when it should be small (margin-top=10 for the GtkTreeView). 

There are a couple of wtf's in that screenshot. While the middle instance of the GtkGrid seems to have the correct (or close to correct?) distance to the above labels, the first instance does not, even if its GtkTreeView is taller than the content of the left column. All instances of this GtkGrid leave too much dead space at the bottom, each with a different amount of dead space. There is no margin-bottom defined.

The alignment/expand of the composed widget as a whole are:
valign=start, halign=fill, hexpand=true, vexpand=false
...and the instances are placed in another GtkGrid when created.

The widgets are laid out the same way in both Gtk master, as in 3.8.2.

Formatting failed, uptade ascii drawing:

 ---------------------------------
|               |   2     |   3   |
|      1        |-----------------|
|               |                 |
|               |                 |
|---------------|                 |
|               |       4         |
|               |                 |
|       5       |                 |
|               |                 |
|               |                 |
 ---------------------------------

nothing is happening here for 3.10, at this stage

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.

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