GNOME Bugzilla – Bug 135978
We use GdkEvent* instead of Gdk::Event in the API.
Last modified: 2017-10-14 07:29:07 UTC
We have a Gdk::Event C++ wrapper for GdkEvent*, but we don't use it yet. It might not be difficult.
Is API compatibility the issue here? I really like the idea of not exposing the GdkEvent structure directly within gtkmm (if for no other reason than that you have to actually go read the GTK documentation to figure out what a GdkEvent is -- it's not in the gtkmm documentation).
> Is API compatibility the issue here? Probably for at least some of the API - for instance, we can't add (virtual) default signal handlers without breaking ABI, and we certainly can't change the existing ones. Maybe there are some uses of GdkEvent in methods that we can just deprecate-and-replace. But one day we will have an opportunity to break API and I'd like to have this ready for that time. The GdkEvent structs are a hierarchy but they don't use the regular GObject class system - that's probably why we didn't wrap them so far. It might not be complicated though.
I also don't think it will be complicated. If necessary, we could even implement a zero-cost wrapper with identical memory layout.
Created attachment 220142 [details] [review] Gdk::Event: implement a class hierarchy representing different GdkEvent** types The above patch adds a separate class for each of the GdkEvent** enum types. The classes inherit from Gdk::Event and use its gobject_ pointer to access the GdkEvent structure. Each class provides accessors to all valid properties of the event, as well as to the underlying enum casted to the specific GdkEvent** type. The boilerplate code is generated using new tools/m4/class_gdkevent.m4 script. Existing scripts were not usable because the subclasses classes use gobject_ of the Gdk::Event class. A separate wrap_event() wrapper was added in addition to the usual wrap(). It wraps a C instance in such a way that no copy needs to be done when the owner of the object does not change. I guess it would be useful to have similar function for any class using _CLASS_BOXEDTYPE, though there are not many of them. The change is backwards-compatible. If this patch gets accepted, I think a possible further step would be to deprecate usage of the old GdkEvent** structs without waiting for an API break. We could suggest the users to use wrap_event()/wrap() wherever they receive an old event. When the gtkmm API is finally broken and Gdk::Event and friends are used everywhere, the only thing the users would need to do would be to remove the wrapping functions and change the prototypes of the signal handlers. Note, Gdk::Event::get and Gdk::Event::peek leaks memory on each call. I'll post a separate patch fixing this issue.
Is this something that should be implemented in gtkmm 4? Mark Vender has written an ambitious patch. It's worth a better fate than being forgotten.
Yes, I'd like to see this in gtkmm 4. It would need to be cleaned up a bit - for instance, there are some conversions that don't belong in the general .m4 file. I guess that the Gdk::Event would never be instantiated unless we are using a derived class, right? We wouldn't want to have lots of construction and destruction happening in code that runs so often.
Created attachment 352031 [details] [review] Gdk::Event: Implement a class hierarchy representing different GdkEvent* types This is a modified version of Mark's patch, suitable for gtkmm 4. I've kept him as the author. It's still at least 90% his code. Some differences from Mark's patch: 1. _WRAP_ENUM(EventType,GdkEventType) is not moved to the new gdk/src/enums.hg file. It's kept as _WRAP_ENUM(Type,GdkEventType) in the Gdk::Event class. 2. GdkEventVisibility is not wrapped. It's deprecated, and not used in gtk+ 4. 3. Unsigned data members (e.g. guint32 time) keep their types in gtkmm, instead of being cast to unsigned int, unsigned short and unsigned char. This is how unsigned data is usually handled in gtkmm and glibmm. (Don't ask me why gint is replaced by int, but guint is not replaced by unsigned int. Perhaps because gmmproc once had problems with type names consisting of more than one word.) 4. New _CONVERSION macros are put in event.hg instead of convert_gdk.m4. I'd like to make more modifications (in one or more separate patches) before I push this. 1. _GDKEVENT_INHERITS() is not necessary. _CLASS_GDKEVENT() can take either 2 or 4 parameters, 2 for the base class, 4 for the subclasses. _CLASS_GDKEVENT(C++ class, C class [, C++ base class, C base class]) Almost like _CLASS_GOBJECT() and _CLASS_GTKOBJECT(). 2. Use _WRAP_METHOD() instead of _MEMBER_GET() in some places, at least for gdk_event_get_event_type(). 3. The newest event types are not included in Mark's 5-year-old patch. Add TouchpadSwipe, TouchpadPinch, PadButton, PadAxis, PadGroupMode. 4. Events are usually received in signal handlers. It would be nice to be able to create a Gdk::EventXxx wrapper that does not own the GdkEvent that gobject_ points to. Compare Gtk::SelectionData_WithoutOwnership in selectiondata_private.h. It would save us from unnecessarily copying. If a signal handler wants to modify the event and send it to the next signal handler, it would be much easier if the GdkEvent is not copied. Subclassing each Gdk::EventXxx would double the number of subclasses. I would prefer to add a 'bool has_ownership_' member to Gdk::Event, analoguous to Glib::OptionContext. explicit EventKey(GdkEventKey* gobject, bool make_a_copy = false, bool take_ownership = false); 5. The subclasses don't need get_type() functions. They all call the same gdk_event_get_type() as the base class, Gdk::Event. 6. Add move constructor and move assignment. And then it remains to actually use these new subclasses of Gdk::Event.
Created attachment 352162 [details] Test case for Gdk::wrap_event() This program tests the Gdk::wrap_event() functions, which can be used when C++ signal handlers with Gtk::Event& parameters are called.
Created attachment 352163 [details] [review] patch: Gdk::Event: Remove _GDKEVENT_INHERITS() and other improvements This patch fixes some of the improvements, suggested in comment 7. 1. Remove _GDKEVENT_INHERITS(). 4. I had not studied Mark's patch well enough. His Gdk::wrap_event() is made for use in signal handlers. It's better than what I discussed in comment 7. But it was buggy, as can be demonstrated with the test case in comment 8. My patch fixes the bug, by replacing return reinterpret_cast<__CPPNAME__&>(*event); by return reinterpret_cast<__CPPNAME__&>(event); 5. Only one get_type(). 6. Move constructor and move assignment. Items 2 and 3 are not fixed yet, but I think that it's nevertheless okay to push the patches in comments 7 and 9 now. I'll do that unless someone objects soon.
I have pushed the patches in comments 7 and 9 (actually patches very similar to those patches). I have also pushed a patch that implements suggested improvements 2 and 3 from comment 7. Concerning implement 2, I noted that it's seldom possible to use _WRAP_METHOD in the subclasses. _WRAP_METHOD generates code that calls gobj() for the first parameter in the call to the gtk+ function. That's not possible here. E.g. Gdk::EventKey::gobj() returns a GdkEventKey*, but a typical gtk+ function takes a const GdkEvent*. Now I understand why Mark used _MEMBER_GET.
After a few more patches, Gdk::Event and its subclasses are now used consistently in gtkmm-4.0. It took about 13 years from the time this bug was filed.
Quick and probably silly question: What are the cases where it's useful to have the callbacks take a non-const Event& reference?
Quick answer: I dunno. I have made const references where gtk+ has const pointers, and non-const references where gtk+ has non-const pointers. There are probably many non-const references that ought to be const. As you probably know, gtk+ is not very const- correct. I have not taken the time to investigate each occurrence of a non-const pointer in gtk+, to see if the corresponding reference in gtkmm ought to be const.
I guess they should all be const. Do these Event* classes have any non-const methods that the handlers would be likely to call? Reopening until we resolve this last detail.
The Gdk::Event* classes have very few non-const methods. There are some that return a RefPtr: get_window(), get_screen(), get_seat(), get_device(), get_context() and perhaps others. As is common for such methods, there are both const and non-const versions. Neither Mark Vender nor I included _MEMBER_SET() macros. They can be added if anyone wants them, but I doubt that they are useful in signal handlers. Gtk+ contains many functions that take a non-const GdkEvent* pointer, some that can be called from event signal handlers. gtk_search_bar_handle_event() is called in gtkmm's icon browser demo. There are several examples in gtkmm-documentation of overridden default signal handlers that call the base class's signal handler. The default signal handlers in GtkWidget take non-const GdkEvent* pointers. Most of them probably don't modify the event, but shall we rely on that when it's not documented?
It would be good to check with GTK+ whether the lack of const is intentional. I have seen a few patches to constify things recently, so it happens. If the lack of const was not intentional, or is no longer needed, now is of course the time to get it changed there, then we can safely rely on it here. Of course, getting a wide-ranging patch into GTK+ purely because 'it is philosophically better this way' is usually not something that happens easily. But it seems to be worth asking anyway... if only to get a closed/unanswered bug report for reference of future people with the same question!
It's perhaps reasonable to have non-const event pointers or references in event signal handlers. The emitter of a signal must then accept that the event can be modified by one of the signal handlers. It's probably unusual, but a signal handler can modify the event and then return false. The next signal handler will handle the modified event. Should such tricks be forbidden? If the event signal handlers will get const events in the future, shall we modify the event methods that return a RefPtr? A signal handler shall be able to call non-const methods in e.g. the Gdk::Window that received the event, shouldn't it? Shall we then have a const method that returns a RefPtr to a non- const object? Glib::RefPtr<Gdk::Window> get_window() const;
Daniel Boles wrote: > It would be good to check with GTK+ whether the lack of const is intentional. I have seen a few patches to constify things recently, so it happens. Please feel free, but I don't think I would enjoy that. Part of the problem is that C developers generally don't understand how limiting the const keyword is in C, and think it's all about whether bytes will actually change, even when it can be implemented properly in C++ (with mutable, const methods, etc). Kjell Ahlstedt wrote: > The default signal handlers in GtkWidget take non-const GdkEvent* pointers I don't think that's a problem. People expect some const_cast<>ing when calling C functions. > It's probably unusual, but a signal handler can modify the event and then > return false. The next signal handler will handle the modified event. Should > such tricks be forbidden? I think it should be forbidden yes, unless we can find a good example of where a signal handler, or default signal handler, or vfunc, has some good reason to change the event that it's been sent. But, the problem here is us deciding what the constness means. Those get_window(), get_screen(), etc, methods are an issue.
Created attachment 354237 [details] [review] 0001-Take-all-Gdk-Event-and-subclasses-parameters-as-cons.patch This patch changes all the signals to send const & events and all the signal handlers to take them. It changes a few methods to do the same. However, see my following patch about Gdk::Cursor.
Created attachment 354238 [details] [review] 0001-Adapt-to-Event-classes-passed-by-const.patch This makes gtkmm-documentation build against gtkmm with the previous Event-as-const-ref patch. Everything builds just fine.
Created attachment 354241 [details] [review] 0002-Gdk-Cursor-Construct-from-a-const-Display.patch This patch shows a change to Gdk::Cursor that is necessary when the signals send events as const & (instead of non-const &), using the previous patch for gtkmm. I saw this problem in my Glom source code (simplified here), to show a drag cursor while dragging: void CanvasItemMovable::on_button_press_event(const Glib::RefPtr<Goocanvas::Item>& target, const Gdk::EventButton& event) { ... auto window = event.get_window(); auto display = window->get_display(); auto cursor = Gdk::Cursor::create(display, Gdk::Cursor::Type::FLEUR); ... } Gdk::Cursor::create() normally takes a Glib::RefPtr<Gdk::Display>, but here the display is a Glib::RefPtr<const Gdk::Display>. It's hard for me to say whether this is a good change. What does it mean for a Display to be const? Calling Display::close() or flush() definitely seem like changing the display. But is it changing the display to create a cursor that shows on that display? I think maybe not, but where do we draw the line, choosing which are const and which are not const. If we made changes like this, I think we'd have to make lots more.
If we let all event signal handlers receive const event references, it's perhaps best to have some unconventional get-methods in the event classes. Glib::RefPtr<Gdk::Window> get_window() const; Glib::RefPtr<Gdk::Screen> get_screen() const; etc. It would be easy to get a RefPtr to a non-const object anyway, either with a const_cast, or by making a non-const copy of the event and calling get_window() etc. on the copy.
Actually, I did recently encounter a case where it was advantageous - albeit hacky - to modify the event in its handler, then pass that event elsewhere. In this case I could just as easily have copied the event, and passed the copy to said function. However, I wonder whether there are legitimate (and hopefully less hacky) situations where it is needed to modify the same event that was received, i.e. if it still represents the current event as viewed by other observers - in cases where we cannot freely pass a copy of it instead. GDK is still largely mystifying to me, so I can't do much more than speculate.
Created attachment 355102 [details] [review] patch: Gdk::Event: Return RefPtr<NonConstClass> from a const Event If we decide to pass const Event& to all signal handlers, then this patch can be used in combination with the patches in comment 19 and comment 20. Then the patch in comment 21 (Gdk::Cursor) and similar patches would not be necessary.
Gtk+ contains the functions gdk_event_get_window(), gdk_event_get_screen(), gdk_event_get_seat(), gdk_event_get_device(), gdk_event_get_device_tool(). They all take a const GdkEvent* parameter and return a pointer to a non-const object. Gtk+ also contains gdk_event_put(const GdkEvent*) and gdk_event_request_motions( const GdkEventMotion*). This suggests that Gdk::Event::put() and Gdk::EventMotion::request_motions() should be const methods, as in the patch in the previous comment.
Can we decide what to do with the constness of Gdk::Event in signal handlers? I see two reasonable decisions: 1. Keep it as is: Let signal handlers take non-const Event&. 2. Apply the patches in comments 19, 20 and 24: Let signal handlers take const Event&. Return RefPtr<NonConstClass> from a const Event.
1 seems more reasonable, though maybe there are one or two that can be changed to const.
Created attachment 360729 [details] [review] patch: Take some Gdk::Event& (and subclasses) parameters as const& Is this an acceptable compromise? X event signal handlers that return bool still take a non-const Event&. Other signal handlers and most methods take a const Event& (or subclass of Event). In the attachment to comment 19, there is this comment to gesture.hg: // TODO: Use EventSequence by & and by value? Use by value is impossible. There is no public definition of GdkEventSequence. I haven't found _any_ definition of GdkEventSequence. In the gtk+ code it looks like GdkEventSequence* is an integer, cast to a pointer with GINT_TO_POINTER(). Strange! I don't understand why it's not an integer, such as typedef GdkEventSequenceId int; Perhaps it's a trick to force the compiler to treat GdkEventSequence* and int as incompatible types.
Gtk+ has made GdkEvent and its subclasses opaque (inaccessible to us) in the master branch. _MEMBER_GET() can't be used in events.hg. There are about 100 of them. I'm making events.hg and events.ccg compatible with the latest gtk+, but it might take a few days.
I have pushed the patch in comment 28, excluding the changes in event.hg and events.hg. Those two files were modified when GdkEvent became opaque.
Comment on attachment 355102 [details] [review] patch: Gdk::Event: Return RefPtr<NonConstClass> from a const Event A _MEMBER_GET_NONCONST_GOBJECT() macro can't be used now that GdkEvent has become an opaque struct. And its functionality is not needed when most Gdk::Event& parameters are non-const.