Created attachment 115395 [details] [review]
possible solution

Possible solution of the problem has attached

Eek. Where is this enum used?

(In reply to comment #2)
> Eek. Where is this enum used?
> 

Nowhere. I found this problem when I was testing my patch (bug 520778 comment 23).

Thanks.

Because this is not used anywhere else, we can just hand-code this enum in the class anyway, giving us the chance to do it correctly, and allowing us to omit the deprecated LEAFS mispellings.

I have done that in svn.

I am afraid the TraversFlags enum inserted to a wrong place (not exactly to the Glib namespace).

Created attachment 115526 [details] [review]
move the TraverseFlags to the right place

Created attachment 115528 [details] [review]
move the TraverseFlags to the right place

It's meant to be inside NodeTree, because it is not used anywhere else.

(In reply to comment #8)
> It's meant to be inside NodeTree, because it is not used anywhere else.
> 

It is not used, but it will be, just like in case of TraverseType.
If I made the following change in the test file it would cause compilation problem (without attachment 115528 [details] [review]):

--- tests/glibmm_nodetree/main.cc	(revision 704)
+++ tests/glibmm_nodetree/main.cc	(working copy)
@@ -54,7 +54,7 @@
   std::cout << std::endl;
 
   std::cout << "Leaf children of 'a':" << std::endl;
-  ta.foreach(sigc::bind<bool>(sigc::ptr_fun(echol), true));
+  ta.foreach(sigc::bind<bool>(sigc::ptr_fun(echol), true), Glib::TRAVERSE_ALL);
   std::cout << std::endl;
 
   std::cout << "Non-leaf children of 'a':" << std::endl;


error message of gcc:

main.cc: In function ‘int main()’:
main.cc:57: error: ‘TRAVERSE_ALL’ is not a member of ‘Glib’

you should use Glib::NodeTree::TRAVERSE_ALL

> > It's meant to be inside NodeTree, because it is not used anywhere else.
> It is not used, but it will be,

The C enum is not used anywhere else, so I doubt that.

> If I made the following change in the test file it would cause compilation
> problem (without attachment 115528 [details] [review] [edit]):

Obviously, because it should be Glib::TreeNode<std::string>::TRAVERSE_ALL. I agree that is not pretty, but you should be using a typedef for the TreeNode to simplify your code anyway.

(In reply to comment #10)
> you should use Glib::NodeTree::TRAVERSE_ALL
> 

Thanks. I really know it, but I don't understand it. Is it a good idea to handle similar enums (TraverseType, TraverseFlags) in a different way (point of namesapces) ?

(In reply to comment #11)
> > > It's meant to be inside NodeTree, because it is not used anywhere else.
> > It is not used, but it will be,
> 
> The C enum is not used anywhere else, so I doubt that.
> 
> > If I made the following change in the test file it would cause compilation
> > problem (without attachment 115528 [details] [review] [edit] [edit]):
> 
> Obviously, because it should be Glib::TreeNode<std::string>::TRAVERSE_ALL. I
> agree that is not pretty, but you should be using a typedef for the TreeNode to
> simplify your code anyway.
> 

I agree taht it is not so pretty, because in compile time different enum will be generated for each and every different Glib::NodeTree<T> depending on T.

Where an enum is used by only one class, then, yes, that enum should be in the class. That makes it more self-documenting.

We would do this with more enums, but _WRAP_ENUM() doesn't do that for us yet:
http://bugzilla.gnome.org/show_bug.cgi?id=86864

I would like the two enums to have different prefixes for their values, but I can't think of good ones.

(In reply to comment #14)
> Where an enum is used by only one class, then, yes, that enum should be in the
> class. That makes it more self-documenting.
> 
> We would do this with more enums, but _WRAP_ENUM() doesn't do that for us yet:
> http://bugzilla.gnome.org/show_bug.cgi?id=86864
> 
> I would like the two enums to have different prefixes for their values, but I
> can't think of good ones.
> 

I see. The concept is pretty good. The only thing which disturbs me is the inconsistency (comment 12). Otherwise in case of a template class each enums which defined in the class will be different types (depending on T). In my humble opinion it is worth considering that (maybe it's not a big deal).

> in case of a template class each enums
> which defined in the class will be different types (depending on T). In my
> humble opinion it is worth considering that (maybe it's not a big deal).

Two specializations of the template can't be used polymorphically (there's no base class) so I don't see a real problem with this. Generic use of the types would be via a template, which would allow use of the templated typename.

Closing for now. But please reopen if you can suggest an improvement. Thanks for the vigilance.