Signal editor rework

[Johannes Schmid]

Hi!

Code: http://git.gnome.org/browse/glade3/log/?h=signal-tree-model

Interesting parts:
*
http://git.gnome.org/browse/glade3/tree/gladeui/glade-signal-editor.c?h=signal-tree-model
*
http://git.gnome.org/browse/glade3/tree/gladeui/glade-signal-model.c?h=signal-tree-model

Why?

My starting idea was to be able to drag signal handlers from glade into
anjuta and automatically creating the prototypes. Some may have seen my
GUADEC lightning talk on that issue.

So I looked at the SignalEditor and that was a big monolitic block with
many custom cell rendering methods with a backend based on a GtkTreeStore.
The GtkTreeStore meant that updating things was rather painful and that
the data structure of the GladeWidget was duplicated and had to be kept up
to date with the widget.

So my idea was to use a custom GtkTreeModel instead that would use the
signal structure of the GladeWidget directly. I though about implementing
directly in glade-widget.c but realized pretty soon that this would bloat
that file two much and that it wouldn't be obvious that a GladeWidget
implements a GtkTreeModel.

As such, I started on GladeSignalModel which is basically initialised
using a GladeWidget and displays it's data. In the model/view-concept this
holds the signal data which is taken from the GladeWidget directly without
duplicating it.

This model implements the drag and drop stuff based on GtkTreeDragSource.
The most complicated thing in the tree model is that we need to take care
of dummy nodes that a used to enable the user to add new signals.

The implementation of the GladeSignalEditor on top of that was trivial then.

Status

Everything is feature complete compared to the old GladeSignalEditor,
minus the deprecation warnings and the devhelp button. These are both not
difficult to add I just didn't come to it, yet.

What needs to be improved?

I don't like the way the GtkCellRendererCombo works for the object
selection with a real tree (not a list). I think that isn't convenient yet
and needs a better solution.

Some horizontal space could be saved by using the "Name" column for both,
object names and signal-names instead of a additional "Handler" column.
This is quite easy to do.

Conclusion:
While I didn't save as much code as I wanted to (mainly because of the
GObject template code necessary) I think the implementation is much
cleaner than the code before.

Regards,
Johannes

_______________________________________________
Glade-devel maillist - Glade-***@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/glade-devel

[Tristan Van Berkom]

Post by Johannes Schmid
Hi!
Code: http://git.gnome.org/browse/glade3/log/?h=signal-tree-model
*
http://git.gnome.org/browse/glade3/tree/gladeui/glade-signal-editor.c?h=signal-tree-model
*
http://git.gnome.org/browse/glade3/tree/gladeui/glade-signal-model.c?h=signal-tree-model

Hi Johannes !
I am sorry I did not give your branch more attention thus far, I'm
giving it a run down
right now and try to get the ball rolling again...

So here are some points I can spot so far...

- Firstly I can see you went and implemented the GtkTreeIter->stamp a bit more
robustly than in GladeProject, additionally the code needs to assert for
any input iter: (iter->stamp == model->priv->stamp). I've already
done this for
GladeProject and will commit that soon (it helped alot to debug...
unfortunately
I could still not completely solve bug 627078 and have not
committed that yet,
I'll commit the checking code now regardless).

- Can you explain why the model->priv->stamp must be incremented
when the overall
model data changes ? (it could be that GladeProject needs this
treatment too... or
that the model's stamp should not change... I'm not sure).

- The iter->stamp should be initialized to something random,
otherwise every model
starts with stamp == 0 (possibly making the iter from the last
destroyed signal model
appear "valid")

- I would prefer we stick with firing Glade's generic object
selection dialog to select
the project object for a signal user data - I dont like the idea
of navigating recursive menus
or dropping down a combo box with a possible > 1000 objects to select.

I have to compile and run the code to say more, but my initial
feelings are that we
really need to stamp those iters and check the iters on every model input.

Cheers,
-Tristan

Post by Johannes Schmid
Why?
My starting idea was to be able to drag signal handlers from glade into
anjuta and automatically creating the prototypes. Some may have seen my
GUADEC lightning talk on that issue.
So I looked at the SignalEditor and that was a big monolitic block with
many custom cell rendering methods with a backend based on a GtkTreeStore.
The GtkTreeStore meant that updating things was rather painful and that
the data structure of the GladeWidget was duplicated and had to be kept up
to date with the widget.
So my idea was to use a custom GtkTreeModel instead that would use the
signal structure of the GladeWidget directly. I though about implementing
directly in glade-widget.c but realized pretty soon that this would bloat
that file two much and that it wouldn't be obvious that a GladeWidget
implements a GtkTreeModel.
As such, I started on GladeSignalModel which is basically initialised
using a GladeWidget and displays it's data. In the model/view-concept this
holds the signal data which is taken from the GladeWidget directly without
duplicating it.
This model implements the drag and drop stuff based on GtkTreeDragSource.
The most complicated thing in the tree model is that we need to take care
of dummy nodes that a used to enable the user to add new signals.
The implementation of the GladeSignalEditor on top of that was trivial then.
Status
Everything is feature complete compared to the old GladeSignalEditor,
minus the deprecation warnings and the devhelp button. These are both not
difficult to add I just didn't come to it, yet.
What needs to be improved?
I don't like the way the GtkCellRendererCombo works for the object
selection with a real tree (not a list). I think that isn't convenient yet
and needs a better solution.
Some horizontal space could be saved by using the "Name" column for both,
object names and signal-names instead of a additional "Handler" column.
This is quite easy to do.
While I didn't save as much code as I wanted to (mainly because of the
GObject template code necessary) I think the implementation is much
cleaner than the code before.
Regards,
Johannes
_______________________________________________
http://lists.ximian.com/mailman/listinfo/glade-devel

_______________________________________________
Glade-devel maillist - Glade-***@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/glade-devel

[Johannes Schmid]

Hi Tristan!

Post by Tristan Van Berkom
- Firstly I can see you went and implemented the GtkTreeIter->stamp a bit more
robustly than in GladeProject, additionally the code needs to assert for
any input iter: (iter->stamp == model->priv->stamp). I've already
done this for
GladeProject and will commit that soon (it helped alot to debug...
unfortunately
I could still not completely solve bug 627078 and have not
committed that yet,
I'll commit the checking code now regardless).

I filed bug https://bugzilla.gnome.org/show_bug.cgi?id=623879 for that
some time ago to also fix it in GladeProject. But you might be right that
we might want to check it with an assertion.

Post by Tristan Van Berkom
- Can you explain why the model->priv->stamp must be incremented
when the overall
model data changes ? (it could be that GladeProject needs this
treatment too... or
that the model's stamp should not change... I'm not sure).

IMHO the gtk+ documentation says that an iter is only valid as long as the
model doesn't change. If you can assure that the iter is valid after a
change you might not need it but when a node is deleted, the iter of that
node definitly becomes invalid. I think it's good to catch those things
with an assertion before running into a memory corruption.

Post by Tristan Van Berkom
- The iter->stamp should be initialized to something random,
otherwise every model
starts with stamp == 0 (possibly making the iter from the last
destroyed signal model
appear "valid")

I wasn't assuming that someone would use an iter on a wrong model but if
we want to catch that case we might want to start which a random number. I
think we should check what GtkTreeStore does.

Post by Tristan Van Berkom
- I would prefer we stick with firing Glade's generic object
selection dialog to select
the project object for a signal user data - I dont like the idea
of navigating recursive menus
or dropping down a combo box with a possible > 1000 objects to select.

Yeah, I will change that. I just was easy to implement for now but the UI
is satisfying at all.

Just some additional note: The dnd code currently uses the old gdk_drawing
funtions that have been removed from gtk+. I didn't have the time to
change that as I didn't fully understood how to do that in cairo. But it
shouldn't be a big deal.

Regards,
Johannes

_______________________________________________
Glade-devel maillist - Glade-***@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/glade-devel

[Tristan Van Berkom]

Post by Johannes Schmid
Hi Tristan!

  - Firstly I can see you went and implemented the GtkTreeIter->stamp a
bit more
    robustly than in GladeProject, additionally the code needs to assert
for
    any input iter: (iter->stamp == model->priv->stamp). I've already
done this for
    GladeProject and will commit that soon (it helped alot to debug...
unfortunately
    I could still not completely solve bug 627078 and have not
committed that yet,
    I'll commit the checking code now regardless).

I filed bug https://bugzilla.gnome.org/show_bug.cgi?id=623879 for that
some time ago to also fix it in GladeProject. But you might be right that
we might want to check it with an assertion.

  - Can you explain why the model->priv->stamp must be incremented
when the overall
    model data changes ? (it could be that GladeProject needs this
treatment too... or
    that the model's stamp should not change... I'm not sure).

IMHO the gtk+ documentation says that an iter is only valid as long as the
model doesn't change. If you can assure that the iter is valid after a
change you might not need it but when a node is deleted, the iter of that
node definitly becomes invalid. I think it's good to catch those things
with an assertion before running into a memory corruption.

Ok that makes perfect sense to me. Sorry I missed the stamp fix
for GladeProject, I'm going to go over your patch and make sure
I didnt miss anything (for instance I at least missed the incrementing
of the stamp when the project tree changes...).

Post by Johannes Schmid

  - The iter->stamp should be initialized to something random,
otherwise every model
    starts with stamp == 0 (possibly making the iter from the last
destroyed signal model
    appear "valid")

I wasn't assuming that someone would use an iter on a wrong model but if
we want to catch that case we might want to start which a random number. I
think we should check what GtkTreeStore does.

I guess it is at least important that the initial stamp is not 0, since
0 is what we generally use to represent an invalid iter (when
the iter to return was not found for instance).

Actually I was just guessing at why treestore initializes its stamp
to a non-zero random number... I can vouch that treestore (or liststore that
I checked... not sure) does this but I'm not exactly sure why.

Post by Johannes Schmid

  - I would prefer we stick with firing Glade's generic object
selection dialog to select
    the project object for a signal user data - I dont like the idea
of navigating recursive menus
    or dropping down a combo box with a possible > 1000 objects to select.

Yeah, I will change that. I just was easy to implement for now but the UI
is satisfying at all.
Just some additional note: The dnd code currently uses the old gdk_drawing
funtions that have been removed from gtk+. I didn't have the time to
change that as I didn't fully understood how to do that in cairo. But it
shouldn't be a big deal.

Extra note... I went ahead and copied out some GTK+ internals:
glade_utils_cairo_draw_rectangle(), glade_utils_cairo_draw_line()

... in the case they may be helpful to you... also feel free to add some
convenience drawing functions to glade-utils.[ch] if you think that makes
sense for what needs drawing.

Cheers,
-Tristan
_______________________________________________
Glade-devel maillist - Glade-***@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/glade-devel