WIP: First version of touch events #142
Conversation
|
@rafou, Awesome! Thanks for taking a stab at this. I'll try to take a first pass at some point today. Probably this eve after work. |
|
Hey @JoshMarler, did you have time to have a quick look at this by any chance ? |
|
@rafou , Apologies for the delay! I shall try to take a look this evening. I've a few thoughts and need to think over the problem a little more. |
|
@rafou , Right I'm going to add a couple little things about the code itself and then I'll add a wider "how I think it should be done" discussion this eve. |
There was a problem hiding this comment.
@rafou, Thanks for taking a stab at this! I've left some initial review points which are about the code in general. I'll follow up with a more thorough "how I think TouchEvents should be handled" comment this eve after some more thought.
| dispatchViewEvent("onMouseDown", detail::makeViewEventObject(e, *this)); | ||
| break; | ||
| case juce::MouseInputSource::InputSourceType::touch: { | ||
| Touch* t = new Touch(e.source.getIndex(), e.x, e.y, e.getScreenX(), e.getScreenY(), getViewId()); |
There was a problem hiding this comment.
So we're using raw new here and then storing the allocated pointer in a juce::Array. To my knowledge juce::Array is non-owning. As a result there is a memory leak here as I see no matching delete anywhere for these Touch objects.
See juce::Array docs:
"Holds a resizable array of primitive or copy-by-value objects"
I would question why we need to use Touch* here. Touch is a struct and not a polymorphic base or similar so this code should probably use a value type array juce::Array<Touch> and operate with references to a "copy-by-value" type.
| dispatchViewEvent("onMouseDrag", detail::makeViewEventObject(e, *this)); | ||
| break; | ||
| case juce::MouseInputSource::InputSourceType::touch: { | ||
| Touch* t = getTouch(e.source.getIndex(), touches); |
There was a problem hiding this comment.
From a style point of view some white space between lines here would be nice to assist readability.
|
|
||
| juce::var getVar() | ||
| { | ||
| juce::String jsonString = "{\"identifier\": " + juce::String(identifier) + ",\"x\": " + juce::String(x)+ ",\"y\": " + juce::String(y)+ ",\"screenX\": " + juce::String(screenX)+ ",\"screenY\": " + juce::String(screenY)+ ",\"target\": " + target.toString() + "}"; |
There was a problem hiding this comment.
It seems a shame to have this manual building of a JSON string here. We should be able to convert a Touch to a juce::DynamicObject instance.
| } | ||
|
|
||
| /** Retrieve the index of a specific Touch object in an array of Touch objects, if it exists. */ | ||
| static int getTouchIndexInList (const int touchIdentifier, juce::Array<Touch*> myList) |
There was a problem hiding this comment.
juce::Array has anindexOf function which would remove this boiler plate.
| juce::Array<Touch*> changedTouches; | ||
|
|
||
| /** Retrieve a specific Touch object in an array of Touch objects, if it exists. */ | ||
| static View::Touch* getTouch (const int touchIdentifier, juce::Array<Touch*> myList) |
There was a problem hiding this comment.
Here I'd go for more modern C++ style. So juce::Array has begin() and end() iterators. Meaning we can use std::find_if
| } | ||
|
|
||
| interface CanvasState { | ||
| width: number; | ||
| height: number; | ||
| firstStroke?: boolean; |
There was a problem hiding this comment.
Could you clarify what firstStroke is for here?
| autoclear?: boolean; | ||
| ongoingTouches?: Array<Touch>; |
There was a problem hiding this comment.
Could you clarify what ongoingTouches is for here? It feels like a thing that should not be part of Canvas's props API
| @@ -59,13 +60,15 @@ export function bindCanvasContextProperties(ctx: any) { | |||
|
|
|||
| export interface CanvasProps { | |||
| onMeasure?: (e: any) => void; | |||
| onDraw: (ctx: CanvasRenderingContext2D) => void; | |||
| onDraw: (ctx: CanvasRenderingContext2D, sizeState: object) => void; | |||
There was a problem hiding this comment.
This is probably not a necessary addition.
We should be able to pass width/height to draw routines through other means. For example in your App component's drawingCanvasDraw(ctx, sizeState) function, instead of passing sizeState to onDraw you would add width and height to App's internal state and then inside drawingCanvasDraw refer to this.state.width etc. The idea being that drawingCanvasDraw is a member function of your App component class bound to this.
| @@ -4,7 +4,7 @@ | |||
| "private": true, | |||
| "dependencies": { | |||
| "@babel/runtime-corejs3": "^7.11.2", | |||
| "react-juce": "^0.2.1", | |||
| "react-juce": "file:../../../packages/react-juce", | |||
There was a problem hiding this comment.
This is a thing that should be removed before merge. You might already know this and it's possibly left over from WIP but thought I'd add just to make sure.
Add TouchEvent capability to the View components