Add undo and redo to annotations. #4482

chenba · 2018-05-21T17:41:51Z

This requires PR #4468 to be merged first.

flodolo

(no issues l10n-wise)

chenba · 2018-05-21T17:52:05Z

server/src/pages/shot/editor-history.js

+    return !!this.beforeEdits.length;
+  }
+
+  undo(canvasBeforeUndo) {


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-io · 2018-05-21T17:57:58Z

Codecov Report

Merging #4482 into master will decrease coverage by 1.69%.
The diff coverage is 11.21%.

@@            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

Impacted Files	Coverage Δ
server/src/pages/shot/drawing-tool.js	`10.6% <0%> (ø)`	⬆️
server/src/pages/shot/editor-history.js	`18.96% <18.96%> (ø)`
server/src/pages/shot/editor.js	`7.64% <2.22%> (-0.33%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7b8c1d...0b4a2f2. Read the comment docs.

jaredhirsch

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.

jaredhirsch · 2018-05-24T18:19:22Z

server/src/pages/shot/editor.js

+
+  applyDiff(area, diffCanvas) {
+    this.imageContext.globalCompositeOperation = "source-over";
+    this.imageContext.drawImage(diffCanvas,


I suspect this might be where the devicePixelRatio is being left out

hmmm scale() should have been called on that drawing context at that point.

Also possibly related to #4453.

…ices#4371)

chenba · 2018-05-24T19:49:13Z

@6a68 I pushed the scaling fix as separate commit for easier review.

Pushed a fix. Thanks!

…services#4371, mozilla-services#4453)

chenba · 2018-05-25T00:17:54Z

Added another commit to actually disable the reset button, and fixed another scaling bug.

jaredhirsch

Looking good! Feature seems solid to me. Couple comments / questions about the code.

jaredhirsch · 2018-05-25T00:36:45Z

server/src/pages/shot/editor.js

+    if (!record) {
+      return;
+    }
+    switch (record.recordType) {


This switch seems unnecessarily complex. How about a simple if/else instead?

jaredhirsch · 2018-05-25T00:38:41Z

server/src/pages/shot/editor.js

@@ -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);


Why do you need to move the scale call into componentDidMount?

Doesn't have to be componentDidMount, it just have to execute only once. Otherwise the shot could be scaled multiple times, once per crop.

jaredhirsch · 2018-05-25T00:44:52Z

static/css/frame.scss

+  background-image: url("../img/annotation-redo.svg");
+
+  &.inactive {
+    background: url("../img/annotation-redo-inactive.svg") center no-repeat;


Seems like one svg could work for undo, redo, and the inactive states, by applying filters and transforms.

I don't think it's possible with background images? If you point me to some docs, I'll give it a shot.

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).

But the transform would be on the element though correct, not its background image?

+1 on the brightness filter.

Oh I was already using transform on the undo button. So...ignore whatever I said about that above. Good catch!

jaredhirsch · 2018-05-25T00:45:27Z

static/css/frame.scss

@@ -492,6 +528,7 @@ body {
  height: 40px;
  border-radius: 3px;
  position: absolute;
+  left: 170.5px;


Why the fractional pixels here and in the margin below?

The color picker look more centered with those numbers.

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.

I was surprised to be able to discern the difference when I tried to changed it to a whole number while working on it.

jaredhirsch · 2018-05-25T01:04:04Z

server/src/pages/shot/editor-history.js

+    return this._replay(canvasBeforeRedo, this.afterEdits, this.beforeEdits);
+  }
+
+  _replay(canvasBeforeChange, from, to) {


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.

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.

…#4371)

chenba · 2018-05-25T14:41:50Z

Found an issue with blurriness when the page is zoomed. HOLD until that's fixed.

…zilla-services#4371)

jaredhirsch

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.

…ices#4371)

chenba · 2018-05-25T20:09:56Z

@6a68: ^ One last review on that commit?

jaredhirsch · 2018-05-29T18:50:47Z

Looks great, just need to run it one more time to make sure the icons behave.

jaredhirsch

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.

chenba requested a review from flodolo as a code owner May 21, 2018 17:41

flodolo approved these changes May 21, 2018

View reviewed changes

chenba commented May 21, 2018

View reviewed changes

chenba force-pushed the 4370-anno-undo branch from b670075 to be6ad36 Compare May 23, 2018 15:55

jaredhirsch previously requested changes May 24, 2018

View reviewed changes

jaredhirsch reviewed May 24, 2018

View reviewed changes

chenba added 2 commits May 24, 2018 14:00

Add undo & redo for annotations. (mozilla-services#4370, mozilla-serv…

4f7b66c

…ices#4371)

Scale the drawing when saving history. (mozilla-services#4370)

a50e819

chenba force-pushed the 4370-anno-undo branch from be6ad36 to a50e819 Compare May 24, 2018 19:47

Scale once and disable reset button. (mozilla-services#4370, mozilla-…

7ecd764

…services#4371, mozilla-services#4453)

chenba mentioned this pull request May 25, 2018

[Annotations] Annotation tools are broken on smaller screens or if the page zoom level was changed before editing #4453

Closed

jaredhirsch suggested changes May 25, 2018

View reviewed changes

Replace switch with if-else. (mozilla-services#4370, mozilla-services…

10b253d

…#4371)

chenba changed the title ~~Add undo and redo to annotations.~~ [HOLD] Add undo and redo to annotations. May 25, 2018

Prevent decimals from zoom or DPI scaling. (mozilla-services#4370, mo…

e1deac6

…zilla-services#4371)

chenba changed the title ~~[HOLD] Add undo and redo to annotations.~~ Add undo and redo to annotations. May 25, 2018

jaredhirsch approved these changes May 25, 2018

View reviewed changes

Use more CSS and less svg files. (mozilla-services#4370, mozilla-serv…

0b4a2f2

…ices#4371)

jaredhirsch approved these changes May 29, 2018

View reviewed changes

jaredhirsch merged commit 8b6bd09 into mozilla-services:master May 29, 2018

testeaxeax pushed a commit to testeaxeax/screenshots that referenced this pull request Jun 7, 2018

Revert "Add undo and redo to annotations. (mozilla-services#4482)"

e4d17ac

This reverts commit d1fd9f8.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add undo and redo to annotations. #4482

Add undo and redo to annotations. #4482

chenba commented May 21, 2018

flodolo left a comment

chenba May 21, 2018

codecov-io commented May 21, 2018 •

edited

Loading

jaredhirsch left a comment

jaredhirsch May 24, 2018

chenba May 24, 2018

chenba commented May 24, 2018

chenba commented May 25, 2018

jaredhirsch left a comment

jaredhirsch May 25, 2018

jaredhirsch May 25, 2018

chenba May 25, 2018

jaredhirsch May 25, 2018

chenba May 25, 2018

jaredhirsch May 25, 2018

chenba May 25, 2018

chenba May 25, 2018

jaredhirsch May 25, 2018

chenba May 25, 2018

jaredhirsch May 25, 2018

chenba May 25, 2018

jaredhirsch May 25, 2018

chenba May 25, 2018

chenba commented May 25, 2018

jaredhirsch left a comment

chenba commented May 25, 2018

jaredhirsch commented May 29, 2018

jaredhirsch left a comment

Add undo and redo to annotations. #4482

Add undo and redo to annotations. #4482

Conversation

chenba commented May 21, 2018

flodolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 21, 2018 • edited Loading

Codecov Report

jaredhirsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenba commented May 24, 2018

chenba commented May 25, 2018

jaredhirsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenba commented May 25, 2018

jaredhirsch left a comment

Choose a reason for hiding this comment

chenba commented May 25, 2018

jaredhirsch commented May 29, 2018

jaredhirsch left a comment

Choose a reason for hiding this comment

codecov-io commented May 21, 2018 •

edited

Loading