-
Notifications
You must be signed in to change notification settings - Fork 128
Conversation
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.
(no issues l10n-wise)
return !!this.beforeEdits.length; | ||
} | ||
|
||
undo(canvasBeforeUndo) { |
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.
Both undo and redo takes in the canvas before the action is applied because we need to save the image or part of it to revert the action (undo a redo or redo an undo).
Codecov Report
@@ Coverage Diff @@
## master #4482 +/- ##
=========================================
- Coverage 46.73% 45.03% -1.7%
=========================================
Files 41 42 +1
Lines 1622 1712 +90
Branches 307 318 +11
=========================================
+ Hits 758 771 +13
- Misses 864 941 +77
Continue to review full report at Codecov.
|
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.
Unfortunately, his patch creates blurry (wrong DPI) canvas sections after undoing a draw action. The blurriness remains if the undo is followed by a redo.
|
||
applyDiff(area, diffCanvas) { | ||
this.imageContext.globalCompositeOperation = "source-over"; | ||
this.imageContext.drawImage(diffCanvas, |
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 suspect this might be where the devicePixelRatio is being left out
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.
hmmm scale()
should have been called on that drawing context at that point.
Also possibly related to #4453.
@6a68 I pushed the scaling fix as separate commit for easier review. |
Added another commit to actually disable the reset button, and fixed another scaling bug. |
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.
Looking good! Feature seems solid to me. Couple comments / questions about the code.
server/src/pages/shot/editor.js
Outdated
if (!record) { | ||
return; | ||
} | ||
switch (record.recordType) { |
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.
This switch seems unnecessarily complex. How about a simple if/else instead?
@@ -271,6 +352,8 @@ exports.Editor = class Editor extends React.Component { | |||
|
|||
componentDidMount() { | |||
document.addEventListener("mouseup", this.onMouseUp); | |||
const imageContext = this.imageCanvas.getContext("2d"); | |||
imageContext.scale(this.devicePixelRatio, this.devicePixelRatio); |
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.
Why do you need to move the scale
call into componentDidMount
?
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.
Doesn't have to be componentDidMount
, it just have to execute only once. Otherwise the shot could be scaled multiple times, once per crop.
static/css/frame.scss
Outdated
background-image: url("../img/annotation-redo.svg"); | ||
|
||
&.inactive { | ||
background: url("../img/annotation-redo-inactive.svg") center no-repeat; |
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.
Seems like one svg could work for undo, redo, and the inactive states, by applying filters and transforms.
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 think it's possible with background images? If you point me to some docs, I'll give it a shot.
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.
Setting a CSS filter
to brightness(0.1)
will make the inactive icon look active.
You can flip the undo into redo by changing the button's CSS transform
from scale(-1, 1)
to scale(-1, 1) rotateY(360deg)
.
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 the transform would be on the element though correct, not its background image?
+1 on the brightness filter.
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.
Oh I was already using transform on the undo button. So...ignore whatever I said about that above. Good catch!
@@ -492,6 +528,7 @@ body { | |||
height: 40px; | |||
border-radius: 3px; | |||
position: absolute; | |||
left: 170.5px; |
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.
Why the fractional pixels here and in the margin below?
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 color picker look more centered with those numbers.
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, it's true. I guess subpixel rounding should be fine, since we're only supporting Firefox, and the problem starts with the .color-board .triangle
having left: 9.5px
, and that's not part of this patch. Still seems odd to me, but that's fine.
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 was surprised to be able to discern the difference when I tried to changed it to a whole number while working on it.
return this._replay(canvasBeforeRedo, this.afterEdits, this.beforeEdits); | ||
} | ||
|
||
_replay(canvasBeforeChange, from, to) { |
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'm a bit confused by this part of the code. If I understand correctly, in the push
method above, each time the image is edited, an EditRecord
is appended to this.beforeEdits
. Why, then, does this method need to create a new EditRecord
each time undo or redo is pressed? I wonder if it would be more performant to have just one array of edits, and, when undo or redo is pressed, increment or decrement an index cursor into that existing array of EditRecords
.
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.
It can't work like that. Or at least we can't have just one array of edits. When the user is editing the image, the affected area before the change is saved, but not the change itself. So when the user performs an undo, we need to save the change at that time so it can be (re-)applied should the user decide to redo.
Using an index is possible. I think it's a matter trading memory usage and less clean(?) code for performance at that point.
Found an issue with blurriness when the page is zoomed. HOLD until that's fixed. |
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'm fine to land this as-is. Up to you if you want to reduce the SVG count; http/2 makes that a fairly low-value optimization anyway.
@6a68: ^ One last review on that commit? |
Looks great, just need to run it one more time to make sure the icons behave. |
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.
Nice work on the icon changes! Looks good.
I found another bug, but it exists on master, so this is good to land 🎉
* Add undo & redo for annotations. (mozilla-services#4370, mozilla-services#4371) * Scale the drawing when saving history. (mozilla-services#4370) * Scale once and disable reset button. (mozilla-services#4370, mozilla-services#4371, mozilla-services#4453) * Replace switch with if-else. (mozilla-services#4370, mozilla-services#4371) * Prevent decimals from zoom or DPI scaling. (mozilla-services#4370, mozilla-services#4371) * Use more CSS and less svg files. (mozilla-services#4370, mozilla-services#4371)
This reverts commit d1fd9f8.
Fix #4370 and #4371
This requires PR #4468 to be merged first.