Skip to content

Conversation

GregStanton
Copy link
Contributor

@GregStanton GregStanton commented Jul 23, 2023

THE REASON FOR THIS CHANGE
The previous interface caused confusion. For example, setObjectIsHovered might be interpreted as a setter for a Boolean (which it is not), rather than as a setter for a method that returns a Boolean (which it is). This problem was surfaced by pull request #9.

CRITERIA FOR THE NEW INTERFACE
The interface in this PR is intended to alleviate confusion by satisfying the following design criteria:

  • flexible enough to handle a wide range of events
  • intuitive enough for beginners
  • consistent enough with p5

THE NEW INTERFACE
Here's the new interface:

setEventDetector(type, detector)
setEventResponder(type, responder)

  • type: a string (e.g. ‘mousepressed’ and ‘mousereleased’)
  • detector, responder: callback functions for the listener and handler, respectively1

Note that the type parameter supports the exact names of p5 handlers, such as mousePressed().2 When there's no corresponding event handler in p5, we'll typically match Web API names (e.g. ‘mouseover’ and ‘mouseout’), but we may choose our own names if there's a good reason to do so. This pull request also renames some internal variables using a more intuitive naming scheme that matches the new interface.

COMPARISON TO OTHER INTERFACES
The approach in this pull request is similar to the addEventListener() method of the EventTarget Web API and the .on() method of Konva.js. One major difference is that setEventDetector() and setEventResponder() are attached to a Mathemagical interaction object, rather than the ultimate event target (a drawing object).

This way, the user only needs to create a single interaction object in order to apply multiple listeners and handlers (e.g. for a draggable square, multiple listeners and handlers are used to change the cursor on mouseover and mouseout events, and also to change the position on drag events).

Footnotes

  1. "Detector" and "responder" are hopefully more intuitive to beginners than "listener" and "handler."

  2. Since event types are passed as strings, they're all lowercase, but otherwise they have the same names as p5 event handlers when applicable.

The previous interface caused confusion. For example, setObjectIsHovered might be interpreted as a setter for a Boolean, rather than a setter for a method that returns a Boolean.

CRITERIA FOR NEW INTERFACE
The interface in this PR is intended to alleviate confusion by satisfying the following design criteria:

* flexible enough to handle a wide range of events
* intuitive enough for beginners 
* consistent enough with p5

NEW INTERFACE
setEventDetector(type, detector)
setEventResponder(type, responder)

The type parameter accepts strings that match p5's names when they exist (e.g. ‘mousepressed’ and ‘mousereleased’) and they typically match Web API names when they do not (e.g. ‘mouseover’ and ‘mouseout’). We may create our own names in some circumstances.

The detector and responder parameters are callback functions.

This PR also renames some internal variables using a more intuitive naming scheme that matches the new interface.

OTHER ADVANTAGES
This approach also has a couple other advantages over the previous approach:

* The names make it clear these are setter methods.
* The type parameter supports the exact names of p5 handlers.

COMPARISON TO OTHER INTERFACES
The approach in this PR is similar to the addEventListener() method of the EventTarget Web API and the .on() method of Konva.js. One major difference is that .setEventDetector() and .setEventHandler() are attached to an interaction object, rather than an event target. This maintains the original design that predates this pull request.

This way, the user only needs to create a single interaction object in order to apply multiple listeners and handlers (e.g. for a draggable square, multiple listeners and handlers are used to change the cursor on mouseover and mouseout events, and also to change the position on drag events).

TESTING
I haven't had time to test this code yet.
Was in a hurry and forgot to add these in.
Fix mismatched closing braces. Also fix a function call (accidentally failed to change a function name in a call after I renamed it).

Test this version against the four demo sketches that the README  links to (point in custom coordinates, arrows, animated square rotation, draggable square)
In the Draggable class, replace the error message in the definition of setEventDetector() and setEventResponder() with a template literal that includes the erroneous user input, so that it's easier for the user to see which part of their code is causing the problem.

Also run a few informal tests of the setters for both detectors and responders by adapting and executing the draggable line demo in pull request #9, including tests with and without erroneous input for the type parameter. Test coverage is not complete but is sufficient for the prototype.
@GregStanton GregStanton marked this pull request as ready for review July 24, 2023 01:46
GregStanton added a commit that referenced this pull request Jul 24, 2023
These updates account for a new, improved interface (implemented in pull request #10) for setting event listeners and handlers on interaction objects.
tfadali and others added 8 commits August 1, 2023 11:30
Replace the object name "Interaction" with the name "EVENT_TYPES," so that it's more descriptive and so that it follows typical casing conventions for JavaScript. 

Upper snake case isn't always used for objects declared with const since they're not immutable like primitives, and even frozen objects can be changed if they have a property that stores another object, but the event type properties are all immutable strings.

Typically, we can use lower camel case for objects and upper camel case for classes, but "EVENT_TYPES" suggests both immutability and that it's some kind of object (since the name is plural).
Note that we already used console.error() in addEventDetector()
MAJOR CHANGES

* Remove multiple detectors:
Remove support for multiple detectors. Having multiple detectors for a single event type turned out to be problematic. For example, suppose the user tries to add a detector that expands the region used to detect mouse-over events. If the mouse is inside the new region and outside the old one, mouse-over and mouse-out events will both fire.

* Enhance square:
Update square drawing object to support stroke-weight property and methods. This is useful for testing addEventResponder().

* Refactor:
Separate responders into two maps (one for default responders, one for user-provided responders). This makes it possible to ensure the defaults always run first and lays the foundation for methods that allow the user to deactivate and reactivate defaults.

MINOR FIXES

* Replace the "in" operator with the has() method, since the has() method is used with Map objects.

* Replace remove() with delete() and rename removeEventResponder to deleteEventResponder (delete() is the Set object method that removes elements).

* Make some changes for consistency.

* Adjust semicolons, whitespace, and comments.
This sketch demonstrates the addEventResponder() and deleteEventResponder() methods of interaction objects.
@GregStanton
Copy link
Contributor Author

GregStanton commented Aug 16, 2023

Quick note:

I used multiple-event-responders.js to test addEventResponder() and deleteEventResponder(), including the error messages that they produce.

I also used that sketch to do basic tests of the new stroke-weight features in the Square class.

@GregStanton
Copy link
Contributor Author

GregStanton commented Aug 16, 2023

TODO

Although we have more features planned (e.g. custom events and custom interaction objects), all that's left for this PR is to implement the following methods on Draggable:

deactivateDefaultResponder(type)
reactivateDefaultResponder(type)

We could do a basic test by calling these with the 'mouseover' type from multiple-event-responders-demo.js. If the deactivate method works, the cursor will no longer change to a move cursor but the stroke will still change; reactivating will restore the original behavior. Once that's done, we can do a quick code review and merge what we have.

Here's my reasoning on prioritizing deactivate/reactivate: Without this, we have no way of resetting default responders, which is something we were already able to do in the original design. (In the original design, we could also reset default detectors. However, that's a less common use case, and the planned features will provide other ways of customizing the default detectors. For example, we'll have customizable hit regions, and it will be possible to create default detectors and responders from scratch for custom events. That all may change, but for now, that's the idea.)

UPDATE

The commits below complete this TODO item. I used deactivate-default-responders-demo.js to do basic tests. We may want to do a quick code review, but otherwise this PR is ready to be merged.

MAJOR CHANGES
* Include new map with default responders that's meant to be changed by deactivate/reactivate methods.

* Add deactivate/reactivate methods.

MINOR CHANGES
* Rename maps that shouldn't be changed (append "Reference" to the end of the names to indicate this). If we eventually freeze those objects, we may want to switch to upper snake case for the names, according to convention.

* Add TODO and other comments.
Also add a TODO comment about error handling.
This sketch is based on interaction-demo.js and multiple-event-responders-demo.js. It includes a couple lines that demonstrate how to deactivate and reactivate default event responders.
@GregStanton GregStanton merged commit 6b001c0 into main Aug 18, 2023
@GregStanton GregStanton deleted the GregStanton-interface-improvement-events branch August 18, 2023 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants