GNOME Bugzilla – Bug 723801
Wrap GtkFlowBox
Last modified: 2014-04-09 07:23:28 UTC
Created attachment 268345 [details] [review] GtkFlowBox wrapper I think it's almost complete. I left out child-activated signal because its g_signal_new does not match the callback signature (see https://bugzilla.gnome.org/show_bug.cgi?id=723716). Once that gets fixed I'll upload a revised and complete wrapper. I could have patched gtk_signals.defs so that the argument to that signal would be a GtkFlowBoxChild. But for now I prefer to wait for this bug to be fixed. If it doesn't receive attention soon, then I'll upload a revised wrapper with gtk_signals.defs patched.
Created attachment 268439 [details] [review] GtkFlowBox wrapper + patch for child-activated This wrapper extends the previous one by wrapping also ::child-activated. For this to work patching gtk_signals.defs is required, but I'm not sure if I generated the .defs.patch file correctly :S Also, some of the conversions included in convert_gtk.m4 are not strictly required, but I included it for consistency sake with FlowList.
Created attachment 268484 [details] [review] GtkFlowBox wrapper Final wrapper
The bug I reported against Gtk+ got fixed, so now there's no need to patch gtk_signals.defs. I've just uploaded a final patch for review.
Thanks. That looks perfect. I've pushed it.
I was just about to add the following comment, but Murray had closed the bug a few minutes before. The first of the following paragraphs is still worth considering. The "activate" signal in GtkFlowBoxChild shall be wrapped, I think. The documentation says "While this signal is used as a keybinding signal, it can be used by applications for their own purposes." Compare e.g. Gtk::Entry::signal_activate(), which was discussed at length in bug 655489 and bug 655500 before a similar sentence was added for the GtkEntry::activate signal. In flowbox.hg: * The @a slot will be called for ach child after the call, and "ach" should be "each". It's misspelt also in gtkflowbox.c, but that's not worth a bug report. Declaring functions static within an anonymous namespace is a tautology. An anonymous (unnamed) namespace is the recommended way to make functions and data local to a compilation unit in C++. "static" must be used to achieve the same result in C.
(In reply to comment #5) > The "activate" signal in GtkFlowBoxChild shall be wrapped, I think. > The documentation says "While this signal is used as a keybinding signal, > it can be used by applications for their own purposes." > Compare e.g. Gtk::Entry::signal_activate(), which was discussed at length in > bug 655489 and bug 655500 before a similar sentence was added for the > GtkEntry::activate signal. OK, yes. > In flowbox.hg: > * The @a slot will be called for ach child after the call, and > "ach" should be "each". It's misspelt also in gtkflowbox.c, but that's not > worth a bug report. Thanks. I have fixed that in gtk+'s git master. > Declaring functions static within an anonymous namespace is a tautology. > An anonymous (unnamed) namespace is the recommended way to make functions and > data local to a compilation unit in C++. "static" must be used to achieve the > same result in C. Yes, but I think I started doing this (using static too) a few years ago while trying to reduce the built library's code size. I think that using static was the only way to keep the function out of the list of exported symbols, but I still wanted to use the C++ way too. I vaguely remember that the anonymous namespace is not meant to prevent exporting of the symbol, for some subtle reasons. I could be wrong.
(In reply to comment #6) > Yes, but I think I started doing this (using static too) a few years ago while > trying to reduce the built library's code size. I think that using static was > the only way to keep the function out of the list of exported symbols, but I > still wanted to use the C++ way too. I vaguely remember that the anonymous > namespace is not meant to prevent exporting of the symbol, for some subtle > reasons. I could be wrong. You may be right. The exact effect of anonymous namespace and static may depend on the compiler and linker. Bjarne Stroustrup says in "The C++ Programming Language", 4th edition, 15.2.1: "The effect of an unnamed namespace is very similar to that of internal linkage." Not necessarily exactly the same. When I checked the result of building gtkmm with gcc 4.8.1, I found $ nm libgtkmm-3.0.so.1.1.0 | grep SignalProxy_Filter 0000000000326000 t SignalProxy_Filter_gtk_callback 0000000000325fd0 t SignalProxy_Filter_gtk_callback_destroy 0000000000336aa0 t _ZN12_GLOBAL__N_131SignalProxy_Filter_gtk_callbackEP14_GtkListBoxRowPv 0000000000336a10 t _ZN12_GLOBAL__N_139SignalProxy_Filter_gtk_callback_destroyEPv 0000000000323690 t _ZN12_GLOBAL__N_1L31SignalProxy_Filter_gtk_callbackEP16_GtkFlowBoxChildPv 0000000000323770 t _ZN12_GLOBAL__N_1L39SignalProxy_Filter_gtk_callback_destroyEPv The first 2 SignalProxy_Filter* are defined in fontchooser.ccg with anonymous namespace, extern "C" and static. The middle 2 SignalProxy_Filter* are defined in listbox.ccg with anonymous namespace. The last 2 SignalProxy_Filter* are defined in flowbox.ccg with anonymous namespace and static. When I added static in listbox.ccg, the size of libgtkmm-3.0.so.1.1.0 decreased from 24,810,668 bytes to 24,810,550 bytes. When I then removed static from listbox.ccg, calendar.ccg, flowbox.ccg and fontchooser.ccg, the size decreased to 24,810,485 bytes. The results of other similar tests are equally confusing. In some cases the size of the .so file increases when static is added, in other cases it decreases. I suppose that the results may be different with another compiler or another version of gcc.
I just noticed another detail in flowbox.hg. gmmproc reports unwrapped signals because _IGNORE is used instead of _IGNORE_SIGNAL.
Created attachment 268738 [details] [review] Various fixes to Gtk::FlowBox I think this patch addresses the discussed issues, except for the 'anonymous namespace + static' constructs.
Comment on attachment 268738 [details] [review] Various fixes to Gtk::FlowBox Looks good. Please push.
Pushed. Thanks.
Comments on the FlowBox example program in https://mail.gnome.org/archives/gtkmm-list/2014-April/msg00016.html I think this example program is OK as it is. Most of our example programs in the gtkmm tutorial don't use every method, signal and property of a class. There are some details that can be improved, if you like, but it's not important. - The year in the copyright text should be 2014. - 665 colored areas is way too many. About 100 is more than enough for a small demonstration program. - The dependence between N_COLORS, defined in examplewindow.h, and the number of elements in colors[], defined in examplewindow.cc, is not nice. It's a kind of dependence that makes a program hard to maintain. It's not serious in such a small program as this one, but if possible example programs in the gtkmm tutorial shall show good programming. I guess you can do without N_COLORS if m_color_swatch and m_color_drawing_area are std::vectors instead of arrays.
Created attachment 273749 [details] [review] Gtk::FlowBox book example Here's an improved version of the Gtk::FlowBox example app, taking into account comments by Kjell. I'd rather commit this as a book example, since I have never played with gtkmm-demo and it would require me some time to push this code into gtkmm-demo. However, I'll do whatever you think is best; I think I could get this into gtkmm-demo by the end of this week.
I get the following compilation error: book/flowbox/examplewindow.cc:36:42: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] for (int i(0); i != m_color_names.size(); ++i) ^ cc1plus: all warnings being treated as errors I changed to for (std::size_t i(0); i != m_color_names.size(); ++i) Otherwise it looks fine. gtkmm-documentation/book/flowbox or gtkmm/demos/gtk-demo? That's up to Murray. He said in a gtkmm-list posting that he'd prefer this example in gtkmm/demos. I don't know why.
gtkmm-documentation/book/flowbox should be gtkmm-documentation/examples/book/flowbox.
I'd prefer the full version to be in our demo because it is in the C demo: https://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/flowbox.c We try to keep our gtkmm-demo as close as possible to the gtk+ demo. I'd happily have a simpler version in gtkmm-documentation too. I can take care of that if nobody else wants to.
On second thoughts, it doesn't look too complicated apart from the long list of colors, though I'd rather have random colors instead. I've pushed it and committed some small improvements. I'll add this to gtkmm-demo too. Thanks.