Customisable Toolbars

[greebo]

I just committed the ToolbarCreator class and some smaller changes. As for now, the standard radiant toolbar is outsourced into the ui.xml file.

 

@orbweaver: Could you test it on your machine if it compiles without errors? I also made a change to sconscript so I just want to make sure it compiles on Linux as well.

 

The cleanup of the old, unused code is still to come, I wanted to do this in a separate commit - it would be easier to revert if I break anything.

 

Oh, and please do step on my feet if I committed some horrible code or design sins, feedback is always appreciated

[OrbWeaver]

Well it compiles, runs and the toolbar looks OK, so it's looking good so far.

 

I will look at the code in more detail tomorrow.

[greebo]

The code cleanup is committed now, I performed a full compile and it finished without errors.

[OrbWeaver]

OK, some preliminary comments in decreasing order of importance.

• Overall, the code structure is well-commented and readable, which is good.
  
• You seem to be allocating the toolbar creator on the heap with new but never deleting it. You should never do that in C++, it is a memory leak. If you only need the object for the duration of the function, allocate it on the stack as follows:
   
  

  toolbar::ToolbarCreator toolbarCreator(GameToolsPath_get());

  
   
  This way it gets destructed automatically at the end of the scope, and doesn't cause a leak.
  

• UI code should be in the ui namespace, rather than an individual namespace like toolbar
  
• Don't write functions to take const char* as arguments -- make them take const std::string& which can be implicitly created from const char* without having to code an explicit cast.
  
• Your ToolbarCreator::ParseXml() function is taking an xml::Document by value rather than by reference, which means that the object will get copied when the function is called. This doesn't actually matter in this case because xml::Document only contains a pointer, but for larger objects this may be a problem. In fact, if I had written xml::Document properly it would actually "own" the entire XML document tree, which means that a pass-by-value would result in a complete traversal of the XML tree in order to copy it.
  
• I don't think ui.xml should be in the doom3.game folder, as it is not game-specific. I would put this in the main DarkRadiant install folder.
  
• Functions should start with lowercase letters, so that they can be distinguished easily from objects which have capital letters (i.e. ToolbarCreator::getToolbar() not GetToolBar())
  
• You don't need to indent everything within a namespace -- most new code should be in a namespace and this would cause you to lose 4 characters for everything.
  

Good work in all though, this should set the stage for greater UI customisability in future. I wonder if it is possible to merge the UI and game XML trees so that you could call GlobalRadiant().getXPath() with either a "/ui/blah/bleh" or "/game/blah/bleh" from other code.

[greebo]

OK, some preliminary comments in decreasing order of importance.

First of all thanks for reading through the code, I will do my best and will update the files as soon as possible.

[*]You seem to be allocating the toolbar creator on the heap with new but never deleting it. You should never do that in C++, it is a memory leak. If you only need the object for the duration of the function, allocate it on the stack as follows:

 

toolbar::ToolbarCreator toolbarCreator(GameToolsPath_get());

 

This way it gets destructed automatically at the end of the scope, and doesn't cause a leak.

Yes, I suspected something like that. I will test if it is sufficient to stack-allocate the class or if this causes problems with the GtkToolbar instances (but it should not, as the GtkToolbars are pointers to GTK-allocated objects, aren't they?).

 

I don't know if this is a memory leak as well: I store all the created GtkToolbar* pointers in a std::map, do I have to clear that one in the destructor of the class or is this done by C++ routines automatically?

 

[*]UI code should be in the ui namespace, rather than an individual namespace like toolbar

Ok, I was unsure about that one, I will change this to ui, makes more sense anyway.

 

[*]Don't write functions to take const char* as arguments -- make them take const std::string& which can be implicitly created from const char* without having to code an explicit cast.

Ok, so in general, I should not use const char*? I will change this, no problem.

 

[*]Your ToolbarCreator::ParseXml() function is taking an xml::Document by value rather than by reference, which means that the object will get copied when the function is called. This doesn't actually matter in this case because xml::Document only contains a pointer, but for larger objects this may be a problem. In fact, if I had written xml::Document properly it would actually "own" the entire XML document tree, which means that a pass-by-value would result in a complete traversal of the XML tree in order to copy it.

I know about that one and I tried to change it once to xml::Document& but it somehow didn't work so I left it, knowing that it is copied into the call stack. I will look into it and iron this out.

[*]I don't think ui.xml should be in the doom3.game folder, as it is not game-specific. I would put this in the main DarkRadiant install folder.

Ok, no problem, the path to the file is passed to the constructor, so this should be easy.

[*]Functions should start with lowercase letters, so that they can be distinguished easily from objects which have capital letters (i.e. ToolbarCreator::getToolbar() not GetToolBar())

I obviously mixed this one up. I will change the methods to lowercase beginnings.

 

[*]You don't need to indent everything within a namespace -- most new code should be in a namespace and this would cause you to lose 4 characters for everything.

Will change it too, no problem.

 

Good work in all though, this should set the stage for greater UI customisability in future. I wonder if it is possible to merge the UI and game XML trees so that you could call GlobalRadiant().getXPath() with either a "/ui/blah/bleh" or "/game/blah/bleh" from other code.

Yes this should indeed be possible. At first I kept the files separate because GtkUIManager did not like the XML definitions to be encapsulated by another container (it expected the tag to be the root tag). Now that I kicked out GtkUIManager this should be no problem, we just need to store everything within a top level container, in accordance to the XML definitions, like this:

<darkradiant>
  <game>
  </game>
  <ui>
  </ui>
</darkradiant>

But on the other hand, we take the risk to mix everything into one file (as you said above, the UI is not game-specific) that maybe hard to maintain or overview if it is getting larger.

 

And thanks again for providing the feedback!

[OrbWeaver]

Yes, I suspected something like that. I will test if it is sufficient to stack-allocate the class or if this causes problems with the GtkToolbar instances (but it should not, as the GtkToolbars are pointers to GTK-allocated objects, aren't they?).

 

I don't know if this is a memory leak as well: I store all the created GtkToolbar* pointers in a std::map, do I have to clear that one in the destructor of the class or is this done by C++ routines automatically?

 

All GTK widgets are owned by GTK, so you don't need to free them manually (except in certain cases). They use something called "floating reference counting" which means that although they start off with a reference count of 1, this reference is automatically owned by the parent widget as soon as they are packed (and hence will be destroyed with the parent widget).

 

Ok, so in general, I should not use const char*? I will change this, no problem.

 

Yep. Never use const char*, ever. The only reason to use this is for GTK functions which require it, but in this case you should convert to char* at the last minute by calling the c_str() function on the string.

 

Yes this should indeed be possible. At first I kept the files separate because GtkUIManager did not like the XML definitions to be encapsulated by another container (it expected the <ui> tag to be the root tag). Now that I kicked out GtkUIManager this should be no problem, we just need to store everything within a <darkradiant> top level container, in accordance to the XML definitions, like this:

<darkradiant>
  <game>
  </game>
  <ui>
  </ui>
</darkradiant>

But on the other hand, we take the risk to mix everything into one file (as you said above, the UI is not game-specific) that maybe hard to maintain or overview if it is getting larger.

 

I didn't mean in one file, I meant that the code could merge the two trees into a single in-memory tree. I haven't investigated in depth whether libxml2 can do this, although I suspect it can.

 

One file is certainly what we don't want - it would mix up the user-specific UI settings with game-specific options that are not touched by the user.

[greebo]

Ok, all these changes are committed now.

 

Any new tasks for me? Or should I pick one from the DarkRadiant ToDo-List in the Programming Forum?

[OrbWeaver]

Ok, all these changes are committed now.

 

Excellent.

 

Any new tasks for me? Or should I pick one from the DarkRadiant ToDo-List in the Programming Forum?

 

That's the best bet.

[greebo]

I just noticed that the tooltips are missing in newly created toolbars, fix is in progress and will be committed soon.

[greebo]

The fix is now on SVN.