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 60331 - Object properties should handle multiple objects
Object properties should handle multiple objects
Status: RESOLVED FIXED
Product: dia
Classification: Other
Component: general
0.95.1
Other All
: High major
: 0.97
Assigned To: Sameer D. Sahasrabuddhe
Dia maintainers
: 105083 113139 117417 134310 138071 140928 167497 301449 305108 310104 392827 526284 526906 538617 606943 (view as bug list)
Depends on: 57060
Blocks:
 
 
Reported: 2001-09-10 19:24 UTC by Devin Carraway
Modified: 2010-01-14 16:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Dirty way of fixing this bug (1.38 KB, patch)
2005-01-26 16:33 UTC, Toady
rejected Details | Review
signal handling in the properties dialog to handle only those properties that have changed (2.29 KB, patch)
2007-02-01 05:01 UTC, Sameer D. Sahasrabuddhe
committed Details | Review
corrections to the patch dynamic menu signal handling (3.37 KB, patch)
2007-04-18 07:42 UTC, Sameer D. Sahasrabuddhe
none Details | Review
signal renamed in DiaDynamicMenu, removes all warnings observed (9.55 KB, patch)
2007-04-19 05:03 UTC, Sameer D. Sahasrabuddhe
committed Details | Review
simplified some widgets in the property dialog and all of them now have an event handler to track PXP_NOTSET (15.93 KB, patch)
2007-07-12 04:38 UTC, Sameer D. Sahasrabuddhe
committed Details | Review
implements undo for groups; but redo does not work; recursion not verified (5.05 KB, patch)
2007-07-30 11:18 UTC, Sameer D. Sahasrabuddhe
none Details | Review
undo/redo in setting properties now works for grouped objects, even with nested groups (69.20 KB, patch)
2007-08-01 11:32 UTC, Sameer D. Sahasrabuddhe
needs-work Details | Review
retrofitting the vtable with a new function to handle undo, especially when changing group properties (84.90 KB, patch)
2007-10-02 06:33 UTC, Sameer D. Sahasrabuddhe
none Details | Review
retrofitting the vtable with a new function to handle undo, especially when changing group properties (84.56 KB, patch)
2007-10-02 06:58 UTC, Sameer D. Sahasrabuddhe
needs-work Details | Review
apply_properties_list() makes correct use of an unused slot; assertion changed to a warning (83.43 KB, patch)
2007-10-16 12:47 UTC, Sameer D. Sahasrabuddhe
committed Details | Review
introduce a separate function to return property descriptions for a list of objects (5.47 KB, patch)
2008-12-03 17:33 UTC, Sameer D. Sahasrabuddhe
committed Details | Review
the properties dialog now tracks multiple objects (16.62 KB, patch)
2008-12-07 11:05 UTC, Sameer D. Sahasrabuddhe
committed Details | Review
updated the vtable for Database/table.c to use the correct function (1015 bytes, patch)
2008-12-09 09:16 UTC, Sameer D. Sahasrabuddhe
committed Details | Review

Description Devin Carraway 2001-09-10 19:24:07 UTC
At present, the object properties dialog can only alter a single shape, and
it's not always clear what shape will be adjusted.

For the former, it would be much more efficient for some uses (e.g.
changing the background color of a whole swath of objects) if the object
properties, or at least the primitive ones (colors, line style, font) could
be changed en-masse.  Alternately, a bounding set of all possible property
changes could be shown in the object properties dialog, if that was easier
to implement.  If it really must affect only a single shape, it would be
useful to have a visual indication of which object is being changed --
scrolling to it and giving it an animated or colored outline, to
distinguish it from the other selected objects, for example.

For the latter, there's some confusion in which object will be altered when
the object properties dialog is invoked (esp via the menu) while multiple
objects are selected.  It seems to be the rule that the most recently
selected shape is the one affected, but this requires more temporal
recollection than the user should be required to do.
Comment 1 James Henstridge 2002-05-02 01:26:11 UTC
Mass reassign of bugs to dia-maint@bugzilla.gnome.org.
Comment 2 andrew.junk 2002-05-26 10:10:58 UTC
I second this very, very strongly. This is one of my biggest
bugbears when using Dia and it's more and more annoying the more
objects one has. It would be great if this could be added for 0.91
Comment 3 Lars Clausen 2002-06-17 19:44:23 UTC
I agree with you totally.  Even I find myself trying to set properties for a multiple selection.
Note that if you group the objects, you can set properties for the entire group.  This, however, is not an intuitive solution.
Comment 4 Lars Clausen 2002-11-26 21:00:54 UTC
Upping priority.  I would like this done for release 0.91.
Comment 5 Lars Clausen 2003-02-03 15:57:12 UTC
*** Bug 105083 has been marked as a duplicate of this bug. ***
Comment 6 Lars Clausen 2003-02-03 15:57:58 UTC
Won't make it for 0.91.  As a usability issue, a must for 0.92.
Comment 7 Lars Clausen 2003-07-15 06:37:07 UTC
*** Bug 117417 has been marked as a duplicate of this bug. ***
Comment 8 Lars Clausen 2004-02-23 13:57:23 UTC
*** Bug 113139 has been marked as a duplicate of this bug. ***
Comment 9 Lars Clausen 2004-02-26 22:49:45 UTC
*** Bug 134310 has been marked as a duplicate of this bug. ***
Comment 10 Lars Clausen 2004-03-25 17:29:08 UTC
*** Bug 138071 has been marked as a duplicate of this bug. ***
Comment 11 Hans Breuer 2004-05-17 20:31:01 UTC
*** Bug 140928 has been marked as a duplicate of this bug. ***
Comment 12 Alan Horkan 2004-11-27 00:22:44 UTC
Bugs should not target milestones we have already passed, mass retargetting to
version 0.95.  
Comment 13 Toady 2005-01-26 16:33:22 UTC
Created attachment 36560 [details] [review]
Dirty way of fixing this bug

This patch allows you to apply the properties from the object you get
properties from to all objects selected.
This is not a really nice way of resolving this issue though, but I let
maintainers decide what to do with this.
A cleaner way would be to get the properties in common among all the objects
selected, if I'm asked to do so, I can manage to work on this issue.
Comment 14 Hans Breuer 2005-01-28 21:22:47 UTC
IMO there needs to be a more advanced version. Some work in progress is discused
on the list:
http://mail.gnome.org/archives/dia-list/2005-January/msg00033.html
Comment 15 Toady 2005-01-30 23:58:48 UTC
Looks like just what I said : You must only display properties that all the
objects selected have in common. I'll provide something as soon as I have time.
Comment 16 Lars Clausen 2005-02-16 13:45:11 UTC
*** Bug 167497 has been marked as a duplicate of this bug. ***
Comment 17 Lars Clausen 2005-03-01 12:34:16 UTC
Comment on attachment 36560 [details] [review]
Dirty way of fixing this bug

This patch a) does not deal with different kinds of objects being selected, b)
smashes unchanged properties that differ between objects, and c) does not work
with undo.  The discussion on the dia list has better work in this direction.
Comment 18 Lars Clausen 2005-03-22 06:17:23 UTC
Without that patch, we can't expect to get this working for 0.95
Comment 19 Toady 2005-03-24 18:41:36 UTC
ok, I should find time to fix this patch with your instructions.
That shouldn't be so hard to fix. I will do my best.
Comment 20 Lars Clausen 2005-04-21 17:25:29 UTC
*** Bug 301449 has been marked as a duplicate of this bug. ***
Comment 21 Lars Clausen 2005-05-22 20:11:50 UTC
*** Bug 305108 has been marked as a duplicate of this bug. ***
Comment 22 Alan Horkan 2005-07-12 14:10:49 UTC
*** Bug 310104 has been marked as a duplicate of this bug. ***
Comment 23 Sameer D. Sahasrabuddhe 2006-11-23 08:28:08 UTC
This is how I think the properties dialog should work for multiple selected objects:

1) When objects of different kinds are selected, the dialog should actually show a union of all the applicable properties, not their intersection. The user can then freely select objects and set properties. Each property will be set for all objects for which it actually exists. Of course, this is subject to the requirement that the number of properties displayed is not overwhelming to the user.

2) When a property has different values for different objects, the dialog should show it as grayed out, but not disabled. If the user sets a value to this property, it gets coloured again, and is set for all objects as applicable. If the user does not modify it, each object retains the existing value for that object. Currently if I group a dashed line and a solid line, and try to change their colour, both lines become either dashed or solid, depending on which was selected last.

A very good implementation of such an interface can be seen in Ex Falso, the tag editor for audio files.
Comment 24 Lars Clausen 2006-12-02 06:55:49 UTC
I agree with Sameer on how it should work.  However, implementation is not trivial.  *Working* patches would be welcome.  Not happening by my hand this version, as text handling has superseded it.
Comment 25 Hans Breuer 2007-01-05 17:53:29 UTC
*** Bug 392827 has been marked as a duplicate of this bug. ***
Comment 26 Sameer D. Sahasrabuddhe 2007-01-30 11:13:33 UTC
(In reply to comment #24)
> I agree with Sameer on how it should work.  However, implementation is not
> trivial.  *Working* patches would be welcome.  Not happening by my hand this
> version, as text handling has superseded it.

Been spending some time exploring the dia source code ... I think I at least found the place where work needs to be done, and some idea of what changes to make. I havn't joined the mailing list yet, but I hope someone will give a general nod here so that I can think of implementing an actual solution.

I suppose the start would be to replace the call to prop_desc_lists_intersection() in lib/group.c with something better, that creates a union, and at the same time marks properties that have different values. The relevant flag seems to be PROP_FLAG_MULTIVALUE. Once we have a property list with these marked properties, we can modify the property dialog to gray out these marked entries, and apply them only if changed.

Eventually, the same kind of union can be created for a general ungrouped selection of objects. Am I right??
Comment 27 Sameer D. Sahasrabuddhe 2007-02-01 05:01:30 UTC
Created attachment 81642 [details] [review]
signal handling in the properties dialog to handle only those properties that have changed

This patch makes it possible to show a properties dialog that is the union of properties for all objects in a group. The PXP_NOTSET flag is used in the property experience field to determine which properties are actually modified by the user.

This patch attaches the higher-level property_signal_handler() without checking for prop->event_handler. The signal_handler will call prop->event_handler only if defined.

The patch will not work in the current state. For that we need to define a suitable signal for all property widgets that are currently using "FIXME".

Also, some signal connections are not working. I couldn't figure out why due to a general lack of experience with Gtk+, but I have fixed one example for the REAL property type.
Comment 28 Lars Clausen 2007-02-06 19:44:52 UTC
It's a little confusing to look at what's happening, but it does look like you've gotten them to only change if the user has actually fiddled with the value.  Some indication of whether a value has been changed or not will be necessary, as will as "reset" button, given that you can't reset to a multivalue.  What I see definitely not working is that only one object is actually changed when applying.
Comment 29 Sameer D. Sahasrabuddhe 2007-02-08 14:18:11 UTC
(In reply to comment #28)
> It's a little confusing to look at what's happening, but it does look like
> you've gotten them to only change if the user has actually fiddled with the
> value.

Right ... "fiddled" is the correct word, and not "changed". It seems quite non-trivial and unnecessary to actually check whether the value has changed after the user has fiddled with the property widget in the dialog.

> Some indication of whether a value has been changed or not will be
> necessary, as will as "reset" button, given that you can't reset to a
> multivalue.

Yes. This is more of a "user-friendliness" call, which I am not experienced enough to make. The indication could be some form of colour change. For the reset, we could have a tickbox per property, like the current python plugin that tries to provide a similar functionality. And of course a reset button in addition. Basically, I don't know what to do with the UI ... hoping someone else can provide better ideas.

Aside from that, I have a feeling that using a union of properties might be quite overwhelming for a user and probably not very useful ... an intersection might turn out to be better.

> What I see definitely not working is that only one object is
> actually changed when applying.

Please explain!

Comment 30 Lars Clausen 2007-03-08 21:02:33 UTC
Since each property can start out multi-valued, the user cannot easily change a value back through the control, so a value-change check doesn't make sense.

I think a checkbox at each value would be the right thing -- definitely with a tooltip.  The user could then uncheck it to reset the property to its original (possibly multivalued) value.  The concept of the checkbox would be "set this property".

The union has the disadvantage that the user cannot tell which objects are affected.  However, I don't think it's something we need to strictly decide on now, it'd be nice to try either way, given that the code difference isn't too great.  

I tried using your code to change the values of several selected objects, but only one object actually changed value.  I can give you a more precise description if you want.
Comment 31 Sameer D. Sahasrabuddhe 2007-03-09 04:26:15 UTC
(In reply to comment #30)
> Since each property can start out multi-valued, the user cannot easily change a
> value back through the control, so a value-change check doesn't make sense.

I agree.

> I think a checkbox at each value would be the right thing -- definitely with a
> tooltip.  The user could then uncheck it to reset the property to its original
> (possibly multivalued) value.  The concept of the checkbox would be "set this
> property".

Only one thing to take care of --- the checkbox doesn't make sense when changing the properties of a single object. In that case, it becomes an unnecessary extra click for the user. It should be easy enough to hide the checkbox when the diaglog is created on a single object.

> I tried using your code to change the values of several selected objects, but
> only one object actually changed value.  I can give you a more precise
> description if you want.

The patch I provided is not supposed to work for most types of objects, since it depends on signals not yet implemented in the property widgets. Also, you've said "several selected objects" ... I didn't target that. The patch is for group properties only.

PS: We should go for the checkbox approach. But that means I don't need to worry about implementing signals in the property widgets. But anyway, I would still like to push the patch submitted elsewhere, for the dynamic_menu, since it makes that widget a bit cleaner.
Comment 32 Lars Clausen 2007-03-09 13:45:13 UTC
I just had a talk with our local usability expert about this issue (and more).  He suggested that we follow the standard of having a greyed-out selector when there is multiple values, and GTK supports this.  Only problem is that our home-grown selectors don't support it, so that'll take some work.  I suggest we use the checkbox for those widgets that don't have an "inconsistent" state (I think that's the word) until such time as it gets implemented.

He also agreed that using the intersection is the right start, though possibly extending with showing the "standard" settings (line color, line width, dashing etc) if at least one element uses them -- there's a slight problem there of how can the user tell the difference.  Another possible extension would be to have tabs of properties, where the first tab shows properties shared by all objects, the next tab shows it for some subset, etc.  That, too, would be further down the road.
Comment 33 Sameer D. Sahasrabuddhe 2007-03-11 06:44:48 UTC
(In reply to comment #32)
> use the checkbox for those widgets that don't have an "inconsistent" state (I
> think that's the word) until such time as it gets implemented.

Makes sense. I intend to tackle both tasks --- getting the properties dialog to work correctly for groups, and cleaning up the home-grown widgets. Cleaning includes making them behave like standard gtk+ widgets, and removing the deprecated API calls while at it.
Comment 34 Lars Clausen 2007-03-14 12:03:13 UTC
I tried out your patch, and it does indeed work for those properties it supports.  Nice job.  One thing that will need to be incorporated (sooner probably better than later) is undo -- currently undo sets all the objects to the same values.
Comment 35 Lars Clausen 2007-04-16 19:38:19 UTC
The patch is now included in the development code.  Undo would be a very nice thing to have, and I'm not closing the bug until we have it.
Comment 36 Lars Clausen 2007-04-16 19:47:37 UTC
Two important things:  We're now getting a *lot* of errors and warnings on startup, starting with 

(lt-dia:27060): GLib-GObject-WARNING **: gsignal.c:1249: signal "changed" already exists in the `GtkOptionMenu' class ancestry

(lt-dia:27060): GLib-CRITICAL **: g_strcasecmp: assertion `s1 != NULL' failed

(lt-dia:27060): Gtk-CRITICAL **: gtk_container_foreach: assertion `GTK_IS_CONTAINER (container)' failed

** (lt-dia:27060): CRITICAL **: gtk_wrap_box_set_aspect_ratio: assertion `GTK_IS_WRAP_BOX (wbox)' failed


Also, sheet selection now changes the menu immediately, but the sheet icons only change after the menu is selected again.

Both these things need fixing ASAP.  I'm including the code to allow more people to look at it and to make progress easier.  This must be fixed by 0.97.
Comment 37 Sameer D. Sahasrabuddhe 2007-04-18 07:42:34 UTC
Created attachment 86555 [details] [review]
corrections to the patch dynamic menu signal handling

The earlier patch for DiaDynamicMenu and its users had two memory leaks. It also declared default_handlers that were never defined. Fixed the leaks and removed the handlers in this patch.
Comment 38 Sameer D. Sahasrabuddhe 2007-04-18 07:50:08 UTC
(In reply to comment #35)
> The patch is now included in the development code.  Undo would be a very nice
> thing to have, and I'm not closing the bug until we have it.
> 

I have not looked at how undo works in Dia, and how to make it work correctly for group-wide properties. Any help will be greatly appreciated.

The bug is far from being closed --- there's lots to do. I had been unable to work on this for some time, and one thing is clear --- it's not worth it to have a stop-gap solution of checkboxes in the properties dialog. I would rather put whatever time I have into doing it the right way from the first time itself. Hence  I intend to go forward and fix all the signal handling code related to the properties dialog. Then we can also figure out how to support "inconsistent" states in the home-grown widgets.
Comment 39 Sameer D. Sahasrabuddhe 2007-04-18 07:51:52 UTC
> (lt-dia:27060): GLib-GObject-WARNING **: gsignal.c:1249: signal "changed"
> already exists in the `GtkOptionMenu' class ancestry

This seems to be for the DiaDynamicMenu widget. My mistake for choosing that name. Will either use the parent's "changed" signal or invent another name for that signal.

> Also, sheet selection now changes the menu immediately, but the sheet icons
> only change after the menu is selected again.
> 
> Both these things need fixing ASAP.  I'm including the code to allow more
> people to look at it and to make progress easier.  This must be fixed by 0.97.

Great.
Comment 40 Lars Clausen 2007-04-18 19:36:27 UTC
I've applied your patch from above.  The sheet selection menu seems to simply lag one selection behind, and there's still massive amounts of warnings.  I've started a page on live.gnome.org about the undo system.
Comment 41 Sameer D. Sahasrabuddhe 2007-04-19 05:03:01 UTC
Created attachment 86618 [details] [review]
signal renamed in DiaDynamicMenu, removes all warnings observed

This new patch includes the earlier patch about corrections. The signal "changed" was already present in GtkOptionMenu, that DiaDynamicMenu is derived from, which was causing all the headaches. This signal has been renamed to "value-changed". The "changed" signal cannot be reused since the behaviour of DynamicMenu is complicated and causes race conditions.

Also fixed DiaSizeSelector so that all the users of DiaDynamicMenu now work correctly and without warnings --- sheet menu, font style selector, color selector and arrow selector.

Now to cleanup the rest of the widgets. Note that the property dialog will not work, irrespective of groups or individual objects, until all property widgets can emit signals.
Comment 42 Lars Clausen 2007-04-19 05:32:00 UTC
Thank you!  I've applied this latest patch to SVN, it does appear to work for the props that have the signal.
Comment 43 Sameer D. Sahasrabuddhe 2007-07-12 04:38:38 UTC
Created attachment 91660 [details] [review]
simplified some widgets in the property dialog and all of them now have an event handler to track PXP_NOTSET

Got tired of waiting for feedback on the mailing list, for some specific changes I was making to one widget. Hence dumping it all here. I have no experience in splitting patches, else would have been glad to do that. If only we were using something like darcs instead of SVN!
Comment 44 Steffen Macke 2007-07-22 20:26:08 UTC
The 2007-07-12 patch applied fine for me. But is it supposed to allow changing of properties for multiple objects already? That doesn't seem to work for me yet.
Comment 45 Sameer D. Sahasrabuddhe 2007-07-23 05:09:29 UTC
No, the current work is only for a group, not multiple selected objects.
Comment 46 Lars Clausen 2007-07-24 20:53:31 UTC
That's definitely a lot easier to look at now.  Thanks!  Applied in SVN.  Now the unit spinner can reasonably be put on more things (corner radius, text padding, arrow head sizes, dash/dot length)...

But I can do that.  Getting undo to work with multi-object setting is more important.
Comment 47 Sameer D. Sahasrabuddhe 2007-07-30 11:18:18 UTC
Created attachment 92692 [details] [review]
implements undo for groups; but redo does not work; recursion not verified

Here's a basic implementation of undo for setting properties of multiple objects in a group. For some reason redo does not work; any pointers will be useful. Also have not checked yet with a group that contains a group. This particular beast also needs to be tested for the properties dialog!
Comment 48 Sameer D. Sahasrabuddhe 2007-08-01 06:59:02 UTC
Update: Further checking shows that the undo/redo works correctly for a group. But redo has problems when there is a group inside a group. Inside the group, the redo sets one value for each property on all objects. Thus if there was a red circle in the nested group and the border was changed to blue, there is a chance that the redo will set the border of all objects to red. Currently investigating this.
Comment 49 Sameer D. Sahasrabuddhe 2007-08-01 11:32:14 UTC
Created attachment 92845 [details] [review]
undo/redo in setting properties now works for grouped objects, even with nested groups

It is done. Undo/Redo now works in all cases, even when a group contains other groups. For this I had to modify the vtable for Objects, ie, struct _ObjectOps. 
The function object_apply_props() in lib/propobject.c must behave differently when called on a group object. Hence introduced this as a new function in the vtable as "ApplyObjectPropertiesFunc".
In order to avoid confusion, also renamed ApplyPropertiesFunc to ApplyPropertiesDialogFunc. All files under objects/ have been updated to reflect these changes in the vtable, hence the large size of this patch. The interesting stuff is happening only in a few files under app/ and lib/
Comment 50 Lars Clausen 2007-08-10 10:25:08 UTC
*That's* why it was bugging me that you're working on the groups rather than the current selection: The group system is currently somewhat broken and in the middle of being replaced (using #ifdefs for now, though).  See my notes on http://live.gnome.org/Dia/CurrentDevelopment.  Adding a new function to the vtable simply to apply properties in a group when we're planning to move that functionality outside of grouping is not good, especially since it breaks binary compatibility (that's why there are empty slots in the object_ops table).

I think it would be better to move to selection-based multi-properties now, before it gets even messier.  Sorry about not picking up on this earlier.
Comment 51 Sameer D. Sahasrabuddhe 2007-09-19 06:00:32 UTC
After a bit of soul searching, I am back. I believe this is still worth doing.

> *That's* why it was bugging me that you're working on the groups rather than
> the current selection: The group system is currently somewhat broken and in
> the middle of being replaced (using #ifdefs for now, though).

This is why it is worth doing. Because the new group system is a long way away, and a really important feature currently sucks in Dia. I have a fix for it.

> Adding a new function to the
> vtable simply to apply properties in a group

That's not a trivial feature. And we are not introducing something new. My changes actually fix a bad abstraction of a class method. Every class should have a method to apply properties to itself. Instead the current code wrongly uses a single global function to do this for all classes. Dia was not able to handle group properties because of this problem with the original interface described in struct _ObjectOps.

> when we're planning to move that
> functionality outside of grouping is not good, especially since it breaks
> binary compatibility (that's why there are empty slots in the object_ops
> table).

I suppose it would be ok if I moved the new function to a reserved slot then. A couple of things that come to mind. Since this new function actually fixes a broken interface, it should not be considered optional, even for new code. I would vote for breaking binary compatibility just to force everyone to take note of this new "fix". Exactly what will be the result of breaking compatibility? Given that Dia is still below 1.0, is it too important?

> I think it would be better to move to selection-based multi-properties now,
> before it gets even messier.  Sorry about not picking up on this earlier.

It shouldn't get any messier. I think waiting is a bad idea in this case.
Comment 52 Hans Breuer 2007-09-20 21:50:59 UTC
As noted in http://mail.gnome.org/archives/dia-list/2007-August/msg00026.html 

""" Just looked at the lst patch. It is changing every objects 'vtable' and
thus breaking binary compatibility. If necessary at all it should be added
at the end of the vtable (see objects.h struct _ObjectOps) having only 4
unused left. Also one would need to implement a default handling for case
where the objects function pointer is NULL. """

I'd like to get the functionality in before we may have the all new group object *some* day. But the placement in a new slot (rather than changing exisiting ones) is crucial not only for binary compatibility, but also for better source code maintenance. With a true OO language the compiler would match the correct signature to the correct 'slot'. But with C and also the function pointer casts we would produce very hard to detect bugs if we change function positions in the 'vtable'.
Comment 53 Lars Clausen 2007-09-23 18:14:04 UTC
I agree now that it should go into the current system, and not be held up by the new groups.  I do agree with Hans, though, that using a reserved slot is the way to go, both to avoid messing with too much code and to keep binary compatibility.  Otherwise, your patch looks ok.
Comment 54 Sameer D. Sahasrabuddhe 2007-10-02 06:33:12 UTC
Created attachment 96497 [details] [review]
retrofitting the vtable with a new function to handle undo, especially when changing group properties


Updated all the objects to use the first unused slot in the vtable. Also changed a few names for the sake of readability assuming that will remain transparent in terms of binary compatibility! Especially a new typedef for "ReservedSlotFunc" that helps in type-casting when initialising the vtable.

Note that I have not delved into newgroup.c much ... only inserted a warning there, with a null value for the relevant function.
Comment 55 Sameer D. Sahasrabuddhe 2007-10-02 06:58:12 UTC
Created attachment 96498 [details] [review]
retrofitting the vtable with a new function to handle undo, especially when changing group properties

Slightly cleaner version of the same patch ... forgot to dump a diff after compiling and testing it.
Comment 56 Lars Clausen 2007-10-06 16:27:13 UTC
There's a couple problems with this patch yet, but it's getting better.

You should not use an unused slot in the sense of unused[0], but make a new field and reduce the number of unused slots to four.  Also, you don't need to add initialization for the unused slots, as long as there's a comma at the end.

Also, rather than having an assert on obj->ops->apply_properties_list, we should just call the default function in case of NULL; otherwise, old binaries will cause assertions by default rather than just working nicely.  If the default function is called on NULL, we don't need to insert it in all the other objects.

I don't understand the note in object.h:

    IMPORTANT: In this case, all code should fail if this op is not
    supported, since there is no sane default behaviour that can handle
    undo/redo when applying properties.

Isn't object_apply_props exactly the behaviour desired for all other objects than Group, given that that used to be the behaviour anyway?  Why can't we assume that if the function field is NULL, we can call this function?

I'm not yet convinced that adding a new object op is really necessary for doing this.  Can you give an example of why any other object than the group object would want to not use the default?  There is already an IS_GROUP macro in use, why not keep the old function and have it do a special case for groups?  I think it'd be easier to figure out what's going on that way.

Passing the object to the object change functions may be a bit outdated, the TextChange doesn't have the object, but it doesn't need it either.  It's probably something that could be fixed.
Comment 57 Sameer D. Sahasrabuddhe 2007-10-07 03:31:24 UTC
> You should not use an unused slot in the sense of unused[0], but make a new
> field and reduce the number of unused slots to four.  Also, you don't need to
> add initialization for the unused slots, as long as there's a comma at the end.

Given the talk about binary compatibility, I decided to choose the most stable approach of doing this, since I have zero experience in such work. I can change this to reduce the number of slots, although personally I still think the best way to do this is to just forget binary compatibility and introduce a new function.

> Also, rather than having an assert on obj->ops->apply_properties_list, we
> should just call the default function in case of NULL; otherwise, old binaries
> will cause assertions by default rather than just working nicely.  If the
> default function is called on NULL, we don't need to insert it in all the other
> objects.

The problem is that supporting undo when applying properties is fundamental. It either works or doesn't work, there is no "work nicely". About the need to insert it in all objects, one could say that for all the functions in the vtable! The only reason I am checking the pointer here is because of the fear of the unknown in terms of backward compatibility ... rather than crashing, the code will at least tell us if some old unmaintained object is missing this function. Again, if we didn't care about binary compatibility this check would not be necessary because the missing function would cause compile time errors instead.

>     IMPORTANT: In this case, all code should fail if this op is not
>     supported, since there is no sane default behaviour that can handle
>     undo/redo when applying properties.

This is simply meant as a warning for people who write new code ... they should not overlook unused[0]. But once I rearrange the unused array itself, this warning can go away.

But I'll ask this once, just to clarify matter --- should I reduce unused[] and insert a new function, or just totally redo the vtable and ignore unused[]? I am not asking this because I like to be a pain in the ass, but only because I totally believe that the second option is the Right Thing to do. Personally, I would like to just remove unused[] from the vtable.

> I'm not yet convinced that adding a new object op is really necessary for doing
> this.  Can you give an example of why any other object than the group object
> would want to not use the default?  There is already an IS_GROUP macro in use,
> why not keep the old function and have it do a special case for groups?  I
> think it'd be easier to figure out what's going on that way.

I must admit, the reason I initially did this was simply because I don't like special cases ... what's the point of emulating object oriented behaviour in C if we don't go the whole way? But only this weekend, I discovered this bug:

Undo does not restore size of automatically resized shapes
http://bugzilla.gnome.org/show_bug.cgi?id=318182

The perfect way to solve this is that any object that automatically changes other properties as a side effect should create its own composite Change object.

> Passing the object to the object change functions may be a bit outdated, the
> TextChange doesn't have the object, but it doesn't need it either.  It's
> probably something that could be fixed.

I think this refers to the comments I inserted in unrelated places in the same patch. I admit asking questions in code comments is not very good style ... I'll instead flag this with a FIXME saying this is deprecated behaviour, and should be cleaned up eventually.
Comment 58 Hans Breuer 2007-10-07 10:51:20 UTC
Sameer: I'm not questioning that you need to "add a new function" to the vtable, but apparently we made not yet clear how this is done while preserving binary and source compatibility. I tried in comment #52, you may also want to look into http://svn.gnome.org/viewcvs/dia/trunk/lib/object.h?r1=3029&r2=3028&pathrev=3029 which is at least done correctly on the technical level.

1) declare a new function pointer
2) add it at the after the last used slot - this will give 0 initialization for all vtables not implementing it, without the need to change anything in the object implementations (source compatibility).
3) reduce the size of the 'unused' array added for padding, thus the overall size of the vtable stays the same for all object vtables (binary compatibility)
4) use the new function by obj->ops->new_fun(). Before accessing it the function pointer should be checked for !=NULL. If it is an optional member function a fallback should be implmented. If it is a required function a g_warning would be appropriate. (True object oriented design does not allow the function pointer being NULL, so there should be always a default implementation. But there is no mechanism in C to enforce this, like it is in C++ where you can not instantiate a derived object without implementing all pure virtual functions.)

Comment 59 Sameer D. Sahasrabuddhe 2007-10-16 12:47:22 UTC
Created attachment 97273 [details] [review]
apply_properties_list() makes correct use of an unused slot; assertion changed to a warning

Here's the latest version. Fixed everything according to the comments so far. This should be good to go now!
Comment 60 Hans Breuer 2007-10-20 12:08:47 UTC
Indeed, just committed. The only thing missing seems to be the handling for multiple selection not just the group object. Thanks!

2007-10-20  Hans Breuer  <hans@breuer.org>

	* app/properties.c lib/group.c lib/newgroup.c lib/objchange.h
	  lib/object.h lib/propobject.c objects/AADL/aadlbox.c
	  objects/AADL/aadlbus.c objects/AADL/aadldata.c
	  objects/AADL/aadldevice.c objects/AADL/aadlmemory.c 
	  objects/AADL/aadlpackage.c objects/AADL/aadlprocess.c
	  objects/AADL/aadlprocessor.c objects/AADL/aadlsubprogram.c
	  objects/AADL/aadlsystem.c objects/AADL/aadlthread.c
	  objects/AADL/aadlthreadgroup.c objects/chronogram/chronoline.c
	  objects/chronogram/chronoref.c objects/custom/custom_object.c
	  objects/EML/instantiation.c objects/EML/interaction-ortho.c
	  objects/EML/interaction.c objects/EML/process.c 
	  objects/ER/attribute.c objects/ER/entity.c 
	  objects/ER/participation.c objects/ER/relationship.c 
	  objects/flowchart/box.c objects/flowchart/diamond.c
	  objects/flowchart/ellipse.c objects/flowchart/parallelogram.c
	  objects/FS/flow-ortho.c objects/FS/flow-poly.c objects/FS/flow.c
	  objects/FS/function.c objects/GRAFCET/action.c 
	  objects/GRAFCET/condition.c objects/GRAFCET/step.c
	  objects/GRAFCET/transition.c objects/GRAFCET/vector.c
	  objects/GRAFCET/vergent.c objects/Istar/actor.c objects/Istar/goal.c
	  objects/Istar/link.c objects/Istar/other.c objects/Jackson/domain.c
	  objects/Jackson/phenomenon.c objects/Jackson/requirement.c
	  objects/KAOS/goal.c objects/KAOS/metaandorrel.c 
	  objects/KAOS/metabinrel.c objects/KAOS/other.c
	  objects/Misc/analog_clock.c objects/network/basestation.c
	  objects/network/bus.c objects/network/radiocell.c
	  objects/network/wanlink.c objects/SADT/annotation.c
	  objects/SADT/arrow.c objects/SADT/box.c objects/SISSI/area.c
	  objects/SISSI/faraday.c objects/SISSI/room.c 
	  objects/SISSI/sissi_object.c objects/SISSI/site.c
	  objects/standard/arc.c objects/standard/bezier.c
	  objects/standard/beziergon.c objects/standard/box.c
	  objects/standard/ellipse.c objects/standard/image.c
	  objects/standard/line.c objects/standard/polygon.c
	  objects/standard/polyline.c objects/standard/textobj.c
	  objects/standard/zigzagline.c objects/UML/activity.c
	  objects/UML/actor.c objects/UML/association.c objects/UML/branch.c
	  objects/UML/class.c objects/UML/classicon.c objects/UML/component.c
	  objects/UML/component_feature.c objects/UML/constraint.c
	  objects/UML/dependency.c objects/UML/fork.c
	  objects/UML/generalization.c objects/UML/implements.c
	  objects/UML/large_package.c objects/UML/lifeline.c 
	  objects/UML/message.c objects/UML/node.c objects/UML/note.c
	  objects/UML/object.c objects/UML/realizes.c
	  objects/UML/small_package.c objects/UML/state.c
	  objects/UML/state_term.c objects/UML/transition.c 
	  objects/UML/usecase.c : finally there is proper group properties
	handling thanks to patches from Sameer D. Sahasrabuddhe. Almost fixes 
	long-standing bug #60331.

Comment 61 Tim Abell 2007-10-25 14:52:36 UTC
I came across this bug report as I was simply trying to change the background colour of multiple objects. For the benefit of anyone else with this problem, I though I'd note what I did that worked (presumably thanks to Sameer's hard work). I am on version 0.96.1 on Windows XP.

Although you can't select multiple objects and directly apply a background colour, you can select multiple objects, group them, set a background colour, then ungroup them. Yay.
Comment 62 Lars Clausen 2007-10-26 17:49:54 UTC
Tim, what you'll notice if you keep doing that in 0.96.1 is that you'll change all properties of all the objects, not just the ones you've modified.  So if you try to make a blue line and a red line both be thicker that way, both will end up with the same color. 
Comment 63 Tim Abell 2007-10-27 00:52:59 UTC
Lars, Cheers for the heads up. I had guessed this would be the case from reading the above discussion :-) Fortunately in my case all the other properties were the same anyway.
Comment 64 Hans Breuer 2008-04-06 16:44:53 UTC
*** Bug 526284 has been marked as a duplicate of this bug. ***
Comment 65 Hans Breuer 2008-04-08 19:40:03 UTC
*** Bug 526906 has been marked as a duplicate of this bug. ***
Comment 66 Sameer D. Sahasrabuddhe 2008-11-23 04:12:06 UTC
*** Bug 538617 has been marked as a duplicate of this bug. ***
Comment 67 Sameer D. Sahasrabuddhe 2008-12-03 17:33:41 UTC
Created attachment 123886 [details] [review]
introduce a separate function to return property descriptions for a list of objects

Bulk of the function group_describe_props is actually a more general operation of getting the property descriptions for a list of objects. Moved that into a separate function. Also introduced a flag to choose whether a union or intersection is desired of all the available properties.
Comment 68 Sameer D. Sahasrabuddhe 2008-12-07 11:05:56 UTC
Created attachment 124086 [details] [review]
the properties dialog now tracks multiple objects

With this patch along with the one before it, it is possible to select multiple objects and modify their properties from a single dialog box. In particular _PropDialog in lib/properties.h was modified: the DiaObject* fields orig_obj and obj_copy are replaced by equivalent GList*. This is accompanied by various wrapper functions for handling lists of objects in appropriate places.
Comment 69 Hans Breuer 2008-12-07 23:03:17 UTC
Hi Sameer, I've commited your patches now. Unfortunately the second regression introduced isn't as easy to fix as I thought. At the moment the special dialogs from "UML - CLass" and e.g. Sissi and "Database - Table" are not accesible at all.
Probably my commit was a bit too early ;) Should I revert or do you have a fix?

2008-12-07  Hans Breuer  <hans@breuer.org>

	* app/commands.c app/diagram_tree.c app/modify_tool.c app/properties.c
	  app/properties.h lib/group.c lib/object.h lib/propdesc.c 
	  lib/propdialogs.c lib/properties.h lib/propinternals.h
	  lib/propobject.c : : patch from Sameer Sahasrabuddhe almost fixing
	Dia's oldest bug #60331 - Object properties should handle multiple 
	objects (but it breaks with UM - Class)
	* lib/libdia.def : updated

	* objects/UML/class.c : don't crash when 
	umlclass_apply_props_from_dialog() get callesd without 
	umlclass_get_properties(), instead fall back to 
	object_apply_props_from_dialog() than

Comment 70 Sameer D. Sahasrabuddhe 2008-12-08 02:54:11 UTC
Oops. My mistake for not thoroughly checking for regressions. Didn't check those special dialogs in my eagerness to put up a patch! I don't know what the problem is exactly, since I never even anticipated it. I think you should revert, unless _you_ have a fix :) . I'll poke around a bit more.
Comment 71 Hans Breuer 2008-12-08 08:32:01 UTC
Commited a fix - needs some more review/testing, though.

2008-12-08  Hans Breuer  <hans@breuer.org>

	* objects/UML/class.c : don't crash when 
	umlclass_apply_props_from_dialog() get callesd without 
	umlclass_get_properties(), instead fall back to 
	object_apply_props_from_dialog() than
	* objects/Database/table.c : same here
	
	* app/properties.c : for a single object let Object::get_properties()
	create the "object part" of the property dialog
Comment 72 Sameer D. Sahasrabuddhe 2008-12-09 09:16:42 UTC
Created attachment 124250 [details] [review]
updated the vtable for Database/table.c to use the correct function

In the fix from Hans, the vtable in Database/table.c was not updated. Fixed that to use the correct ApplyPropertiesDialogFunc ... _table_dialog_apply_changes(). Also added a small comment in app/properties.c to improve readability.

This is finally it, I think. Both special classes seem to work now.
Comment 73 Hans Breuer 2008-12-09 20:03:15 UTC
Thanks, again. There probably is an issue remaining with objects/SISSI, but it crashed not where I expected;)
Comment 74 Hans Breuer 2009-01-09 22:55:09 UTC
Sameer, if you don't know remaining issues I think we can finally 
close this bug ;) :

2009-01-09  Hans Breuer  <hans@breuer.org>

	* lib/propobject.c(object_list_get_propdescriptions) : for single 
	object in the list use the "intersection" because otherwise props
	with PROP_FLAG_DONT_MERGE would not be accessible anymore, e.g. 
	for "Standard - Outline" there was no way to change the text anymore
	(hopefully final regression introduced by fixing bug #60331)

Comment 75 Sameer D. Sahasrabuddhe 2009-01-10 00:37:35 UTC
With reference to comment 73, the fix mentioned in comment 71 also needs to be applied for SISSI objects and one more dialog that I can't remember right now. While trying to use SISSI objects I encountered the crasher bug that I had to fix in a recent patch for the create_object tool.

Also one more issue, but I think it should be filed as a separate bug: when one or more UML class objects are selected, the dialog shown should be the the one for UML class objects and not the generic dialog. Same applies for other special dialogs. The fix is to modify all the special dialog to handle lists of objects, just like the generic dialog, and then examine the currently selected objects in object_list_properties_show(), in app/properties-dialog.c
Comment 76 Hans Breuer 2009-01-13 09:02:20 UTC
changing dedicated object dialog implementations to be multi-object aware is definitely a new feature - I'm not even sure we should do. 
So the remaining things here are fixing objects/SISSI and the "one more dialog that I can't remember". 
Comment 77 Hans Breuer 2009-01-25 20:50:49 UTC
Should be settled now:

2009-01-24  Hans Breuer  <hans@breuer.org>

	* objects/SISSI/area.c objects/SISSI/faraday.c objects/SISSI/room.c
	  objects/SISSI/sissi.[ch] objects/SISSI/sissi_dialog.c
	  objects/SISSI/sissi_object.c objects/SISSI/site.c : fixed object
	method DiaObject::apply_props_from_dialog() for the last known issue
	with changing multiple objects properties, bug #60331
	(It did not crash because the SISSI object don't contribute anything
	to the property list.)
	* objects/SISSI/sissi_object.c(280) : local variable 'filename' used
	without having been initialized
	* objects/SISSI/sissi_dialog.c(380) : incompatible types - from 
	'struct _GtkWidget *' to 'struct _GtkScrolledWindow *'

Comment 78 Steffen Macke 2010-01-14 16:32:56 UTC
*** Bug 606943 has been marked as a duplicate of this bug. ***