search

everestpipkin / image-scrubber

A friendly browser-based tool for anonymizing photographs taken at protests
https://everestpipkin.github.io/image-scrubber
MIT License
928 stars 82 forks source link

Adding ctrl/cmd-Z and undo button #18

Closed SaFrMo closed 4 years ago

SaFrMo commented 4 years ago

This should be good to go! Half of #9.

I generalized the path drawing function since both handleMouseUp and undoLastPath both use it. Let me know if there are any notes or improvements you'd like to see and thanks!

everestpipkin commented 4 years ago

Thank you for doing this! It works perfectly on its own and I really like it, but there was an additional commit around the same time that adds an additional canvas to the program, for a blur function that is more secure.

I think they're fighting badly - when I try to merge the current master repo & this pull I get some very weird behavior like it re-blurring the whole path on undo. I will keep poking at it, but if you know what might be a good fix please let me know?

SaFrMo commented 4 years ago

Will do @everestpipkin ! Taking a look at that now. Thanks for the notes @bMacSwigg ! Will respond to them after resolving the issue @everestpipkin mentioned.

SaFrMo commented 4 years ago

@everestpipkin Looks like it's working with the updated blur now! Let me know if anything looks off to you - thanks!

Edited to add: Addressing @bMacSwigg 's points:

If the last path overlaps with a previous path, part of that previous path could end up completely uncovered. I'm not sure I see a better thing to do with that situation, but wanted to highlight it.

I definitely see what you mean! A better fix could be to pop the last path off the pathHistory, reset the working canvas to the initial image, then recreate every path in the pathHistory. I can try that out unless there are higher priority items to work on, since the basic version of cmd-z is working now - thoughts @everestpipkin ?

If the last path was an undo, then we'll blur over it -- even if what it was actually undoing was a paint. Since the undo could cover both blur and paints, or could cover something that was not originally blurred, the behavior might be kinda weird.

Definitely! That would be solved by the fix above too, but until then I just prevented adding undo paths to the pathHistory in the first place. Now the only paths that are undone are blurs and paints.

everestpipkin commented 4 years ago

Thanks for working on this! I'll give it some testing then merge.

bMacSwigg commented 4 years ago

A better fix could be to pop the last path off the pathHistory, reset the working canvas to the initial image, then recreate every path in the pathHistory.

Yeah, that should work! Might be wonky performance-wise with a lot of paths, but hopefully not too bad. I have noticed occasional slow performance, but I've had trouble reproducing it consistently and it might just be my computer having problems.

until then I just prevented adding undo paths to the pathHistory in the first place.

That seems like a reasonable short-term fix. LGTM

everestpipkin commented 4 years ago

Found a bug - some weird behavior if you try to undo after a rotate! The mask inverts or sections don't undo anymore.

SaFrMo commented 4 years ago

Ohh, good catch! I'll take a look at that later today, around 6pm ET. Thanks!

SaFrMo commented 4 years ago

@everestpipkin FYI, I spent some time looking into this and haven't found a decent fix - I tried the "reset canvas, then redo all previous behaviors" undo method @bMacSwigg and I talked about here, but the way the image working data is passed around among the canvases has a few too many moving parts for me to figure out tonight.

I have a few hours I'd like to spend on this Wednesday 6/3 evening starting at 6pm ET and can see a couple paths for that time - (a) I can spend it getting the undo function working in the current context, which will continue to be slow going but would result in less drastic changes to the codebase, (b) I can do a fuller organization and refactor, as suggested in #25, which would include formatting/splitting JS files and doing some fuller documentation on the flow for myself and future contributors, (c) I can leave the undo feature as-is for now and move to something else, or (d) we can ship the undo feature as it is in this PR and make a note that rotating + undoing aren't working well together yet.

I'd personally vote (b) above but am up for these or any other thoughts - let me know @everestpipkin ! If I don't hear anything I'll proceed with (b) and fix any conflicts afterwards. Thanks!

bMacSwigg commented 4 years ago

I would also vote for option (b), personally. I think it would save dev time on future FRs. With the undo brush & the ability to just reload the image from scratch if necessary, I don't think the undo button is so urgent that we need to push forward with (a) or (d).

everestpipkin commented 4 years ago

I agree with b! It seems like urgent fixes are slowing so it would be a good time to refactor.

SaFrMo commented 4 years ago

Excellent, will do! I'll plan on getting started on that around 6pm ET today and will write out more detailed plans in #25.