Your use case and alternative implementation make sense.

The current method seems odd. I don't have the repo handy so can't check why this was added or whether it exists for some internal purpose. It seems like a bit of an odd one out, that doesn't exist in GtkWindow and should probably exist at some lower level than gtkmm's Gtk::Window, being as it takes a GObject.

A problem of simply replacing the current method in Gtk::Window is that it would break API/ABI for anyone currently using it - if they have much reason to do so.

That said, if it turns out that the current implementation is just plain wrong, and a trivial fix is to change to e.g. Widget::get_accel_groups()[0], then an API change _may_ get into 2.52 - but hurry with a patch, as it's imminent for release! :) And that said, it's not my call whether it would be accepted for 2.52

(In reply to Daniel Boles from comment #1)
> That said, if it turns out that the current implementation is just plain
> wrong, and a trivial fix is to change to e.g. Widget::get_accel_groups()[0],
> then an API change _may_ get into 2.52 - but hurry with a patch, as it's
> imminent for release! :) And that said, it's not my call whether it would be
> accepted for 2.52

Hmm, I seem to have forgotten which library we were talking about halfway through. So I doubt the existing, albeit not very useful, method can be removed in gtkmm 3.22. However, it's possible that a proper one can be added, if it represents something that was simply missing; the restrictions on changing API in this version have been relaxed somewhat recently.

Perhaps we could add new API for get_accel_groups(), and deprecate the custom get_accel_group(), as it's not really useful.

Although GTK+'s get_accel_groups() is a bit of a strange one, and can in theory take any GObject, IMO we should just put it in Gtk::Window until anyone provides a rationale for it being needed for any other kind of widget/GObject.


(In reply to Maurice van der Pot from comment #0)
> The problem I have with the current implementation of
> Window::get_accel_group is:
> * glade requires a top-level window if you want to specify accelerators
> * glade creates a new accelerator group for that top-level window when the
> first accelerator is added
> * when I want to use my own subclass of Gtk::Window in my application, I
> would like to move the existing Gtk::AccelGroup from the window glade
> created to my own window
> * but currently I cannot access it

Well, yes, you can, by tolerating the existence of some C code in your project:

  auto glist = gtk_accel_groups_from_object( gtkmm_window.gobj() );

and then use GList API directly

- or even use a glibmm helper to get a yourself vector - along the same lines as this (untested) idea of what the proposed new method would do:

 std::vector< Glib::RefPtr<AccelGroup> > Window::get_accel_groups()
 {
   return Glib::ListHandler< Glib::RefPtr<AccelGroup> >
   ::list_to_vector(gtk_accel_groups_from_object( gobj() ),
                    Glib::OWNERSHIP_SHALLOW);
 }

Can _WRAP_METHOD handle cases like this, where the instance's .gobj() must be converted to a GObject*, or will that have to be coded manually?

Created attachment 359737 [details] [review]
Window: Move to a proper get_accel_groups()

The existing get_accel_group() just creates a per-window AccelGroup on
demand and returns it. That’s no use if you need groups that were added
by other sources, e.g. by Gtk::Builder, Glade, etc.

Deprecate that, in favour of a new get_accel_groups(), which wraps
gtk_accel_groups_from_object() to return the actual set of AccelGroups.

Note that gtk_accel_groups_from_object() takes a GObject, which it says
is usually a GtkWindow. I therefore assume users will only want to get
groups from a Window, instead of putting this in Glib::Object or such.
Of course, file a bug if you want to get AccelGroups from another class.

While here:
 • Explicitly #include <glibmm/vectorutils.h> as used by this and others
 • Add a . to the Doxygen comment to tell it the 1st line is a summary
 • Don’t say the default group can’t be removed; I see no reason why not
 • Remove speculative comments on the old code, now explained elsewhere.

--

assuming a no to the above, if only to upload what I wrote for this so far.

> Can _WRAP_METHOD handle cases like this, where the instance's .gobj() must be
> converted to a GObject*, or will that have to be coded manually?

_WRAP_METHOD can be used, but it requires a complicated _CONVERSION().
This is what can (but IMO should not) be done:

  #m4 _CONVERSION(`GSList*', `std::vector<Glib::RefPtr<AccelGroup>>', `Glib::SListHandler<Glib::RefPtr<AccelGroup>>::slist_to_vector(gtk_accel_groups_from_object(Glib::Object::gobj()), Glib::OWNERSHIP_NONE)')
    _WRAP_METHOD(std::vector<Glib::RefPtr<AccelGroup>> get_accel_groups(), gtk_accel_groups_from_object)
  #m4 _CONVERSION(`GSList*', `std::vector<Glib::RefPtr<const AccelGroup>>', `Glib::SListHandler<Glib::RefPtr<const AccelGroup>>::slist_to_vector(gtk_accel_groups_from_object(const_cast<GObject*>(Glib::Object::gobj())), Glib::OWNERSHIP_NONE)')
    _WRAP_METHOD(std::vector<Glib::RefPtr<const AccelGroup>> get_accel_groups() const, gtk_accel_groups_from_object)

It's not simpler than the manually coded version.

> I therefore assume users will only want to get groups from a Window, instead
> of putting this in Glib::Object or such.

Don't put it in Glib::Object. It returns a std::vector<Glib::RefPtr<
Gtk::AccelGroup>>. Glibmm shall not contain any kind of reference to gtkmm.

A recent conversation on gtkmm-list makes it unlikely that new API will be
added to gtkmm 3.22 in the near future.
https://mail.gnome.org/archives/gtkmm-list/2017-September/msg00008.html
But the new get_accel_groups() can be added to gtkmm 4 in the master branch,
and the old get_accel_group() can be deleted there.

Comments to your patch:
Ownership shall be Glib::OWNERSHIP_NONE.
Why do you include vectorutils.h in the .hg file? It's needed only in the .cc
file, and could be included in the .ccg file instead.

Review of attachment 359737 [details] [review]:

(In reply to Kjell Ahlstedt from comment #6)
> _WRAP_METHOD can be used, but it requires a complicated _CONVERSION().
> [...]
> It's not simpler than the manually coded version.

Right, yikes. Thanks for showing how bad it would be, though!


> > I therefore assume users will only want to get groups from a Window, instead
> > of putting this in Glib::Object or such.
> 
> Don't put it in Glib::Object. It returns a std::vector<Glib::RefPtr<
> Gtk::AccelGroup>>. Glibmm shall not contain any kind of reference to gtkmm.

True, that's a much simpler argument than the fact that it'll probably not be wanted.


> A recent conversation on gtkmm-list makes it unlikely that new API will be
> added to gtkmm 3.22 in the near future.
> https://mail.gnome.org/archives/gtkmm-list/2017-September/msg00008.html
> But the new get_accel_groups() can be added to gtkmm 4 in the master branch,
> and the old get_accel_group() can be deleted there.

OK, I'll prepare a simpler patch that just replaces it outright for gtkmm-4.

Those using gtkmm-3 in the meantime will have to do the conversion themselves, or just deal directly with the GList, as noted in a previous comment.


> Comments to your patch:
> Ownership shall be Glib::OWNERSHIP_NONE.

I'm not really familiar with the list/array conversions. I just used _SHALLOW because it was what other conversions from a GList to a vector<RefPtr> did. Can you explain why they should differ?


> Why do you include vectorutils.h in the .hg file? It's needed only in the .cc
> file, and could be included in the .ccg file instead.

I think that's simply a leftover from when I was trying to do the m4 conversion in the header, as discussed.

OWNERSHIP_SHALLOW is certainly most common. E.g. Gtk::Window::get_icon_list().
It corresponds to gtk+'s [transfer container], i.e. the called gtk+ function
creates a new GList or GSList and populates it with pointers to existing
elements. The caller shall free the container but not its elements.

An example of OWNERSHIP_NONE: Gtk::Application::get_windows(). It corresponds to
[transfer none], i.e. the gtk+ function returns a pointer to an existing GList
or GSList. The caller shall not free anything.

An example of OWNERSHIP_DEEP: Gtk::Application::list_action_descriptions(). It
corresponds to [transfer full], i.e. the gtk+ function creates a new GList or
GSList and populates it with pointers to newly created elements. The caller
shall free both the container and its elements.

Thanks for that. So those correspond 1:1 to GObject's transfer-container, transfer-none, and transfer-full respectively.

And indeed, the return value of gtk_accel_groups_from_object() is annotated as transfer-none. So we should match that in gtkmm.

I just missed that detail. It would've been more obvious if the return was a const GList*. But I don't know if there's some reason that GTK+ does not qualify it as const. It's not like the C libraries are particularly good about const-correctness, either way.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtkmm/issues/18.