Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing input-handling functions. #128

Merged
merged 4 commits into from
Jan 5, 2024

Conversation

Kevinpgalligan
Copy link
Contributor

Including: on-click-release, on-middle-click-release, on-right-click-release, on-text and on-button.

Also, add a call to on-hover for the window, not just entities.

Also, update the docs & examples.

Relevant issue: #125

Including: on-click-release, on-middle-click-release, on-right-click-release,
on-text and on-button.

Also, add a call to on-hover for the window, not just entities.

Also, update the docs & examples.
@@ -47,6 +49,10 @@
(propagate-to-entity *sketch* x y #'on-right-click)
(call-next-method))))

(defmethod on-hover :around ((*sketch* sketch) x y)
(with-sketch (*sketch*)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

draw-mode nil missing - also, shouldn't we propagate-to-entity like everywhere else?

@@ -16,11 +16,13 @@
(defmethod on-click (instance x y))
(defmethod on-middle-click (instance x y))
(defmethod on-right-click (instance x y))
(defmethod on-click-release (instance x y))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on-release?

@@ -99,5 +108,29 @@

;;; Keyboard

(defmethod keyboard-event :after ((instance sketch)
state timestamp repeatp keysym))
(defconstant +scancode-prefix-length+ 9)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we change this to

(defconstant +scancode-prefix-length+ (length "scancode-"))

? thanks!


(defmethod on-text :around ((*sketch* sketch) text)
(with-sketch (*sketch*)
(call-next-method)))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

draw mode missing


(defmethod on-key :around ((*sketch* sketch) key state)
(with-sketch (*sketch*)
(call-next-method)))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

draw mode missing

@@ -16,11 +16,13 @@
(defmethod on-click (instance x y))
(defmethod on-middle-click (instance x y))
(defmethod on-right-click (instance x y))
(defmethod on-click-release (instance x y))
(defmethod on-middle-click-release (instance x y))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the new methods not using with-sketch / draw-mode ?

probably a good candidate for a file-local macro..but we can do that later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I forgot to add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, a macro would be nice to remove all the repetitive code.

@@ -16,11 +16,13 @@
(defmethod on-click (instance x y))
(defmethod on-middle-click (instance x y))
(defmethod on-right-click (instance x y))
(defmethod on-click-release (instance x y))
(defmethod on-middle-click-release (instance x y))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to add -press methods as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I was intuitively interpreting on-click to mean "on press". But now that you say it, onclick events in JavaScript are triggered when the click is released. So it might make more sense that way.

...so that people don't get a surprise.

@Kevinpgalligan
Copy link
Contributor Author

I think I've addressed all the feedback!

  • Set draw-mode to nil where appropriate.
  • Propagate events to entities.
  • Replace repetitive method definitions with macro.
  • Remove magic number.
  • Change the interface so that on-click refers to a full click, including the release of the mouse button, as is the case e.g. in JavaScript. Now there's on-click-press for the initial press of the mouse button. Open to changing this if there are alternative suggestions.

@@ -57,14 +56,17 @@

(defmethod kit.sdl2:mousebutton-event ((instance sketch) state timestamp button x y)
(let ((button (elt (list nil :left :middle :right) button))
(method (elt (list nil #'on-click #'on-middle-click #'on-right-click) button)))
(click-method (elt (list nil #'on-click-press #'on-middle-click-press #'on-right-click-press) button))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey! thanks for making the changes! I'm confused here now --- it's saying click methods are the press ones, and release methods are .. click?

why don't we do all three methods - press, click, release? I feel like release is interesting in a drag & drop scenario

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Click methods are release, ya. That's how it works in JavaScript (onclick()) and I think it matches people's intuitions. E.g. if you press down the mouse button and move it around, you don't expect the click to take effect until you release the button. Could add an explicit on-click-release but it would be doing the same thing as on-click.

@vydd vydd merged commit c1b63b2 into vydd:master Jan 5, 2024
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.

None yet

2 participants