kxgames / glooey

An object-oriented GUI library for pyglet.
MIT License
91 stars 6 forks source link

Mover causes crashes when removed from the GUI #54

Open UplinkPhobia opened 2 years ago

UplinkPhobia commented 2 years ago

When trying to remove a Mover from the gui, I get the following crash:

[...]
File ".../glooey/scrolling.py", line 126, in on_mouse_leave
  x, y = self.screen_to_child_coords(x, y)
File ".../vecrec/shapes.py", line 71, in decorator
  return function(self, vector)
File ".../glooey/scrolling.py", line 190, in screen_to_child_coords
  mover_origin = self.rect.bottom_left
AttributeError: 'NoneType' object has no attribute 'bottom_left'

with this code

import pyglet
import glooey

window = pyglet.window.Window()
gui = glooey.Gui(window)
mover = glooey.Mover()
gui.add(mover)

@gui.event
def on_click(widget):
    gui.remove(mover)

pyglet.app.run()

This doesn't happen with a Frame, for example, so I'm guessing it's a bug, but I couldn't pinpoint the origin. Or maybe I'm doing something wrong, but it's a relatively straightforward code.

kalekundert commented 2 years ago

Thanks again for finding a bug and coming up with a really simple example. This is kinda a note to myself, but I think I found the problem. First, when the mover is removed from the GUI, its rect attribute is unset. This is intended behavior, and is meant to catch bugs. Second, when the mouse next moves after the mover is removed, the GUI notices that the mouse is no longer over the mover (when it previously was). This causes the GUI to issue the mover an on_mouse_leave event, even though the mover is no longer attached to the GUI at this point. This is also intended behavior, because it guarantees that there will be an on_mouse_leave event for every on_mouse_enter event. In this case, though, the program crashes because movers need their rect attributes in order to handle mouse events.

I'm not totally sure how to fix this. I could add an if self.rect is None check in Mover.on_mouse_leave(), but that would mean that widgets in Movers would not consistently get on_mouse_leave events. That seems likely to be a bad idea. I could change on_mouse_enter/leave events to not receive x/y coordinates. These events are mostly used to manage rollovers, which doesn't seem like it should require any coordinates. But the coordinates are currently used to avoid some artifacts, so I'd need to find another way to do that. I could also give widgets access to their "old rectangle" upon being removed from the GUI, to handle situations like this. That doesn't seem very clean, though.

UplinkPhobia commented 2 years ago

I'm not very familiar with how glooey works in details, so I'll give my point of view in a more abstract way.

If we consider the two points you gave, that are that:

the way I see it, it means that when the mover is removed, the removal itself should trigger the on_mouse_leave even to child widgets, since the mouse effectively left the mover (or, well, the mover left the mouse, if that makes sense, but from the children's point of view, it means the same). Then it would be possible to first dispatch the on_mouse_leave event, then set the rect to None. It would seem "clean" too, since we sort of "warn" the child widgets that removal is about to happen (through the event) before effectively removing the mover. However, I don't know how feasible this is, but it is my intuition from the little (and lacking) understanding I have.

kalekundert commented 2 years ago

I agree with your intuition, but the problem is that the on_mouse_leave event currently requires mouse coordinates, which are not known at the times when widgets are being removed. (Mouse coordinates are only known when a mouse event is being handled.) One way around this is what I mentioned above, which is to not provide the enter/leave events with coordinates. Another would be to have the GUI track the last-known mouse coordinates. I've resisted doing this before, because it just feels wrong, but it's something to consider.

UplinkPhobia commented 2 years ago

I might be mistaken, but I don't think pyglet "stores" mouse coordinates, at least the way I see it, it's made to only give coordinates during mouse events, so I agree that storing the mouse coordinates in the GUI seems a bit weird and doesn't really fit with pyglet. It wouldn't be horrible I suppose, but still not perfect.

I don't want to be annoying, but I am wondering, in the usual case (not the widget removal case), if the user leaves a widget with the mouse, it should trigger both the on_mouse_motion and on_mouse_leave events, or is the motion event not always triggered (for example if the mouse goes fast enough for it to "teleport" directly from inside the widget to outside, without moving inside the widget along the way, or if the mouse it at the exact edge of the widget and leaves without interacting with any other "widget pixel")? In the first case, it seems relatively straightforward to remove coordinates from on_mouse_leave events, as we are not trying to get coordinates but an "action" event (like on_click for example). In the second case however it indeed gets complicated, an storing the coordinates might be the lesser evil.

kalekundert commented 2 years ago

Definitely not annoying! I appreciate the discussion.

The way it works in the usual case is that when the GUI gets a mouse motion event, it works out which widgets are under the mouse and saves that information. The next time there's a mouse motion event, it does the same thing, and if it notices that there are any widgets that used to be under the mouse but aren't any more, it issues them an on_mouse_leave event. So the way I'd put it is that on_mouse_motion events (which basically come straight from pyglet) can trigger on_mouse_leave events depending on what was previously under the mouse. This is basically the "always triggered" scenario that you outlined above.

Having thought about this for a couple days now, I agree that the best solution is to remove the x,y coordinates from the on_mouse_enter/leave events, and to treat them like "action" events. The thing that was holding me up initially is that widgets update their rollover state in response to on_mouse_enter/leave events, and the function that updates rollover state needs x,y coordinates because it needs to know if the mouse is really over the widget (because widgets can customize their hitboxes). But none of this is a real problem. I know that the mouse is not over the widget during on_mouse_leave events, so I can add a way to update the rollover without needing coordinates in that case. (It is important for on_mouse_leave to update rollovers, otherwise buttons can get stuck in their "over" states.) I'll have to stop updating rollovers for on_mouse_enter events, but I don't think those updates were doing anything anyways, since every on_mouse_enter event should be accompanied by an on_mouse_motion event.