-
Notifications
You must be signed in to change notification settings - Fork 67
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
Conversation
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.
src/controllers.lisp
Outdated
@@ -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*) |
There was a problem hiding this comment.
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?
src/controllers.lisp
Outdated
@@ -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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on-release?
src/controllers.lisp
Outdated
@@ -99,5 +108,29 @@ | |||
|
|||
;;; Keyboard | |||
|
|||
(defmethod keyboard-event :after ((instance sketch) | |||
state timestamp repeatp keysym)) | |||
(defconstant +scancode-prefix-length+ 9) |
There was a problem hiding this comment.
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!
src/controllers.lisp
Outdated
|
||
(defmethod on-text :around ((*sketch* sketch) text) | ||
(with-sketch (*sketch*) | ||
(call-next-method))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
draw mode missing
src/controllers.lisp
Outdated
|
||
(defmethod on-key :around ((*sketch* sketch) key state) | ||
(with-sketch (*sketch*) | ||
(call-next-method))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
draw mode missing
src/controllers.lisp
Outdated
@@ -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)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/controllers.lisp
Outdated
@@ -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)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I think I've addressed all the feedback!
|
@@ -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)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
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