Created attachment 119963 [details] [review]
Initial patch

This patch adds an "extent" property to GooCanvasItem. The "extent" property is meant to describe the bounds of an item (without transformations applied). Since it is READWRITE, it can be used to resize items properly.

There are a few things to point out about the patch:

 * For most items the extent property is implemented by modifying their other properties, (such as x,y,width,height for GooCanvasRect), so no additional data is stored for the proporty.

 * For GooCanvasTable, only the width and height of the extent is used since it only has width and height properties, but not x/y ones. But they could probably be added easily.

 * For item models, the "extent" property is writeonly (meaning not readable). For most items it is not be a problem to read the property, but it is difficult for GooCanvasTextModel and GooCanvasTableModel when their width or height is set to -1.

 * For GooCanvasGroup, the property is not yet writable. Maybe this could be implemented by clipping items away that are not within the group's extents.

 * Another approach could be not to add the functionality to GooCanvasItem, but to make a separate interface out of it (GooCanvasItemResizable or something) and implement it in items for which it makes sense (perhaps not GooCanvasGroup).

 * The patch also includes a demo application "extent-demo" that allows to move and resize items using this technique. For resizing items within the table properly, the patch from bug #555093 needs to be applied as well.

Note that I asked Armin to to do this for Openismus, because I want a generic position API for base items, to see if it's possible. Damien, it's probably not worth you reviewing this until I've had a chance to do that myself.

I'm not really convinced we need a common interface for resizing items - I don't
think anyone else has requested that. We don't want to make it more complicated
to create new canvas items unless absolutely necessary.

I'd prefer it if a utility function was used to resize the items, which had
special code to handle each type of canvas item. The only problem with that is
that it wouldn't handle new types of canvas items, but I doubt that is needed.

Several people have requested it, in various places. Anyway, let me take a look and try to present it to you later.

(In reply to comment #1)
> Created an attachment (id=119963) [edit]
> Initial patch
> 
> This patch adds an "extent" property to GooCanvasItem. The "extent" property is
> meant to describe the bounds of an item (without transformations applied).
> Since it is READWRITE, it can be used to resize items properly.

I see that extent is a GooCanvasBounds, with x1, x2, y1, and y2.

> There are a few things to point out about the patch:
> 
>  * For most items the extent property is implemented by modifying their other
> properties, (such as x,y,width,height for GooCanvasRect), so no additional data
> is stored for the proporty.

Why did you choose to add an extra property? Why not just move x, y, width and height to GooCanvasItem and implement them for items that don't currently have them.

>  * For GooCanvasTable, only the width and height of the extent is used since it
> only has width and height properties, but not x/y ones. But they could probably
> be added easily.

Yes, that would be necessary.

> 
>  * For item models, the "extent" property is writeonly (meaning not readable).
> For most items it is not be a problem to read the property, but it is difficult
> for GooCanvasTextModel and GooCanvasTableModel when their width or height is
> set to -1.
> 
>  * For GooCanvasGroup, the property is not yet writable. Maybe this could be
> implemented by clipping items away that are not within the group's extents.

I think that setting the x,y of a GooCanvasGroup should be visually the same as translating it - the children should move.

For height, and width, yes, we could clip the area, as long as there is a documented way to unset the height and width (-1 ?).

>  * Another approach could be not to add the functionality to GooCanvasItem, but
> to make a separate interface out of it (GooCanvasItemResizable or something)
> and implement it in items for which it makes sense (perhaps not
> GooCanvasGroup).

I think it can be implemented for all items, making it simple. If not then we can consider that.

(In reply to comment #5)
> I see that extent is a GooCanvasBounds, with x1, x2, y1, and y2.

Is there a problem with this?

> Why did you choose to add an extra property? Why not just move x, y, width and
> height to GooCanvasItem and implement them for items that don't currently have
> them.

For most items, width and height are interpreted to the right or to the bottom, respectively, from the x/y coordinate of the item. However, there are a few that also have an anchor property (GooCanvasText and GooCanvasWidget) which determines how the width and height are interpreted. For example, an anchor of GTK_ANCHOR_CENTER means that x/y is the center of the item, not the upper left corner. This means that x/y/width/height is not enough to know the item's position, we would have to add such an anchor to every item as well. The extent property is always defined so that x1/y1 is the upper left and x2/y2 the lower right corner of the item.

Also, especially for GooCanvasPolyline and GooCanvasPath, it can be expensive to determine the x/y/width/height, so allowing to set all of them at once seems more efficient than setting each of them separately.

> >  * For GooCanvasTable, only the width and height of the extent is used since it
> > only has width and height properties, but not x/y ones. But they could probably
> > be added easily.
> 
> Yes, that would be necessary.
> 
> > 
> >  * For item models, the "extent" property is writeonly (meaning not readable).
> > For most items it is not be a problem to read the property, but it is difficult
> > for GooCanvasTextModel and GooCanvasTableModel when their width or height is
> > set to -1.
> > 
> >  * For GooCanvasGroup, the property is not yet writable. Maybe this could be
> > implemented by clipping items away that are not within the group's extents.
> 
> I think that setting the x,y of a GooCanvasGroup should be visually the same as
> translating it - the children should move.
> 
> For height, and width, yes, we could clip the area, as long as there is a
> documented way to unset the height and width (-1 ?).

I'll try out implementhing this.

> >  * Another approach could be not to add the functionality to GooCanvasItem, but
> > to make a separate interface out of it (GooCanvasItemResizable or something)
> > and implement it in items for which it makes sense (perhaps not
> > GooCanvasGroup).
> 
> I think it can be implemented for all items, making it simple. If not then we
> can consider that.
> 

(In reply to comment #6)
> (In reply to comment #5)
> > I see that extent is a GooCanvasBounds, with x1, x2, y1, and y2.
> 
> Is there a problem with this?

It just seems unnecessary, and makes the patch harder to like because it changes things that don't really need changing.

More practically, it makes it impossible to make the x, y, height and width individually read/write-only, though I don't know if that's actually needed.

> > Why did you choose to add an extra property? Why not just move x, y, width and
> > height to GooCanvasItem and implement them for items that don't currently have
> > them.
> 
> For most items, width and height are interpreted to the right or to the bottom,
> respectively, from the x/y coordinate of the item. However, there are a few
> that also have an anchor property (GooCanvasText and GooCanvasWidget) which
> determines how the width and height are interpreted. For example, an anchor of
> GTK_ANCHOR_CENTER means that x/y is the center of the item, not the upper left
> corner. This means that x/y/width/height is not enough to know the item's
> position, we would have to add such an anchor to every item as well. The extent
> property is always defined so that x1/y1 is the upper left and x2/y2 the lower
> right corner of the item.

I have no problem with just using the GooCanvasText x and y properties as they are. I'm more interested in adding these properties to other widgets, generically.

> Also, especially for GooCanvasPolyline and GooCanvasPath, it can be expensive
> to determine the x/y/width/height, so allowing to set all of them at once seems
> more efficient than setting each of them separately.

Why not just take the first point as the x,y position? As for GooCanvasText, any convention will do as long as its consistent and documented.

> re. GooCanvasTable:
> > For height, and width, yes, we could clip the area, as long as there is a
> > documented way to unset the height and width (-1 ?).
> 
> I'll try out implementhing this.

I suggest that we do this later when we agree about the API.

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > I see that extent is a GooCanvasBounds, with x1, x2, y1, and y2.
> > 
> > Is there a problem with this?
> 
> It just seems unnecessary, and makes the patch harder to like because it
> changes things that don't really need changing.
> 
> More practically, it makes it impossible to make the x, y, height and width
> individually read/write-only, though I don't know if that's actually needed.
> 
> > > Why did you choose to add an extra property? Why not just move x, y, width and
> > > height to GooCanvasItem and implement them for items that don't currently have
> > > them.
> > 
> > For most items, width and height are interpreted to the right or to the bottom,
> > respectively, from the x/y coordinate of the item. However, there are a few
> > that also have an anchor property (GooCanvasText and GooCanvasWidget) which
> > determines how the width and height are interpreted. For example, an anchor of
> > GTK_ANCHOR_CENTER means that x/y is the center of the item, not the upper left
> > corner. This means that x/y/width/height is not enough to know the item's
> > position, we would have to add such an anchor to every item as well. The extent
> > property is always defined so that x1/y1 is the upper left and x2/y2 the lower
> > right corner of the item.
> 
> I have no problem with just using the GooCanvasText x and y properties as they
> are. I'm more interested in adding these properties to other widgets,
> generically.
> 
> > Also, especially for GooCanvasPolyline and GooCanvasPath, it can be expensive
> > to determine the x/y/width/height, so allowing to set all of them at once seems
> > more efficient than setting each of them separately.
> 
> Why not just take the first point as the x,y position? As for GooCanvasText,
> any convention will do as long as its consistent and documented.

You mean that the x/y can be an arbitrary point inside the item, rather than the upper left one?

Does changing width and height with this convention still mean that the upper left corner is fixed and the lower right one changes position? This would mean that changing width and height also changes the x/y  (except the x/y happens to be the upper left corner of the item).

If we want to keep the x/y fixed when changing size, then the item expands to both directions when changing width and height.

Probably the first approach makes more sense, so that generically resizing an item does not depend on where the x/y is inside that item.

However, the "extent" property provides additional information about what area the item is on within the canvas. When the x/y is an arbitrary point inside the item instead of the upper left one, then this information is not available without generically knowing where the x/y is within the item.

I'm not sure whether anyone needs to actually know this, but I just wanted to point it out for completeness. So it seems to be simplicity vs. additional functionality of which we don't know whether anyone will need it.

> > re. GooCanvasTable:
> > > For height, and width, yes, we could clip the area, as long as there is a
> > > documented way to unset the height and width (-1 ?).
> > 
> > I'll try out implementhing this.
> 
> I suggest that we do this later when we agree about the API.

Changing the current "extent" API to one using x/y/width/height properties would not be a big deal. I just wanted to see how to do the actual implementation for this (which we would need anyway, regardless what API we decide to use).

> You mean that the x/y can be an arbitrary point inside the item, rather than
> the upper left one?

Yes, that's OK with me.

> Does changing width and height with this convention still mean that the upper
> left corner is fixed and the lower right one changes position?

Ideally, I guess. But I don't think it's that important. This will be a generic API.

> This would mean
> that changing width and height also changes the x/y  (except the x/y happens to
> be the upper left corner of the item).
>
> If we want to keep the x/y fixed when changing size, then the item expands to
> both directions when changing width and height.

Doesn't sound too bad. Again, it's a generic API. It wouldn't be my choice, but that's the decision that has already been made for us about GooCanvasText, and I'd like minimal changes.

> Probably the first approach makes more sense, so that generically resizing an
> item does not depend on where the x/y is inside that item.
> 
> However, the "extent" property provides additional information about what area
> the item is on within the canvas. When the x/y is an arbitrary point inside the
> item instead of the upper left one, then this information is not available
> without generically knowing where the x/y is within the item.

Isn't this what get_bounds() tells us anyway, when the application really wants that information?

> Changing the current "extent" API to one using x/y/width/height properties
> would not be a big deal. I just wanted to see how to do the actual
> implementation for this (which we would need anyway, regardless what API we
> decide to use).

Cool.

(In reply to comment #9)
> > You mean that the x/y can be an arbitrary point inside the item, rather than
> > the upper left one?
> 
> Yes, that's OK with me.
> 
> > Does changing width and height with this convention still mean that the upper
> > left corner is fixed and the lower right one changes position?
> 
> Ideally, I guess. But I don't think it's that important. This will be a generic
> API.
> 
> > This would mean
> > that changing width and height also changes the x/y  (except the x/y happens to
> > be the upper left corner of the item).
> >
> > If we want to keep the x/y fixed when changing size, then the item expands to
> > both directions when changing width and height.
> 
> Doesn't sound too bad. Again, it's a generic API. It wouldn't be my choice, but
> that's the decision that has already been made for us about GooCanvasText, and
> I'd like minimal changes.

I'm asking this because one use case that I had in mind while creating that initial patch is resizing items via drag+drop. It might be confusing if two different GooCanvasPolyline items behave differently when resizing just because their first point is different.

This is why I prefer to guarantee that items always expand into the direction to the left bottom when resizing. This way, it is easy to resize items into arbitrary directions by simply translating them after resizing. This is not so easy if the "direction of expansion" depends on an arbitrary x/y position.

> > Probably the first approach makes more sense, so that generically resizing an
> > item does not depend on where the x/y is inside that item.
> > 
> > However, the "extent" property provides additional information about what area
> > the item is on within the canvas. When the x/y is an arbitrary point inside the
> > item instead of the upper left one, then this information is not available
> > without generically knowing where the x/y is within the item.
> 
> Isn't this what get_bounds() tells us anyway, when the application really wants
> that information?

Yes, assuming the item is not scaled or rotated or something. But there is no set_bounds equivalent. The current "extent" API allows to set the area the item covers.

> > Changing the current "extent" API to one using x/y/width/height properties
> > would not be a big deal. I just wanted to see how to do the actual
> > implementation for this (which we would need anyway, regardless what API we
> > decide to use).
> 
> Cool.

> It might be confusing if two
> different GooCanvasPolyline items behave differently when resizing just because their first point is different.
> 
> This is why I prefer to guarantee that items always expand into the direction
> to the left bottom when resizing. This way, it is easy to resize items into
> arbitrary directions by simply translating them after resizing. This is not so
> easy if the "direction of expansion" depends on an arbitrary x/y position.

OK. I can see how this could be useful, but let's do that as an extra patch to make the x,y always be the top-left-most point and the width/height be based on the right-bottom-most point.

> > This is why I prefer to guarantee that items always expand into the direction
> > to the left bottom when resizing. This way, it is easy to resize items into
> > arbitrary directions by simply translating them after resizing. This is not so
> > easy if the "direction of expansion" depends on an arbitrary x/y position.
> 
> OK. I can see how this could be useful, but let's do that as an extra patch to
> make the x,y always be the top-left-most point and the width/height be based on
> the right-bottom-most point.

Note that, if we want to use existing "x", "y", "width" and "height" properties where they exist, then this would break compatibility with existing items such as GooCanvasText (as described in comment #6). This is why I eventually chose to add an extra property.

Good point. But we can discuss that later. For now, I want to keep it simple.

I'd like it a bit more if it used interface methods instead of properties.

That way you could keep it entirely separate from the rest of the API
and people could ignore it when implementing new items if they wanted to.

If you use properties it gets tangled up with the other properties and you
have to do all that g_object_notify() stuff.

Wouldn't you then have set_x/y() on various items, plus set_generic_x(), set_generic_y()? That wouldn't be a very pleasant interface.

My intention is for this to be implemented by all items, not optional. That's the point of it. 

Oh, I was commenting on the "extent" property in the first patch, but it looks
like you're leaning towards changing any existing "x", "y", "width" & "height" now. That is a bit nicer.

Created attachment 120710 [details] [review]
Use "x", "y", "width", "height" properties instead

This patch now reuses the "x", "y", "width" and "height" properties where they exist and adds them for the other items.

It implements clipping for GooCanvasGroup when the width and height are set, which is also demonstrated in the extent-demo. Setting a width or height of -1 disables clipping (the default).

I added additional x,y members to the table, for implementing the "x" and "y" properties.

1.

The comment
"
The group is clipped to this region, except width or height is -1 in which case it is unset.
"
would be correctly written as 
"
The group is clipped to this region unless width or height are -1, in which case the clipping is deactivated.
"
and this should probably be in the description documentation for the actor.

2.
GooCanvasWidget had this documentation for its height/width properties:
"The width of the widget, or -1 to use its requested width."
We should not lose that documentation so that should probably be in the GooCanvasWidget description.

3.
GooCanvasText has a TOOD:

case PROP_HEIGHT:
      /* TODO: Clip accordingly, if set? */
      break;

What are the choices here? How does/did the width property behave?

4.
Shouldn't you be using g_object_class_override_property() instead of g_object_interface_install_property(), or are you just copying some existing code?


5.
Are you sure that we should be defining new PROP_* enums in the derived items?

(In reply to comment #18)
> 3.
> GooCanvasText has a TOOD:
> 
> case PROP_HEIGHT:
>       /* TODO: Clip accordingly, if set? */
>       break;
> 
> What are the choices here? How does/did the width property behave?

The width property makes the text wrap, so the item requires more height.

> 4.
> Shouldn't you be using g_object_class_override_property() instead of
> g_object_interface_install_property(), or are you just copying some existing
> code?

To which class do you refer to here? As GooCanvasItem and GooCanvasItemModel are interfaces we can't use g_object_class_something() on them. The other properties of GooCanvasItem are installed using g_object_interface_install_property() as well.

> 5.
> Are you sure that we should be defining new PROP_* enums in the derived items?

GObject requires that any property installed in an interface needs to be overriden in classes implementing the interface. The new PROP_* enumerators are required for this, and for implementing theproperty in the get_property and set_property functions.

Created attachment 120961 [details] [review]
Implement GooCanvasText:height

This patch fixes the mentioned documentation issues and implements the height property for GooCanvasText by clipping the text to the height set as long as it is not -1.

Thanks.

extent-demo.c seems to be missing - maybe you didn't svn add it. I think it should be called generic-position-demo.c or suchlike now anyway.

Created attachment 121061 [details] [review]
Including generic-extent-demo.c

Oops, right. Here it is.

Thanks. The ChangeLog entry needs to be corrected for the new name. And I think that the clipping decisions should be mentioned in the ChangeLog.

Created attachment 121066 [details] [review]
Updated ChangeLog

Created attachment 121536 [details] [review]
(x,y) always refers to the topleft corner

This patch changes the "x" and "y" properties of GooCanvasEllipse, GooCanvasPolyline and GooCanvasPath to always refer to the topleft corner of the item, instead of some other point, as discussed on the mailing list.

Created attachment 121781 [details] [review]
Don't add the generic properties to GooCanvasItem

With this patch, "x", "y", "width" and "height" properties are added to items that don't have them yet, without adding these to the GooCanvasItem interface. Also, GooCanvasText makes sure to only show lines that are fully visible.

Created attachment 122040 [details] [review]
Fix a crash when using models with private data

This patch fixes a problem with items that have new instance variables via g_type_class_add_private (GooCanvasGroup, GooCanvasText). It crashed when instantiating model versions of these.

Damon, are any of these patches likely to be committed?

This one will be eventually. But I need to spend a few hours going through it at some point.

I'm about to commit this, but I did make quite a few changes:

GooCanvasEllipse:

  o The calculations of the x & y properties look wrong in get/set_property():

+    case PROP_X:
+      g_value_set_double (value, ellipse_data->center_x - ellipse_data->radius_x/2.0);

    I don't think you need to divide the radius by 2.0.

GooCanvasPath:

  o cairo_path_extents() is a fairly new function in cairo, so I'd rather
    use cairo_fill_extents() for now, so we don't need to depend on a new
    cairo.

GooCanvasText:

  o To handle the new "height" property I used the simple clipping code
    from a previous patch, as the newer code which only displayed complete
    lines looked like it might have problems with markup, and was a bit
    inefficient. Maybe we can add extra properties to do that in future,
    when we can depend on pango_layout_set_height().

GooCanvasTable, GooCanvasText, GooCanvasWidget:

  o I didn't like returning the final width or height of the item in
    get_property(). If the "width" or "height" property is set to -1.0,
    then I think it should just return -1.0. (This also meant we didn't
    need to call g_object_notify() when the widget was set or the table
    size changed, which I didn't like much either.)

GooCanvasGroup & GooCanvasText:
  o When the item had a model I used the model's private data directly
    rather than trying to keep the item's private data up-to-date with it.

I also added a model/view demo to test that code.

I'll attach my patch here, so people can check through it. It would be
handy if people tested it out as well. I'm still not too sure that the
table allocation code will work when the new properties are used.

Created attachment 126319 [details] [review]
Updated patch, about to commit

Thanks for committing that. It lets me remove a little code from Glom.

Armin, I still believe that the interface (the properties) should be in a shared base class (GooCanvasItem) so they can be used polymorphically. Could you please  create a patch to make that change against the latest svn. Even if it's just for me.

Created attachment 129845 [details] [review]
Adding position properties to base class

This patch adds the "x", "y", "width" and "height" properties to the GooCanvasItem and GooCanvasItemModel classes, and override them in their children.

For some classes, this requires allowing the width or height being set to -1, in which case it simply chooses an arbitrary default (for example, for GooCanvasRect, 100.0).

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/goocanvas/-/issues/14.