After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 135978 - We use GdkEvent* instead of Gdk::Event in the API.
We use GdkEvent* instead of Gdk::Event in the API.
Status: RESOLVED FIXED
Product: gtkmm
Classification: Bindings
Component: gdkmm
2.4
Other All
: Normal normal
: 3
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2004-03-02 15:58 UTC by Murray Cumming
Modified: 2017-10-14 07:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Gdk::Event: implement a class hierarchy representing different GdkEvent** types (44.39 KB, patch)
2012-08-02 11:35 UTC, Mark Vender
none Details | Review
Gdk::Event: Implement a class hierarchy representing different GdkEvent* types (42.34 KB, patch)
2017-05-17 14:18 UTC, Kjell Ahlstedt
committed Details | Review
Test case for Gdk::wrap_event() (978 bytes, text/plain)
2017-05-19 14:38 UTC, Kjell Ahlstedt
  Details
patch: Gdk::Event: Remove _GDKEVENT_INHERITS() and other improvements (26.60 KB, patch)
2017-05-19 14:41 UTC, Kjell Ahlstedt
committed Details | Review
0001-Take-all-Gdk-Event-and-subclasses-parameters-as-cons.patch (29.59 KB, patch)
2017-06-22 10:33 UTC, Murray Cumming
needs-work Details | Review
0001-Adapt-to-Event-classes-passed-by-const.patch (19.88 KB, patch)
2017-06-22 10:34 UTC, Murray Cumming
needs-work Details | Review
0002-Gdk-Cursor-Construct-from-a-const-Display.patch (3.65 KB, patch)
2017-06-22 10:48 UTC, Murray Cumming
none Details | Review
patch: Gdk::Event: Return RefPtr<NonConstClass> from a const Event (7.42 KB, patch)
2017-07-07 16:14 UTC, Kjell Ahlstedt
rejected Details | Review
patch: Take some Gdk::Event& (and subclasses) parameters as const& (11.33 KB, patch)
2017-10-01 17:03 UTC, Kjell Ahlstedt
committed Details | Review

Description Murray Cumming 2004-03-02 15:58:45 UTC
We have a Gdk::Event C++ wrapper for GdkEvent*, but we don't use it yet. It
might not be difficult.
Comment 1 Jonathon Jongsma 2006-01-29 05:29:54 UTC
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).
Comment 2 Murray Cumming 2006-01-29 07:58:52 UTC
> 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.
Comment 3 Daniel Elstner 2009-06-12 16:56:47 UTC
I also don't think it will be complicated. If necessary, we could even implement a zero-cost wrapper with identical memory layout.
Comment 4 Mark Vender 2012-08-02 11:35:49 UTC
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.
Comment 5 Kjell Ahlstedt 2017-03-29 14:26:50 UTC
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.
Comment 6 Murray Cumming 2017-03-30 10:01:13 UTC
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.
Comment 7 Kjell Ahlstedt 2017-05-17 14:18:49 UTC
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.
Comment 8 Kjell Ahlstedt 2017-05-19 14:38:38 UTC
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.
Comment 9 Kjell Ahlstedt 2017-05-19 14:41:51 UTC
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.
Comment 10 Kjell Ahlstedt 2017-05-26 16:37:32 UTC
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.
Comment 11 Kjell Ahlstedt 2017-05-31 14:44:56 UTC
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.
Comment 12 Daniel Boles 2017-05-31 15:03:34 UTC
Quick and probably silly question: What are the cases where it's useful to have the callbacks take a non-const Event& reference?
Comment 13 Kjell Ahlstedt 2017-05-31 17:46:23 UTC
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.
Comment 14 Murray Cumming 2017-06-19 12:30:19 UTC
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.
Comment 15 Kjell Ahlstedt 2017-06-20 14:26:30 UTC
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?
Comment 16 Daniel Boles 2017-06-20 14:33:54 UTC
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!
Comment 17 Kjell Ahlstedt 2017-06-21 13:56:52 UTC
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;
Comment 18 Murray Cumming 2017-06-22 10:30:34 UTC
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.
Comment 19 Murray Cumming 2017-06-22 10:33:18 UTC
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.
Comment 20 Murray Cumming 2017-06-22 10:34:36 UTC
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.
Comment 21 Murray Cumming 2017-06-22 10:48:09 UTC
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.
Comment 22 Kjell Ahlstedt 2017-06-22 17:06:26 UTC
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.
Comment 23 Daniel Boles 2017-07-07 15:08:58 UTC
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.
Comment 24 Kjell Ahlstedt 2017-07-07 16:14:38 UTC
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.
Comment 25 Kjell Ahlstedt 2017-07-29 07:42:38 UTC
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.
Comment 26 Kjell Ahlstedt 2017-09-29 14:58:59 UTC
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.
Comment 27 Murray Cumming 2017-09-29 16:27:10 UTC
1 seems more reasonable, though maybe there are one or two that can be changed to const.
Comment 28 Kjell Ahlstedt 2017-10-01 17:03:18 UTC
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.
Comment 29 Kjell Ahlstedt 2017-10-04 13:01:24 UTC
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.
Comment 30 Kjell Ahlstedt 2017-10-14 07:21:12 UTC
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 31 Kjell Ahlstedt 2017-10-14 07:29:07 UTC
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.