For those of you who do not know about gtkmm (taken from the gtkmm project website):
"gtkmm is the official C++ interface for the popular GUI library GTK+. Highlights include typesafe callbacks, and a comprehensive set of widgets that are easily extensible via inheritance. You can create user interfaces either in code or with the Glade User Interface designer, using Gtk::Builder. There's extensive documentation, including API reference and a tutorial.
gtkmm is free software distributed under the GNU Library General Public License (LGPL).
gtkmm follows the official GNOME Platform Bindings release schedule. This guarantees API/ABI-stability and new releases on a predictable schedule, delivering C++ API for the underlying GTK+ and GNOME APIs as soon as possible." (from gtkmm project's website).
This has been fun again. Working on the project made me realize how laborious it can be to:
- learn C++. What a monster!. This is all about experience, so my hope is that one day I will be the monster here, the C++ monster ;-).
- convert an app that you have already developed into another language. The appropriate word here would actually be "rewrite" and not "convert", since you have to consider it as an entirely new project. Forget about what you did and focus on the new structure and libraries. This is a new project!
- have a nice history in your repository... This takes practice, practice and more practice.
I am happy with the results so far although improvements are still being made. You can find and follow the project on GitHub where all comments and merge requests are welcome. Do not hesitate to do this.
Thanks for reading!
Nice, some comments:
ReplyDelete1. Consider making ctors explicit and dtors virtual. You can find the the reasoning in effective c++
2. in src/main.cc: I dont remember how GtkBuilder works, but is it really necessary to make "GHangtuxmmApp* GHapp = 0;" a pointer? Then, "refBuilder->get_widget_derived("main_window", &GHapp);" should do it. Oh, and why capital letters for a non-constant?
3. You have some widget pointers as members in GHangtuxmmApp. You should make clear that those pointers are not managed by the object itself, but through GtkBuilder. You could document that by grouping them into their own struct.
4. Try to spread your code with the const disease. "Glib::ustring ui_menu_toolbar" is a typical const variable. Remember, whenever there's constness, it makes it easier to follow your code, because mostly I only need to follow the non-const stuff.
Even for pointers that are used over a longer scope, it can be nice if you guarantee that the pointer's value wont change, ie, "SomeType *const someVar = ..." This will still allow you to modify the underlying object, but the pointer is now tied to the object, at least.
5. You assign stuff to variables that are never used? Turn on -Wall compiler flags.
6. Prefer anonymous namespaces over static variables hiding in some functions, for constants (looking at static const int N_ROWS = 2; static const int N_COLS = 13;). It's eventually the same thing (if you follow one-class-per-file), but at least the constants are documented and defined at the top of the impl file.
7. You call show_all_children() in GHangtuxmmKeyboard - that's usually considered bad style (although it works for you). Reason: The ctor already assumes how this widget will be used. Such implicit (unnecessary) assumptions make your components less flexible. A ctor should do the minimal setup necessary to make an object (barely) functional. This is even more true for C++, where ctors are non-virtual, and where complex ctors in deeply nested hierarchies become a major PITA.
In general, avoid side effects like this. Dont let one function do *everything*, only because it feels convenient at the time being.
8. Consider using doxygen comments for C++ headers. It'll come in handy if you already started partially with comments, and decide to make your API fully documents. Being able to create doxygen documentation from your code is a huge help for others who want to start to contribute to your project. And yeah, no comments in the impl files please. If you were working on a library, then usually only the headers are available for others. It's nice to have the documentation there where others can access it.
> Consider making ctors explicit
ReplyDeleteI don't see any single-parameter constructors.
> and dtors virtual.
Glib::ObjectBase already has a virtual destructor.
> is it really necessary to make "GHangtuxmmApp* GHapp = 0;" a pointer?
Yes. Gtk::Builder instantiates instances. It doesn't just change the internal states of existing instances.
> Turn on -Wall compiler flags.
There's already a --enable-warnings=fatal configure option.
> You call show_all_children() in GHangtuxmmKeyboard > - that's usually considered bad style
No, it isn't. It would be if it was show_all(). Without show_all_children() (or lots of show()s), the widget would appear blank when show() is called on it.
Your other comments are useful.
> I don't see any single-parameter constructors.
ReplyDeleteI know, but I'd still use explicit for anything where you are sure you never ever want implicit casting (usually true for most of your classes), and never ever think about it again. Reason: ctor params keep changing all the time (esp. true for larger projects), and you might start with two params, then someone decides to give the second one a default value, but forgets the explicit keyword.
For the virtual dtor, it's similar: if it's a derived class, fine. If it is a base class: it's easily forgotten (and when I derive from it, I'd always have to check the base classes first to find out whether I can omit the virtual or not - the base class doesn't need to have members, for example). Again, I'd always do it, and stop thinking about it (unless we talk about structs or such where I surely dont want vtables).
> --enable-warnings=fatal
Then something else is wrong. Unused variables should not be compilable, with fatal warnings.
> show_all vs. show_all_children
ah alright then, mea culpa
> > I don't see any single-parameter constructors.
ReplyDelete> I know, but I'd still use explicit for anything
I'm pretty sure that used to cause a compile warning, though maybe it doesn't any more. Actually, maybe that's only on some compilers. gtkmm builds with a variety of weird unix compilers and I remember having to write the code generator to only add the explicit keyword where it would make sense. Anyway, yes, yours is a good rule.
Michael: thank you very much for investing part of your time on reviewing GHangtuxmm and explaining with detail why those changes should be applied. That really helps. I will revise the code as soon as I can.
ReplyDeleteMurray: thanks to you too. As I said to Michael, I will revise the code as soon as I can.
Thank You and I have a dandy give: Where To Start With Whole House Renovation remodeling companies near me
ReplyDelete