GNOME Bugzilla – Bug 90126
Box::pack_start() is not very typesafe.
Last modified: 2004-12-22 21:47:04 UTC
With the new release we see under Solaris/Sparc (both compiled by g++ and Forte C++) some layout distortions. Gtk::SHRINK does not seem to work anymore. This seems to be connected with the following run-time warning (menu:21056): gtkmm-WARNING **: gtkmm: Attempt to call Gtk::manage() on a Gtk::Window, but a Gtk::Window has no parent container to manage its lifetime. which we get for instance with almost all example programs now.
The complete build consists of: pkgconfig-0.12.0 glib-2.0.6 atk-1.0.3 pango-1.0.4 gtk+-2.0.6 libsigc++-1.1.13 gtkmm-1.3.19
Which examples? They would need to be fixed. Please give us an example of the Gtk::SHRINK problem.
The program ~/examples/mene/menu shows a single menubar which is about 50 pixel in height. All programs in this directory (menu, menu2, and stock) show the given warning.
I went through all example programs now. Beside the menu example programs I find the warning only in examples/tictactoe/ttt_test. This program as well as menu/menus and menu/stock look okay, only the warning occurs otherwise. At work we had massive layout distortions on out test applications, which all have menus and followed the menu/menu.cc example. With gtkmm-1.3.18 the code worked fine. It just happened with the new release.
I am going to delete those menu examples. There are new ones in examples/book/menus. You _cannot_ use manage() on top-level windows. So we need 1. Another example of "layout disturbance". 2. To fix the tictactoe example's use of manage().
Actually we have two problems in this case: (1) managed Gtk::Windows without parent containers This gives the shown warnings. The code of the affected examples has to be fixed here. This has nothing to do with the "layout disturbance", it simply happened to show up at the same time in examples/menu/menu.cc. (2) use of Gtk::SHRINK instead of Gtk::PACK_SHRINK Box::pack_start was changed (1.13.18 -> 1.3.19) from AttachOptions to PackOptions type for the option parameter: class Box { ... void pack_start(Widget& child, bool expand, bool fill = true, guint padding = 0); void pack_start(Widget& child, PackOptions options = PACK_EXPAND_WIDGET, guint padding = 0); ... }; Some people (like me) still have Gtk::SHRINK applied to pack_start member function of box type containers. The problem here is now, that using wrong AttachOptions here gives no compiler error, because it automatically evaluates the second and third parameter to bool thus matching the first form of Box::pack_start. I can clearly see that when hide to first member function: "menu.cc", line 66: Error: Formal argument options of type Gtk::PackOptions in call to Gtk::Box::pack_start(Gtk::Widget&, Gtk::PackOptions, unsigned) is being passed Gtk::AttachOptions. After changing all occurances of Gtk::SHRING to Gtk::PACK_SHRINK all "layout disturbance" in our test applications is gone. So, we have to think about the first pack_start member function. Their automatic matching has to be prevented in my eyes. Seems to me that have to melt somewhat which was just frozen :-(
1. Like I said, I will delete those old menu examples. Do any other examples have that problem. If so, please create a separate bug, with a patch if possible. 2. I don't undertand. Please show the a line of code that has the problem, and show me what method overload it tries to use. If it can be fixed without making the API more difficult then will.
Calling Box::pack_start(...) mistakenly with Gtk::SHRINK for the options parameter like pBox->pack_start(widget, Gtk::SHRINK, 2); actually call the member function void Box::pack_start(Widget& child, bool expand, bool fill = true, guint padding = 0), means that we are calling pBox->pack_start(widget, true, true, 0); This is not what the user expects. The possibility to evaluate to the wrong function call is evil here. A compile error should happen instead, otherwise the enum types are needless. Just replace in line 66 of examples/menu/menu.cc vbox->pack_start(*menu_bar, Gtk::SHRINK, 2); by vbox->pack_start(*menu_bar, Gtk::PACK_SHRINK, 2); and see the difference in he layout yourself.
I have no idea why Gtk::SHIRNK is being converted implicitly to bool. If we remove the default values then we actually break the API for people porting from gtkmm 1.2. Do any examples need to be fixed?
That what I still see in the examples examples/menu/menu.cc: vbox->pack_start(*menu_bar, Gtk::SHRINK, 2); examples/menu/menu3.cc: vbox->pack_start (*menubar, Gtk::SHRINK); examples/packbox/packbox.cc: m_box1.pack_start(m_seperator2, Gtk::SHRINK, 5); examples/progressbar/progressbar2.cc: vbox.pack_start(align, Gtk::SHRINK, 5); examples/progressbar/progressbar2.cc: vbox.pack_start(hseparator1, Gtk::SHRINK); examples/progressbar/progressbar2.cc: vbox.pack_start(hseparator2, Gtk::SHRINK); examples/progressbar/progressbar2.cc: vbox.pack_start(button, Gtk::SHRINK); Hopefully, we dont have such pitfalls in the library code.
> I have no idea why Gtk::SHIRNK is being converted > implicitly to bool. Well, that's the way it goes: Enums are as integral types convertible, thus they get implicitly converted to int (but not the opposite way), and int are implicitly converted to bool. A small example for the doubters: $ cat test.cc #include <iostream> using namespace std; enum Case { One=1, Two=2 }; int main() { int a = One; bool b = Two; cout << "a = " << a << endl << "b = " << (b ? "true" : "false") << " (" << b << ")" << endl; return 0; } $ g++ test.cc $ ./a.out a = 1 b = true (1) $ CC test.cc # Sun Forte C++ $ ./a.out a = 1 b = true (1) I dont see any way to improve the situation with respect to the enums here. Normally in class scope we could overload the conversion operators to dump a warning at least, but enums have no scope. When the overloaded pack_start member function exists only for compatibility reasons, then we could pricipally surround their definition by a define which could switch the visibility of all old backward compatibility functions to the compiler. By setting this define the user would have a chance to let the compiler check the type safety of the function call.
Examples fixed. Is this correct?: People who have never used Gtk::SHRINK, Gtk::FILL, etc, will have no problem with the current API?
And if we just rename Gtk::SHRINK & Co into Gtk::ATTACH_SHRINK & Co? That would be more consistent anyway considering the naming pattern of our enums.
> And if we just rename Gtk::SHRINK & Co into Gtk::ATTACH_SHRINK & Co? I would also recommend such a naming scheme. It helps to improve the readability of the Gtk-- code. But the basic problem in the case is that some (old) overloaded member functions break the type safety of enum parameters. This behavior is very dangerous and normally to be avoided for C++ code. A check in box.h shows that the same flaw applies to the set_options and pack_end member functions of class Box. In this case we could use a define (see below) to avoid this situation when backward compatibility is not required. TANSTAAFL. Of course, the library should always be built with backward compatibility set. First we should find out how often this situation might occur, which is not an easy task with 119 different named enums. class Box { ... #ifdef GTKMM12_BACKWARD_COMPATIBILITY void pack_start(Widget& child, bool expand, bool fill = true, guint padding = 0); #endif void pack_start(Widget& child, PackOptions options = PACK_EXPAND_WIDGET, guint padding = 0); ... };
I checked randonly chosen the enums AccelFlags AnchorType ArrowType AttachOptions CalendarDisplayOptions EventType Orientation PackType PositionType ResizeMode SelectionMode TextDirection TextSearchFlags WidgetFlags and could not find any ambigious situation with enum parameters.
First answer my question.
> Examples fixed. > > Is this correct?: Beside of the two cases below all examples seem to be okay. Also no "layout disturbances" was found. $ examples/calendar/calendar [ after opening font selection ] (calendar:5921): gtkmm-WARNING **: gtkmm: Attempt to call Gtk::manage() on a Gtk::Window, but a Gtk::Window has no parent container to manage its lifetime. ./examples/tictactoe/ttt_test (ttt_test:5598): gtkmm-WARNING **: gtkmm: Attempt to call Gtk::manage() on a Gtk::Window, but a Gtk::Window has no parent container to manage its lifetime.
OK, I'll look at those examples, but my question was: Is this correct?: People who have never used Gtk::SHRINK, Gtk::FILL, etc, will have no problem with the current API?
> Is this correct?: > People who have never used Gtk::SHRINK, Gtk::FILL, etc, will have no > problem with the current API? Yes and no. People who never used Gtk::SHRINK, Gtk::FILL, etc, will not be concerned about the change from AttachOptions to PackOptions. But the overloading situations give no type safety at all. Every wrong integral type options parameter will evaluate to the wrong member function without any compiler warning.
A good point. Here are some reasons not to change it: - If we have a deprecated API then people will expect us to support _all_ of the deprecated GTK+ API. That's impossible at this stage. - We are source-API frozen, so we need a _really_ good reason to break the API by removing the bool, bool overrides. - The bool, bool overrides make it easy to port to gtkmm2. - If they get it wrong then they will at least see it at runtime. But I understand, and wish that it was typesafe at compile-time. By the way, why is this bug marked with the "portability" keyword?
> A good point. Here are some reasons not to change it: > ... > But I understand, and wish that it was typesafe at compile-time. I can agree with that - backward compatibility is an important point. What about the define proposal? This will not break the API. Such things, e.g._REENTRANT, _BSD_SOURCE, _POSIX_SOURCE , etc. are used quite often. > By the way, why is this bug marked with the "portability" keyword? It first looked so strange, that I could'nt believe it happens under Linux too. There was no information about the change from AttachOptions to PackOptions in the release notes.
re. the define proposal: - If we have a deprecated API then people will expect us to support _all_ of the deprecated GTK+ API. That's impossible at this stage. What would you suggest as an alternative to GTKMM_DEPRECATED?
> re. the define proposal: > - If we have a deprecated API then people will expect us to support > _all_ of the deprecated GTK+ API. That's impossible at this stage. I dont see the point here. The fact that the old form of set_options, pack_start, and pack_end member functions of the box class was kept for backward compatibility reasons does not mean we have to support all of the deprecated GTK+ API. Just let us deprecate them because they dont fit with the wanted type safety standard of gtkmm2. > What would you suggest as an alternative to GTKMM_DEPRECATED? Maybe GTKMM_USE_DEPRECATED makes it more clear. I suppose that both names would be used with #ifdef, meaning that the default case is type safe one. That would be great.
I think it would create confusion, and then disatisfaction. If we support any part of the deprecated API, or use a deprecation system such as that used in GTK+ then people will expect us to support all of the deprecated API. It should always be clear what part of GTK+ is wrapped, and what is not. If this is important to you then please discuss it concisely on the list. I think that the disadvantages are greater than the advantages.
I'm confused by your statement. In my eyes it was your wish to keep some old functions for backward compatibility reasons, not mine. My point was, that some of them break the type safety of the enum parameters of other overloaded functions. I agree that this bug should be closed now.
It's a pity that we didn't think of this before. It would have been easier not to add the bool, bool override than to remove it now. Punted.
What about this, the same principle as used in Gtk+? class Box { ... #ifndef GTKMM_DISABLE_DEPRECATED void pack_start(Widget& child, bool expand, bool fill = true, guint padding = 0); #endif void pack_start(Widget& child, PackOptions options = PACK_EXPAND_WIDGET, guint padding = 0); ... };
We have already discussed and rejected this. See above.
I talked with Murray about this problem and he agreed that it'd probably be OK to remove the 'bool fill = true' default argument from the legacy overload. This change was actually suggested by Jarek Dukat and Matthew Walton, so it seems we've a broad agreement on this issue. I'm closing this bug now since the overloading ambiguity is solved for the most common case, i.e. when not using the padding argument.
On 2nd thoughts, this comment seems like a good reason not to do it: >> People who have never used Gtk::SHRINK, Gtk::FILL, etc, will have no >> problem with the current API?
But, what the hell. That's logical, isn't it?