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 520778 - [PATCH] GNode wrapper implementation
[PATCH] GNode wrapper implementation
Status: RESOLVED FIXED
Product: glibmm
Classification: Bindings
Component: general
2.15.x
Other All
: Normal enhancement
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2008-03-06 16:31 UTC by Levi Bard
Modified: 2008-07-29 09:03 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
Patch (22.83 KB, patch)
2008-03-06 16:32 UTC, Levi Bard
none Details | Review
Updated per comments (22.61 KB, patch)
2008-03-06 20:42 UTC, Levi Bard
none Details | Review
Added reinterpret_cast to Tree::*sibling (23.33 KB, patch)
2008-03-18 12:00 UTC, Levi Bard
none Details | Review
Updated per comment #6 (25.01 KB, patch)
2008-03-24 15:49 UTC, Levi Bard
none Details | Review
Patch against trunk (24.28 KB, patch)
2008-03-25 13:52 UTC, Levi Bard
none Details | Review
suggested changes to the wrapper (20.35 KB, patch)
2008-07-22 21:32 UTC, Szilárd Pfeiffer
none Details | Review
example to comment 18 (4.44 KB, text/x-c++src)
2008-07-24 13:48 UTC, Szilárd Pfeiffer
  Details
Cleaned version of suggested changes (18.27 KB, patch)
2008-07-27 21:24 UTC, Szilárd Pfeiffer
none Details | Review

Description Levi Bard 2008-03-06 16:31:36 UTC
glibmm wrapper for glib's GNode.
Comment 1 Levi Bard 2008-03-06 16:32:06 UTC
Created attachment 106695 [details] [review]
Patch
Comment 2 Murray Cumming 2008-03-06 16:50:25 UTC
I've always been surprised that standard C++ provides no tree container. I wonder if boost or similar has one already.

Looking at this quickly, I see some superficial problems:
- Don't indent inside the namespace, to be consistent with the rest of glibmm.
- Use "Something& " instead of "Something &".
- You should use a libsigc++ slot instead of a plain C function pointer.
- Your is/get methods should be const.
- All member variables should have the _ suffix.
- Some method parameters should be const. For instance, position_of(Tree &child).
- Tree(gpointer data) should probably use the "explicit" keyword.
- There's probably no reason for the destructor to be virtual - there are no virtual methods.
- We can should probably use void* instead of gpointer in our public API.
- Please avoid use of "using", even in tests.
Comment 3 Levi Bard 2008-03-06 20:42:26 UTC
Created attachment 106714 [details] [review]
Updated per comments
Comment 4 Levi Bard 2008-03-18 12:00:44 UTC
Created attachment 107524 [details] [review]
Added reinterpret_cast to Tree::*sibling
Comment 5 Jonathon Jongsma 2008-03-18 16:11:28 UTC
I've been meaning to take a look at this patch, but haven't gotten around to it yet.  I promise I'll try to do it soon.  Just wanted to let you know that it's not going to be ignored :)
Comment 6 Murray Cumming 2008-03-22 09:44:36 UTC
I don't much like that this container seems to demand that types are used by pointer. For instance, from the test from the patch:

std::string *a = new std::string("a"),
            *b = new std::string("b"),
            *c = new std::string("c"),
            *d = new std::string("d"),
            *e = new std::string("e"),
            *f = new std::string("f");

Glib::Tree<std::string*> ta(a),
  tb(b),
  tc(c),
  te(e);

ta.insert(0, tc);
ta.prepend(tb);
ta.append_data(d);
tc.append(te);
te.prepend_data(f);

If we can't make it work with values, like std::list, std::vector, etc, then we need a really good reason for that. Otherwise people will feel that it is not really C++ enough.
Comment 7 Levi Bard 2008-03-24 15:49:37 UTC
Created attachment 107926 [details] [review]
Updated per comment #6

This will require value types to have a working operator=. Hopefully this isn't too high a price to pay for more C++iness.
Comment 8 Murray Cumming 2008-03-24 19:33:37 UTC
Oh, that's much nicer.

> This will require value types to have a working operator=.

Just as a std container would.
Comment 9 Jonathon Jongsma 2008-03-25 01:25:07 UTC
Is this patch against svn?  I get several failures when I try to apply it.
Comment 10 Levi Bard 2008-03-25 13:52:01 UTC
Created attachment 107992 [details] [review]
Patch against trunk

Sorry, I thought I was working against trunk, but I actually had the 2.14 branch.  Latest patch is against trunk.
Comment 11 Murray Cumming 2008-06-13 15:52:14 UTC
Committed, with some minor documentation changes, and a couple of minor TODOs. Thanks.

Please remember to patch the ChangeLog in future.
Comment 12 Szilárd Pfeiffer 2008-07-22 21:32:48 UTC
Created attachment 115050 [details] [review]
suggested changes to the wrapper
Comment 13 Szilárd Pfeiffer 2008-07-22 21:46:31 UTC
I would like to suggest some changes in the implementation of the wrapper (comment 12). I can see the following benefits:

1; Glib::Node class would be closer to the original GNode structure, so it would be simpler
2; Foreach or traverse slots should use the tree node instead of the node's data only
3; Data would be compared by the operator == of class T instead of pointer comparison in case of find functions

 
Comment 14 Murray Cumming 2008-07-23 07:17:14 UTC
(In reply to comment #13)
> I would like to suggest some changes in the implementation of the wrapper
> (comment 12). I can see the following benefits:
> 
> 1; Glib::Node

You mean Glib::Tree, I guess.

> class would be closer to the original GNode structure, so it
> would be simpler
> 2; Foreach or traverse slots should use the tree node instead of the node's
> data only

What would be the advantage of this?

> 3; Data would be compared by the operator == of class T instead of pointer
> comparison in case of find functions

The patch seems to have lots of unrelated changes, which makes it difficult to review.


Comment 15 Levi Bard 2008-07-23 12:51:03 UTC
(In reply to comment #13)
> 1; Glib::Node class would be closer to the original GNode structure, so it
> would be simpler

Are you suggesting renaming Tree to Node?

> 3; Data would be compared by the operator == of class T instead of pointer
> comparison in case of find functions

Data is already compared using the == operator.
Comment 16 Szilárd Pfeiffer 2008-07-23 14:45:48 UTC
(In reply to comment #14)
> (In reply to comment #13)
> > I would like to suggest some changes in the implementation of the wrapper
> > (comment 12). I can see the following benefits:
> > 
> > 1; Glib::Node
> 
> You mean Glib::Tree, I guess.

Yes, you are right, otherwise it would be lovely not to name the wrapper of GNode to Glib::Tree especially because there is balanced tree implementatation in Glib with a structure name GTree.

> 
> > class would be closer to the original GNode structure, so it
> > would be simpler
> > 2; Foreach or traverse slots should use the tree node instead of the node's
> > data only
> 
> What would be the advantage of this?

In case of the original version (GNode) a GNode * is passed to the callback function, which makes possible to examine the location of the given node in the complete tree (e.g.: depth, index). In case of an own Gtk::TreeModel implementation it is necessary.

> 
> > 3; Data would be compared by the operator == of class T instead of pointer
> > comparison in case of find functions
> 
> The patch seems to have lots of unrelated changes, which makes it difficult to
> review.
> 

Comment 17 Szilárd Pfeiffer 2008-07-23 14:56:06 UTC
(In reply to comment #15)
> (In reply to comment #13)
> > 1; Glib::Node class would be closer to the original GNode structure, so it
> > would be simpler
> 
> Are you suggesting renaming Tree to Node?

Yes. Sure. (comment 16)

> 
> > 3; Data would be compared by the operator == of class T instead of pointer
> > comparison in case of find functions
> 
> Data is already compared using the == operator.
> 

You are right. It is mistake of mine, but with the suggested modifications it is unnecessary to make a copy from the data and delete it in the destructor.

  explicit Tree(T& data)
  {
    T* tmp = new T();
    *tmp = data;
    ...

Additionally in case of this kind of implementation the class T must have a constructor with no parameters and you cannot use the Glib::Tree with reference types of data. For example:

Glib::Tree<std::string &>
Comment 18 Murray Cumming 2008-07-23 16:33:10 UTC
> it would be lovely not to name the wrapper of
> GNode to Glib::Tree especially because there is balanced tree implementatation
> in Glib with a structure name GTree.

Ah. Well spotted. How about using Glib::NodeTree instead, though let's keep that out of any patches for now, to keep things simple. I really don't like the Glib::Node (or GNode) name. It's too obscure.

> > > 2; Foreach or traverse slots should use the tree node instead of the node's
> > > data only
> > 
> > What would be the advantage of this?
> 
> In case of the original version (GNode) a GNode * is passed to the callback
> function, which makes possible to examine the location of the given node in the
> complete tree (e.g.: depth, index). In case of an own Gtk::TreeModel
> implementation it is necessary.

Ah. Could you show this in one of the examples, please.
 

> > The patch seems to have lots of unrelated changes, which makes it difficult to review.


Comment 19 Szilárd Pfeiffer 2008-07-24 13:48:31 UTC
Created attachment 115172 [details]
example to comment 18
Comment 20 Szilárd Pfeiffer 2008-07-24 13:52:40 UTC
(In reply to comment #18)
> > it would be lovely not to name the wrapper of
> > GNode to Glib::Tree especially because there is balanced tree implementatation
> > in Glib with a structure name GTree.
> 
> Ah. Well spotted. How about using Glib::NodeTree instead, though let's keep
> that out of any patches for now, to keep things simple. I really don't like the
> Glib::Node (or GNode) name. It's too obscure.

I work with GLib and Glibmm in parallel, so if the name of the wrapper and the original differ it disturbs me, but it is only my humble opinion. It is your choice, but if I have enough time I will write a wrapper to GTree, so I suggest the consideration of the name.

> 
> > > > 2; Foreach or traverse slots should use the tree node instead of the node's
> > > > data only
> > > 
> > > What would be the advantage of this?
> > 
> > In case of the original version (GNode) a GNode * is passed to the callback
> > function, which makes possible to examine the location of the given node in the
> > complete tree (e.g.: depth, index). In case of an own Gtk::TreeModel
> > implementation it is necessary.
> 
> Ah. Could you show this in one of the examples, please.
> 

I have given you an exemple (attachment 115172 [details]). It is not the final version, but I hope it shows you my goal.

> 
> > > The patch seems to have lots of unrelated changes, which makes it difficult to review.
> 

Comment 21 Murray Cumming 2008-07-25 12:59:33 UTC
> I work with GLib and Glibmm in parallel, so if the name of the wrapper and the
> original differ it disturbs me, but it is only my humble opinion. It is your
> choice, but if I have enough time I will write a wrapper to GTree, so I suggest
> the consideration of the name.

I still don't like the GNode name.

And we still need a patch without the unrelated changes. Time is running out, I'm afraid. GNOME API freeze is July 28th.


> > > In case of the original version (GNode) a GNode * is passed to the callback
> > > function, which makes possible to examine the location of the given node in the
> > > complete tree (e.g.: depth, index). In case of an own Gtk::TreeModel
> > > implementation it is necessary.
> > 
> > Ah. Could you show this in one of the examples, please.
> > 
> 
> I have given you an exemple (attachment 115172 [details] [edit]). It is not the final version,
> but I hope it shows you my goal.

This seems to be a custom Gtk::TreeModel. I guess you mean to show use of methods like is_leaf(), but please try to create a simpler example or test that we can put in glibmm itself.

Comment 22 Jonathon Jongsma 2008-07-25 21:41:05 UTC
OK, I finally got some time to look at this API.

I think I agree with Szilard that it would be nice to pass a reference to the node to the traversal functions.  It doesn't make these functions much more cumbersome and it allows you to do things that you couldn't do otherwise (and it seems to match the C API better as well).

The changes proposed also address my main concerns about the initial patch, which is that it exposes too much internal stuff (e.g. the NodeMap typedef) and keeps too much extra stuff in the wrapper class instead of relying on the data from the underlying C object (e.g. the C++ wrapper class had a children_ member that kept track of the node's children separately from the C object that it is wrapping).  

Regarding the name of the class, I agree that Glib::Tree is a bit problematic considering that there is a totally separate GTree type in Glib.   I'm also not a big fan of the name GNode/Glib::Node, but perhaps it would be best to use it anyway just to stay as compatible with glib as possible...  Maybe NodeTree is OK, I'm not sure.
Comment 23 Szilárd Pfeiffer 2008-07-27 21:24:09 UTC
Created attachment 115391 [details] [review]
Cleaned version of suggested changes

I've created the cleaned version of the suggested changes to make the review easier
Comment 24 Murray Cumming 2008-07-28 07:07:01 UTC
Thanks. Could you explain the removal of the gobject_, owns_gobject_, children_ and parent_ member variables and the subsequent simplification of the code. Why were they necessary before? Why are they not necessary now?

In general, please provide a ChangeLog entry with patches. It should explain what is being changed and why.
Comment 25 Jonathon Jongsma 2008-07-28 14:47:37 UTC
I alluded to the removal of the children_ and parent_ in my comment above.  Essentially, I believe that the previous code was doing a lot of duplicate book-keeping by keeping references to the children and parents in the C++ wrapper instead of relying on the underlying C object to tell us that information.  So I am in favor of removing these and simplifying the wrapper implementation.  There's less chance for bugs in the wrapper if it leaves that stuff to the C object.
Comment 26 Murray Cumming 2008-07-28 15:23:40 UTC
Sure. Simpler code is better. I just like to see patches explained.
Comment 27 Jonathon Jongsma 2008-07-28 15:34:11 UTC
yes, I'd still like to hear szilárd's comments on it as well.
Comment 28 Szilárd Pfeiffer 2008-07-28 18:12:33 UTC
(In reply to comment #24)
> Thanks. Could you explain the removal of the gobject_, owns_gobject_, children_
> and parent_ member variables and the subsequent simplification of the code. Why
> were they necessary before? Why are they not necessary now?
> 
> In general, please provide a ChangeLog entry with patches. It should explain
> what is being changed and why.
> 

gobject_:
The member haven't been removed, but it is protected now (should be private, because it's the class own business). It stores the C version (GNode *) of the object as did it before my patch.

gobject_->data:
Stores this pointer of the C++ object. It makes possible to acquire the C++ version of the object from a C callback.

children_:
Stored the child nodes of the object. Obsoleted by the idea that the data pointer of the C version stores a pointer to the C++ version, so the GNode * pointers which are returned by the wrapped functions can be "converted" to the C++ version without any map.

parent_:
Stored a pointer to the parent. It is unnecessary now, because the parent of an object can be acquired via the parent pointer of the C version, just like in case of children.

owns_gobject_:
Stored whether the node is in a tree. In my humble opinion it is unnecessary at all in case of a wrapper.

subsequent changes:
Result of the changes which have been mentioned above.
Comment 29 Murray Cumming 2008-07-29 09:03:39 UTC
Thanks. I committed the patch, with this ChangeLog entry:

2008-07-29  Szilárd Pfeiffer  <szilard.pfeiffer@gmail.com>

	* glib/src/tree.hg: Make the callbacks take a Tree<> instead of just 
	the data, so they can use methods on the tree (which can be a node 
	in the tree).
	gobject_: Make this protected.
	Provide the this pointer as data to g_node_new() so we can retrieve 
	it later.
	Removed children_ and parent_ because we don't need a separate store now that 
	we can get the C++ instance from the gobject instance.
	owns_gobject_: Removed because it is was always true, so the gobject was 
	always destroyed (and still is).
	* tests/glibmm_tree/main.cc: Updated for the changed API.
	Bug #520778.

Please remember to patch the ChangeLog in future.