GNOME Bugzilla – Bug 780004
FileChooserDialog: Add a constructor for the use-header-bar property
Last modified: 2018-11-08 12:44:38 UTC
Dialog:use-header-bar is a construct-only property, but the gtkmm wrapper provides no way to pass this through to the underlying GtkFileChooser during construction. It would be great if there could be a constructor overload taking a boolean or, perhaps better, a Gtk::DialogFlags enum offering the possibility to set this. The latter could just be propagated to the parent Dialog ctor, which already offers a ctor taking DialogFlags. I'm happy to write a patch if the idea is accepted. It'd be nice to get this into 3.22 too, though I appreciate that at best, it would have to go on the pile of 'hmmm, maaayyybe' ideas that also involve new API. See https://bugzilla.gnome.org/show_bug.cgi?id=738513 for an example of such a constructor being added for Gtk::Assistant - albeit the boolean option.
For my current needs (uniform look/layout across platforms) I'd also request the same for AboutDialog. I can open another bug if you like, or alternatively we could make this a catch-all if the fix is simple enough in all cases.
OK. I'm trying this out now, simplifying the set of existing constructors too.
I have done this for Dialog: https://git.gnome.org/browse/gtkmm/commit/?id=e4b7bf07d81b3e6598237605b5c07fdc602613b9 and for AboutDialog: https://git.gnome.org/browse/gtkmm/commit/?id=886f6b816700493848751dd305f003655ca3de77 But I wonder if we should have the full set of constructors and their parameters for AboutDialog. The inconsistency seems odd.
Thanks a lot! Would you like me to work on FileChooser, or did you already start? (In reply to Murray Cumming from comment #3) > But I wonder if we should have the full set of constructors and their > parameters for AboutDialog. The inconsistency seems odd. Yeah, it did seem a bit strange to me that it only had a default constructor. Would it make sense to have a single constructor with (bool use_header_bar = false), which could therefore also function as the default ctor, or do we need to keep them separate?
As another tangent (sorry!), I was also curious about the explicit keyword here: > explicit Dialog(const Glib::ustring& title, bool modal = false, bool use_header_bar = false); I'm mostly ignorant when it comes to this keyword, except for its purpose in single-argument constructors, preventing implicit conversion from other types. But what benefits does it provide for a multi-arg ctor in a case like this?
(In reply to Daniel Boles from comment #4) > Thanks a lot! > > Would you like me to work on FileChooser, or did you already start? Please go ahead. > Would it make sense to have a single constructor with (bool use_header_bar = > false), which could therefore also function as the default ctor, or do we > need to keep them separate? Possibly. Let's do that as one separate patch, if we decide to do it. > I'm mostly ignorant when it comes to this keyword, except for its purpose in > single-argument constructors, preventing implicit conversion from other types. > But what benefits does it provide for a multi-arg ctor in a case like this? If the other parameters have default values then it can be a single-argument constructor. If you find one that can't take just one argument then it's a mistake.
Created attachment 348749 [details] [review] FileChooserDialog: Tweak spacing for readability [I find this easier to work with, but if you disagree and/or consider such changes to be git noise, then nevermind, and I can rebase the other ones.]
Created attachment 348750 [details] [review] FileChooserDialog: Drop unneeded explicit on ctor This is only useful to us if the constructor can be invoked with one argument (i.e. it only has 1, or the subsequent have default values). That is not true here, so the explicit keyword is a semantic mistake. [as per Murray's reply in Comment 6]
Created attachment 348751 [details] [review] FileChooserDialog: Use static_cast, not C cast static_cast is safer and better C++ style.
Created attachment 348752 [details] [review] FileChooserDialog: Allow constructing with use-header-bar This property is handy e.g. for ensuring consistent UI across platforms. This is a construct-only property, so we must specifically implement it. Doing so is free, by adding it as a defaulted argument to our remaining ctors, with the default value of false equivalent to what we got before.
Review of attachment 348752 [details] [review]: ::: gtk/src/filechooserdialog.ccg @@ +29,3 @@ _CONSTRUCT("title", title.c_str(), + "action", static_cast<GtkFileChooserAction>(action), + "use-header-bar", static_cast<int>(use_header_bar)) I just realised this would be a good candidate for C++14 delegating constructors: this constructor could delegate to the other, as it otherwise reimplements the same stuff. Do you think that is worth doing? It seems like a nice way to incorporate C++14 while reducing code volume (improving D.R.Y.)
Review of attachment 348749 [details] [review]: No, thanks, I'd rather not have lots of these superficial changes. I'd be happy for us to just run clang-format once on the .hg/.ccg files if someone can make that work.
Review of attachment 348750 [details] [review]: Thanks.
Review of attachment 348751 [details] [review]: Thanks. Maybe someone would like to make similar fixes throughout the .ccg files, in some large commits.
Review of attachment 348752 [details] [review]: Thanks.
Thanks for the quick reviews, Murray. So I'll rebase without the spacing changes, then push to master. I'll also file this in my 'stuff for a hypothetical gtkmm-3-24' list! Do you have any thoughts on the idea of using C++14 delegating constructors? There is an opportunity here, as the _CONSTRUCT() call is the same between them both, so we can define that only in the without-parent ctor and have the with-parent one delegate to it. But I don't know whether you want to use this pattern.
Comment on attachment 348750 [details] [review] FileChooserDialog: Drop unneeded explicit on ctor pushed, without the spacing changes, to master and gtkmm-3-22
Comment on attachment 348751 [details] [review] FileChooserDialog: Use static_cast, not C cast pushed, without the spacing changes, to master and gtkmm-3-22
Comment on attachment 348752 [details] [review] FileChooserDialog: Allow constructing with use-header-bar pushed, without the spacing changes, to master
(In reply to Daniel Boles from comment #17) > Thanks for the quick reviews, Murray. So I'll rebase without the spacing > changes, then push to master. > > I'll also file this in my 'stuff for a hypothetical gtkmm-3-24' list! > > Do you have any thoughts on the idea of using C++14 delegating constructors? These were in C++11, right? Or is there something new for this in C++14? > There is an opportunity here, as the _CONSTRUCT() call is the same between > them both, so we can define that only in the without-parent ctor and have > the with-parent one delegate to it. But I don't know whether you want to use > this pattern. Yes, I'd like to use this where possible, throughout our code. It would be great to have some patches in a new bug. Thanks.
(In reply to Murray Cumming from comment #21) > > Do you have any thoughts on the idea of using C++14 delegating constructors? > > These were in C++11, right? Or is there something new for this in C++14? Oh, you're right. (I think sometimes I can't believe just how much stuff C++11 added, so I end up assuming some of the things were C++14!) > Yes, I'd like to use this where possible, throughout our code. It would be > great to have some patches in a new bug. Thanks. Great! I'll try to file bugs for this and the other ideas about static_cast and clang-format later.
I'm trying to dig up gtkmm patches that might be eligible for gtkmm-3-22 now that Murray indicated adding (and presumably deprecating API) is a possibility there. After some quick thoughts about the factors, this one seems tricky: * We can't just add the new argument without keeping the old one around, as that would break ABI of programs compiled with the old signature. * And adding a new overload with the default argument would create unresolvable ambiguity. * Perhaps this could be pushed with the new argument but with no default value on it? This would mean there would be 2 overloads usable to construct without a HeaderBar, which seems a bit clumsy, but having both overloads available could help people prepare for migrating to GTK+ 4. Any thoughts?
I have pushed one of Murray's patches to the gtkmm-3-24 branch, adding the AboutDialog(bool use_header_bar) constructor. His Dialog patch breaks ABI, and it's not as necessary. The constructors with a DialogFlags parameter offer the same functionality. I haven't added anything to FileChooserDialog in the gtkmm-3-24 branch. I wonder if it's safe to do so. If I add FileChooserDialog(Gtk::Window& parent, const Glib::ustring& title, FileChooserAction action, bool use_header_bar); FileChooserDialog(const Glib::ustring& title, FileChooserAction action, bool use_header_bar); can some strange user code become ambiguous? The deprecated constructors FileChooserDialog(Gtk::Window& parent, const Glib::ustring& title, FileChooserAction action, const Glib::ustring& backend); FileChooserDialog(const Glib::ustring& title, FileChooserAction action, const Glib::ustring& backend); differ from the wanted new ones only in the last parameter. Can anything be implicitly converted either to bool or to const Glib::ustring&, and can the compiler find the two conversions equally good?
It seems that char pointers probably do not do what the user expects, as they convert preferentially to bool, not Glib::ustring. The following program prints `f(bool)`: #include <glibmm/ustring.h> #include <iostream> void f(bool const b) { std::cout << "f(bool)\n"; } void f(Glib::ustring const& u) { std::cout << "f(Glib::ustring const&)\n"; } int main() { f("hello"); return 0; }
Thanks for testing this. Your result shows that constructors with an extra bool parameter must not be added. Even though the result is not what we would like in this case, it's reasonable. A pointer to bool conversion is an implicit conversion. A const char* to const ustring& conversion is a user-defined conversion. An implicit conversion is considered better than a user-defined conversion during overload resolution.
I guess a way to get this into gtkmm-3-24 without ambiguity would be to add a constructor taking a tag struct as another argument before the new bool? Alternatively, it seems possible to add a constructor that swaps one of the other (non-ambiguous) arguments for use_header_bar, and then change that dropped argument using set_*(), but this seems (even more?) clumsy.
Would this be acceptible? FileChooserDialog(Gtk::Window& parent, const Glib::ustring& title, FileChooserAction action, bool use_header_bar, int dummy); FileChooserDialog(const Glib::ustring& title, FileChooserAction action, bool use_header_bar, int dummy); That can't be ambiguous, can it? It's not nice, but it's perhaps the best we can do in gtkmm-3-24. It's reasonably similar to the constructors in gtkmm-4.0.
I think an empty struct tag would be nicer, more self-documenting, and less strange/ugly looking in end-user code than a dummy argument. Come to think of it, we could just pass such a tag on its own, not with a bool, since the only reason to call the proposed new constructor would be if you *did* want a HeaderBar, as the old one always presumes you don't. e.g.: > FileChooserDialog(const Glib::ustring& title, FileChooserAction action, Gtk::UseHeaderBarTag); Then the user just calls like > auto dialog = Gtk::FileChooserDialog( "Select file", Gtk::ACTION_OPEN, Gtk::UseHeaderBarTag{} ); This complicates cases where whether or not to use a HeaderBar is dynamic, and the bool argument would be nicer, but I might guess those are relatively rare, in which case it seems better to pick the option that creates less odd looking code.
Next idea: FileChooserDialog(Gtk::Window& parent, const Glib::ustring& title, FileChooserAction action, DialogFlags use_header_bar); FileChooserDialog(const Glib::ustring& title, FileChooserAction action, DialogFlags use_header_bar); Compare Gtk::Dialog in gtkmm3. But here in FileChooserDialog only the Gtk::DIALOG_USE_HEADER_BAR flag is used. I suppose it would be possible to use all three DialogFlags, but I don't want to add functionality that does not exist in FileChooserDialog in gtkmm4.
That's not the worst option, although it would be better to add support for all the flags, instead of having to document that only one is actually listened to. Another, perhaps simpler option is just to shuffle the order of arguments, to e.g. (title, use_header_bar, action). But maybe it's not desirable to have constructors between gtkmm 3 and 4 that have the same arguments in different orders.
Created attachment 374143 [details] [review] Gtk::FileChooserDialog: Allow constructing with use-header-bar (gtkmm-3-24) Is this patch good enough?
Daniel, is my patch in comment 32 acceptable to you? This is, I hope, the last fix for gtkmm 3.24.0. Or do you know any other fixes that should go into that release?
It looks like the best solution and of course looks correct, but I didn't personally test it, though it's hard to see what could possibly go wrong (famous last words!) One stylistic question I wonder about, although it's most likely just my lack of experience: why set some properties in the _CONSTRUCT() call and others using mm API, and not do all in one place or the other? Is there a requirement or preference deciding which properties get set where? Of course, :use-header-bar is construct only, so it must go there, but e.g. :action can be set at any time. So I just wondered what the 'rules' are for such properties.
I copied some code from Gtk::Dialog without thinking much about it. The constructors with a 'flags' parameter in Dialog once called gtk_dialog_new_with_buttons(), and that function sets only the use-header-bar property in the call to g_object_new(). Probably I was too much influenced by gtk_dialog_new_with_buttons(). All properties can be set in _CONSTRUCT(). I've pushed a modified patch that does that. I also tested it by modifying the book/dialogs/filechooserdialog example program in gtkmm-documentation.