robhagemans / pcbasic

PC-BASIC - A free, cross-platform emulator for the GW-BASIC family of interpreters
http://www.pc-basic.org
Other
397 stars 48 forks source link

PEN ON never works #126

Closed incanus closed 2 years ago

incanus commented 3 years ago

Problem

Using PEN ON never seems to actually enable mouse event polling.

Steps

Try the program below. It behaves the same whether PEN ON has been called or not; it never prints.

Program

10 SCREEN 1 : CLS
20 PEN ON
30 PRINT PEN(4);",";PEN(5)
40 PLAY "P64"
50 GOTO 30

Notes

PC-BASIC version: 2.0.3 Operating system version: macOS 11.0.1 Beta

This appears to come down to the fact that basicevents.py sets up the EventHandler superclass to have a self.enabled flag. This isn't used by any of the subclasses (PlayHandler, TimerHandler, KeyHandler, etc.). However, in implementation.py's pen response, pen_fn_(), it is passed (as self.basic_events.pen.enabled) into pen.py's poll(), which always executes:

if not enabled:
    # should return 0 or char pos 1 if PEN not ON
    return 1 if fn >= 6 else 0

If you modify BasicEvents::command() as follows:

 """Turn the event ON, OFF and STOP."""
 if command_char == tk.ON:
     self.enabled.add(handler)
+    handler.enabled = True
     handler.stopped = False

It works properly. However, a mod is needed to properly handle PEN OFF as well, again in BasicEvents::command():

 elif command_char == tk.OFF:
     # we seem to need to keep ComHandler around to make serial events work correctly
     # i.e. apparently they can be triggered when switched off - I don't understand why
     if not isinstance(handler, ComHandler):
+    handler.enabled = False
     self.enabled.discard(handler)

I'm happy to submit a PR if that is easier. I dug around for legacy changes to see why this might have gotten this way, but didn't find any clues so I figured I'd start a conversation first.

robhagemans commented 3 years ago

Hi, thanks! I suspect you're right about the cause of the problem. I don't quite recall the history of this - other than that it is long and painful ;). I recall this module being a major pain in trying to sanitise the circular dependencies and global state that plagued the v1.2 code. However if it's not used anywhere else it looks to me like someone (i.e. me) aimed to remove the .enabled member but didn't quite complete the task...

Given that this information is also in self.basic_events.enabled it might still be best to replace self.basic_events.pen.enabled with self.basic_events.pen in self.enabled, however I need to have a closer look at what exactly is going on with .enabled.

robhagemans commented 2 years ago

Fixed on develop by commit 046696478, thanks @incanus for reporting!