-
Notifications
You must be signed in to change notification settings - Fork 0
Draggable line #9
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
base: main
Are you sure you want to change the base?
Conversation
tfadali
commented
Jul 20, 2023
- added missing styles
- added line object
- new draggable line demo
Rename "center" as "midpoint" due to naming conflict with the center() method of the p5.Element base class in p5. (This was caught by p5's Friendly Error System.)
w = createGraphWindow(xOrigin, yOrigin, xScale, yScale); | ||
|
||
myLine = w.createLine(-1, -2, 3, 4); | ||
myLine.setIsDraggable(true) |
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.
Let's come to a tentative consensus on the 'setIsDraggable' name before we merge this, since it affects the interface. Here are some issues to consider.
Comparison to our naming scheme for event listeners: The name is consistent with the naming scheme for event listener setters, such as setObjectIsHovered(). However, "setIsDraggable" sets a Boolean, whereas "setObjectIsHovered" sets a listener that returns a Boolean. This may be okay, since there are two cues that these setters accept different kinds of input. The biggest cue is the "-able" and "-ed" endings. The other cue is that the passive "isHovered" is paired with "Object" (the thing being hovered). So, the "setIsDraggable" name seems okay from this perspective.
Comparison to p5
Some cases where p5 uses "set" are as follows:
- setAttributes() is a general setter for WebGL that can be called like setAttributes(key, value) or setAttributes(obj)
- setMoveThreshold()
- setShakeThreshold()
- set() for setting pixels or writing a whole image to the display window
- setCamera()
- .set() for p5 vectors and for p5.Camera objects (p5.Camera objects also have .setPosition())
Some cases where p5 uses "is" in a function name: isLooping() and keyIsDown(). Neither is a setter. Both return Booleans, as we'd expect based on the usual convention.
Since the use of "setIsDraggable" seems consistent with usage in p5, and the use of "isDraggable" as a setter seems inconsistent with p5, the name "setIsDraggable" seems preferable. From this perspective, the current name seems good.
Comparison with the combined getter/setter approach of Konva.js: In this approach, methods like .stroke() do two things: they set the relevant property based on the arguments that are supplied, and they also return the value of the property. So, for example, calling .stroke() without arguments just gets the object's current stroke. In the same way, Konva.js has a .draggable() method that acts as a getter and setter for a Boolean "draggable" flag. However, "isDraggable" makes it more clear that we're dealing with a Boolean (due to the naming convention for Booleans), and .isDraggable() probably shouldn't be used as a setter, to avoid conflict with p5. Again, "setIsDraggable" seems best.
Tentative conclusion: The current name .setIsDraggable() seems to work best. For other functions like .stroke(), we can have a separate discussion to break down the pros and cons of combined get/set functionality. As with many things related to getters and setters, there's debate about whether combined getters/setters are generally helpful or unhelpful. Some pros and cons are discussed here:
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 what's missing from this analysis is that setIsDrabble, to me, suggests that there is a boolean inside that we are setting. Other developers familiar with common naming conventions will also think that. It's true that you can look at the signature or docs and see that it's a callback, but that's elevating the importance of documentation, relative to making the API intuitive.
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.
Personally, I didn't pick up on the cues, I don't think. The naming convention is too ingrained for me.
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 don't really see the "-ed" or "-abble" as cues the way you do, and at least in the p5 editor, there isn't going to be any autocomplete suggestion that it takes a callback.
myLine.setIsDraggable(true) | ||
|
||
draggable = w.createDraggable(); | ||
draggable.mouseDragged(object => { |
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.
In the design I have in mind, the default .mouseDragged() handler would work, so it wouldn't be necessary to set it in the sketch code. It could be customized, but for a sketch like this, it wouldn't be necessary. So, lines 18 through 25 involving mouseDragged() and mouseDropped() wouldn't be needed.
Were you planning to take out these lines once you implement more of the design? If not, I could explain the approach I'm thinking of. It's important that we make the basic interactions work as easily as possible. Of course, I'm not sure yet if the idea I'm thinking of will work quite as I hope!
Naming note: mouseDropped() is a placeholder name unless we can't find something better. Until now, it hasn't been a priority since we haven't needed to use it in the demo sketches.
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.
Were you planning to take out these lines once you implement more of the design?
You guessed it.
); | ||
} | ||
|
||
computeGraphFromCanvas() { |
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.
The map() method is more concise than the loop I used for the same method in the Square class. There are some trade-offs, but if one implementation is clearly preferable on balance, then could you go ahead and make the corresponding methods in these classes consistent? Once we get to the point where we have a third repetition of these methods, we may want to go ahead and abstract them into a base class, but for now, we could skip 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.
I don't know if we have to go with 100% loops or 100% functional-style .map() stuff. For example, at my last job, we had the convention of using loops whenever the internal code mutated some state, and preferred functional-style for most other cases. Sometimes, for more complicated cases, a loop will be much easier to deal with.
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.
But yeah, we can make it consistent between methods that do the same thing and/or combine them into common abstract methods.
return createVector(xCenter, yCenter) | ||
} | ||
|
||
getWidthInCanvas() { |
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 guess you hard coded 5 temporarily?
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.
Yeah, just getting it working for now.
const [start, end] = this.verticesInCanvas; | ||
strokeWeight(this.strokeWeight); | ||
line(start.x, start.y, end.x, end.y); | ||
strokeWeight(3) |
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.
Maybe the midpoint doesn't need to be displayed? Or if it does, would it take long to replace the hard-coded 3 with something else? It might be okay for the sake of the prototype.
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.
Midpoint display is to help indicate that you can drag it if your mouse is over that point.
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.