search

sidorares / node-x11

X11 node.js network protocol client
MIT License
518 stars 72 forks source link

Bad mapping for event 17 : "DestroyNotify" #162

Open izanagi1995 opened 6 years ago

izanagi1995 commented 6 years ago

I'm creating a Window Manager with your awesome module.

I had a bug with the "DestroyNotify" event. The event_consumer of the window was never triggered. It's because the real wid is in the field event. I don't know why but the wid in this case is the same as the "MapNotify" event but when I try X.DestroyWindow(ev.wid), it's yield a "BadWindow" error. But when I do X.DestroyWIndow(ev.event), the window is destroyed successfully.

santigimeno commented 6 years ago

According to the spec:

This event is reported to clients selecting StructureNotify on the window and to clients selecting SubstructureNotify on the parent. It is generated when the window is destroyed. The event is the window on which the event was generated, and the window is the window that is destroyed.

My understanding of this is:

IIUC what you're doing, when you call X.DestroyWindow(ev.wid) you're trying to destroy an already destroyed window while when you call X.DestroyWIndow(ev.event) you're trying to destroy its parent and that probably works ok.

izanagi1995 commented 6 years ago

Ok then... There is still a bug somewhere in the windowmanager example and the core in general. The DestroyNotify event is not passed to the event_consumer. Without the trick X.DestroyWIndow(ev.event), the window is still there without content and to close it, I have to call the destroy with ev.event...

IMO, this should be changed to allow the event_consumer to destroy the window.

EDIT : To give you the full context, I'm launching an app through terminal and then kill it with Ctrl-C and try to get the window closed.

sidorares commented 6 years ago

hi @izanagi1995 I'm in 'holiday mode' currently and read notifications diagonally. Can you post code snippet with "what's expected" vs "what's happening"? If you strongly believe your expectations are correct, maybe even create PR which adds your assertion as test case ( obviously currently failing)