Conversation
|
Ran into a bug while testing escape behavior: That's my bad lol, forgot to update a couple func names :/ |
Microck
force-pushed
the
feat/flip-selection
branch
from
0503305 to
c8dbc6f
Compare
hingler
left a comment
hingler
left a comment
There was a problem hiding this comment.
LGTM, thanks for making those changes! I'd like to put a pin in the aforementioned comments but I'm fine committing this as-is
| } | ||
| } | ||
| function flipSelection(horizontal) { | ||
| if (!rectSelection.active) return; |
There was a problem hiding this comment.
Would we be OK throwing an error in this case? IDK what the defacto standard is for error handling on the web but I figure we should handle this more gracefully, as flipSelection should only be called when the rectSelection is already active.
| const c = getFrameCanvas(rectSelection.L, rectSelection.F, rectSelection.key); | ||
| if (!c) return; | ||
| const ctx = c.getContext("2d", { willReadFrequently: true }); | ||
| if (!ctx) return; |
There was a problem hiding this comment.
Same here— not sure what would be best but i figure if c or ctx are null then we're encountering anomalous behavior which we want to log instead of returning silently to avoid a crash.
|
|
||
| ctx.clearRect(rectSelection.x, rectSelection.y, rectSelection.w, rectSelection.h); | ||
|
|
||
| const flipped = ctx.createImageData(rectSelection.w, rectSelection.h); |
There was a problem hiding this comment.
not "mission critical" but at a glance it seems like there should be some more optimal way to snapshot a region of the canvas, flip it, and write it back. Some combination of transformation operations on the dest canvas should work to accomplish the same, I think?
| 8: "eyedropper" | ||
| 3: "line", | ||
| 4: "rect", | ||
| 5: "text", |
There was a problem hiding this comment.
rect line and text aren't in yet, unless this is helping support another commit
| return; | ||
| } | ||
| } | ||
| if (rectSelection.active && (e.key.toLowerCase() === "h" || e.key.toLowerCase() === "v")) { |
There was a problem hiding this comment.
also not "mission-critical" but in the future we should maybe be factoring this "event handler" code into a different function.
| const tag = e.target && e.target.tagName ? e.target.tagName.toUpperCase() : ""; | ||
| if (tag !== "INPUT" && tag !== "TEXTAREA" && tag !== "SELECT") { | ||
| e.preventDefault(); | ||
| gridEnabled = !gridEnabled; |
There was a problem hiding this comment.
gridEnabled does not appear to be defined
2 participants
Description
Add keyboard shortcuts to flip rectangular selections horizontally (H key) or vertically (V key).
Type of Change
Changes Made
Testing
Test Configuration:
Checklist
Related Issues
N/A