CtrlCV #20
CtrlCV #20
Conversation
|
the SelectionManager tests are failing (expected since I changed behaviour) - fixing now. |
|
unit tests now fixed, but |
|
Looks like selection hits the right-hand edge, but only selection content hits the bottom edge (clarified in person). |
| // Offset the pasted selection from the original location. | ||
| var tool = pskl.app.drawingController.currentToolBehavior; | ||
| var isSelectionTool = tool instanceof pskl.tools.drawing.selection.BaseSelect; | ||
| if (isSelectionTool) { |
There was a problem hiding this comment.
Open question: Should this become an early-out? Should we do anything in this method if we don't have a selection tool active?
| var isSelectionTool = tool instanceof pskl.tools.drawing.selection.BaseSelect; | ||
| if (isSelectionTool) { | ||
| var maxHeight = frame.width - 1; | ||
| var maxWidth = frame.height - 1; |
| } | ||
|
|
||
| // Save to state when selection is dropped in place. | ||
| if (offset === 0) { |
There was a problem hiding this comment.
Discussed in person: Maybe should create a save state for each paste operation after the first one and the deselect operation, instead of only on the deselect operation?
| var maxHeight = frame.width - 1; | ||
| var maxWidth = frame.height - 1; | ||
| var xCoordinate = pixels[pixels.length - 1].col; | ||
| var yCoordinate = pixels[pixels.length - 1].row; |
There was a problem hiding this comment.
When using the lasso tool, the 'last' pixel won't necessarily represent the lower-right boundary of your selection.
|
Updated with what we chatted about. Thanks for the help! |
| @@ -91,7 +93,29 @@ | |||
| } | |||
| }; | |||
|
|
|||
| ns.SelectionManager.prototype.paste = function() { | |||
| // Iterate through pixels to get the largest x and y values of a selection. | |||
| ns.SelectionManager.prototype._getBoundaryEdges = function(pixels) { | |||
There was a problem hiding this comment.
Can I suggest some different naming here? You're kind of working out the lower-right corner of your selection's bounding box, and 'edges' may not express that well enough. I'd almost suggest generalizing this function to find and return a bounding box for the set of pixels and name it that, then use bottom and right properties on the returned object. Or maybe just getBottomRightCorner?
There was a problem hiding this comment.
Also, this method is effectively static. It'd be nice to write a unit test for it.
| // Iterate through pixels to get the largest x and y values of a selection. | ||
| ns.SelectionManager.prototype._getBoundaryEdges = function(pixels) { | ||
| var xCoordinate = 0; | ||
| var yCoordinate = 0; |
There was a problem hiding this comment.
Not relevant in this particular case but it's possible for a selection to (at least temporarily) exist entirely above or to the left of the canvas. It might be more precise here to initialize these to -Infinity.
| xCoordinate = pixels[i].col; | ||
| } | ||
| if (yCoordinate < pixels[i].row) { | ||
| yCoordinate = pixels[i].row; |
There was a problem hiding this comment.
It might be easier to understand your goal if these were named maxXCoordinate and maxYCoordinate
Modify Ctrl+C / Ctrl+V behavior after receiving feedback that people thought copy and paste was broken.
New behaviour:
Select an area.
Ctrl+C
Ctrl+V
Move selection to new position.
Deselect.
Some edge cases. When the selected area is on the right or bottom edge, don't offset the paste, just highlight the selection darker. Also note, that Ctrl+V does commit the selection to the canvas before offsetting the overlay, so some of the old behaviour still works.