[WIP] Adding Instagram Filters to Image Viewer via Caman.js (fixes issue #492) by timmoy · Pull Request #694 · mozilla/brackets

timmoy · 2017-04-06T01:49:44Z

Initial Commit Summary

I've created a new file called "instagram.js" in the "src/thirdparty/caman" directory that contains code similar to my implementation for the selfie taker.
The file is in the thirdparty folder to try and avoid the linting error from the previous pull pull request. This seems to have solved the problem for the time being.

We're also using a separate canvas because the Caman function to replace the element with a canvas won't execute synchronously. So the variable "canvas" is assigned a HtmlImageElement and canvas.getContaxt('2d') can't be invoked properly.

The savePhoto() function was ported via some mix of module importing and hard coding some stuff that couldn't be imported (since we don't use a camera object).

The image-view.html file was also modified to include the basic UI for the filters and script tags to load the library and the new file.

Initial Commit To Do Breakdown

Bugs to be fixed (shown in the gif in the order described):

loss of swatches due to script tags being added
can't save the filtered image to replace the original
the canvas also disappears if you open a new image (basically loads for the first image opened per session; if you refresh the page it will lose the canvas for the first image only)

Currently Working:

Loading from image to canvas
Filtering and reset buttons

Other Improvements to be made:

Single canvas loading and filtering
UI also needs to be improved, but that is the focus after the bugs are fixed.

humphd

I left some initial feedback. It's really cool how you've managed to rework your code to do it here vs. in the selfie dialog, nice work! I want to see us move away from having an image and a canvas--we should just always show the canvas, and have the image not be displayed directly. I've left some advice, but feel free to ask questions in this PR for things you're having trouble with.

humphd · 2017-04-06T12:58:32Z

+
 </div>
+<script type="text/javascript" src="/src/thirdparty/caman/caman.full.min.js"></script>
+<script type="text/javascript" src="/src/thirdparty/caman/instagram.js"></script>


I don't think we should use the word "Instagram" anyway, since that's a trademark.

humphd · 2017-04-06T12:59:05Z

@@ -0,0 +1,134 @@
+
+


Remove these blank lines.

humphd · 2017-04-06T12:59:19Z

+$(function() {
+ "use strict";
+
+ var CommandManager = brackets.getModule("command/CommandManager");


We use 4-space indents in Brackets by default.

humphd · 2017-04-06T12:59:54Z

+ var image = this.toBase64();
+ var binaryDataStr = /^data:image\/png;base64,(.+)/.exec(image)[1];
+ //self.camera.savePhoto(base64ToBuffer(binaryDataStr));
+ var binary = window.atob(binaryDataStr);


Watch your indenting here, this next bit got pushed over too far.

humphd · 2017-04-06T13:01:36Z

+ var image = this.toBase64();
+ var binaryDataStr = /^data:image\/png;base64,(.+)/.exec(image)[1];
+ //self.camera.savePhoto(base64ToBuffer(binaryDataStr));
+ var binary = window.atob(binaryDataStr);


Not all browsers support window.atob, specifically I don't think IE has it. We might already have this code somewhere in Brackets, and can expose it. We should figure this out.

I did a search on its compatibility and it seems that IE 10 and onward can use it. Does this mean we should find a workaround for the method for versions before IE 10?

We support:
IE: 10+ (IndexedDB)
Firefox: 26+ (IndexedDB)
Chrome: 31+ (IndexedDB, WebSQL)
Safari: 7.0+ (WebSQL)
Opera: 19+ (IndexedDB, WebSQL)

humphd · 2017-04-06T13:01:57Z

+ bytes[i] = binary.charCodeAt(i);
+ }
+ var data = new Buffer(bytes.buffer);
+ fs.writeFile(savePath, data, {encoding: null}, function() {


This needs error handling in the callback.

humphd · 2017-04-06T13:03:20Z

+ }
+
+ /*
+ Can't use this pattern at the moment since the Caman function won't run before


It means you need to add an event for the DOM to be loaded, maybe use DOMContentLoaded

humphd · 2017-04-06T13:03:55Z

+ canvas.width = img.width;
+ canvas.height = img.height;
+ ctx.drawImage(img, 0, 0, img.width, img.height);
+ };


You probably need an img.onerror too, for cases where things go wrong.

flukeout · 2017-04-06T23:55:07Z

@timmoy - let me know once there is some markup to work with, look at or style and I'll be happy to jump in.

timmoy · 2017-04-09T23:41:01Z

@humphd I got an interesting result when using DOMContentLoaded: the event never occurred. I think it might be the script tags causing the issue as they seem to also be breaking the colour swatch feature. Though I'm at a loss as to why this problem exists for the image viewer but not the selfie-taker.

Anyways, the current workaround I'm using is to reduce the img to 0 pixels and let the image-viewer do its thing to get the image url via {{imgUrl}} that we use to load the image into the canvas. Using {{imgUrl}} caused the file to be undefined, so I had to keep the object around in some capacity for getting the url.

timmoy · 2017-04-16T06:19:50Z

So, with the new commit I've been able to solve two of the bugs at the expense of putting things back into the ImageViewer.js file (so we're failing the lint check again).

But, this means I was able to remove the script tags for the html file which led me to putting the filtering functionality into the onImageLoaded function, which let's us load the Caman.js stuff every time we click on a new image. Also, since the script tags are gone I have been able to get the swatches to show again.

Below is a gif demonstrating the current commit (note saving still doesn't work).

I have 2 images since the swatches extension extracts the colours from the img element so removing it causes it to throw an exception and "hide".
Replacing the img element after the extractColors function could be possible, so I'll test that out in the next few days along with trying to find out the exact problem of why saving can't work.

The code is a huge mess right now, but I'll have it cleaned up after things settle down.

@humphd @gideonthomas @Pomax Let me know if you think I'm heading in the wrong direction or should be making adjustments.

@flukeout I think it's possible we can start on a bit of the design for the interface. I'm using tables and some basic divs and buttons like before in the "image-view.html" changes here.

flukeout · 2017-04-17T20:42:29Z

@timmoy - is there an email I can reach you at? I'm collecting information for a contributors page on the Thimble website and I'd like to include you. Can you please email me at luke@mozillafoundation.org? Thanks!

flukeout · 2017-04-17T20:43:14Z

Also, I'll take a look at the styling and markup probably sometime tomorrow. Most likely, I'll make a pull request against your branch.

gideonthomas · 2017-04-17T21:38:32Z

@timmoy thanks for your work on this! I'm going to take what you've done so far, modify it a bit and open up a PR against your branch since the changes that I'm suggesting are a bit complex. Once that happens, let me know if it makes sense and feel free to suggest changes as well.

gideonthomas · 2017-04-19T02:54:31Z

@timmoy I've made a PR against your branch: timmoy#2. See if it makes sense to you (if not, feel free to ask questions) and then merge it in. The next step for you should be to rebase your branch onto mozilla/master to take in the new changes (you're going to have a conflict with src/main.js; fix it by making sure it has both the envConfig line and the caman line). Then, @flukeout can suggest styling changes.

timmoy · 2017-04-19T18:36:21Z

@gideonthomas I merged the branch and github prompted me to fix some merge conflicts. I also brought the changes into my local machine and found a lot of commits and files changed. This means that I shouldn't need to rebase onto master, right?

gideonthomas · 2017-04-19T21:15:52Z

@timmoy you definitely need to rebase onto master because my patch was only on top of yours and you need the commits from master as well.

…ce the original, loss of swatches due to script tags being added, the canvas also disappears if you click on a new file in the image tree

…; indentation fixes, added basic error handling for callback of line 24

…js fixes the swatches not loading and allows canvas to be shown for multiple images, but save problem persists

"added a comma to fix build error"

timmoy · 2017-04-20T02:07:47Z

@gideonthomas I've rebased the code as requested and it should be up to date with master, plus the seven commits in this branch. I suppose that we can go ahead with @flukeout for the styling changes needed?

timmoy · 2017-04-21T02:55:29Z

@gideonthomas I've noticed after my rebase that button text has disappeared for the most part (selfie taker buttons still work). See the gif below.

It seems that after the rebase, the moustache code you did for the strings isn't loading for me. Is this just for my system and should be ignored?

The most recent commit fixed a redefine of a openSVGasXML preference (it somehow snuck in there during rebase) as well as another line from the rebase that prevented swatches from loading when integrated with our current implementation.

What I changed for the swatch fix was this line back to your version. When using the current version that master does like this, we get a "str" is undefined somehow... I'm not sure how to proceed here since we're kind of regressing this part of the code?

gideonthomas

@timmoy one change still needs to be made. For your strings to appear correctly, run npm run unlocalize and then run npm run build (since I think there were changes in src/bramble) and npm start (which should run npm run localize for you) and open up bramble in the editor (make sure your cache is disabled). You should be able to see the button text in there now. You'll MAYBE also see changes in your local folder tracked by git which you can get rid of by running npm run unlocalize

gideonthomas · 2017-04-21T04:30:38Z


- var stringFormat = Strings.IMAGE_DIMENSIONS;
- var dimensionString = StringUtils.format(stringFormat, this._naturalWidth, this._naturalHeight);
+ var dimensionString = this._naturalWidth + " &times; " + this._naturalHeight + " " + Strings.UNIT_PIXELS;


please undo this change

the change has been undone as requested

timmoy · 2017-04-21T06:29:08Z

@gideonthomas I've reverted the change, and can confirm it's working with swatches, strings and save functionality still intact. Also, I've noted that we're failing the build check because Travis isn't finding the new Image.js file. Does that mean the Travis side needs changes or is there something we can do on our end?

gideonthomas · 2017-04-22T21:17:45Z

 _ = require("thirdparty/lodash"),
- Mustache = require("thirdparty/mustache/mustache");
+ Mustache = require("thirdparty/mustache/mustache"),
+ Image = require("editor/image");


needs to be capital I

I've fixed the issue; thanks for catching that.

gideonthomas · 2017-04-25T18:49:30Z

+
+ var Caman = require("caman");
+ var FilerFileSystem = require("fileSystemImpl");
+ var FileUtils = require("filesystem/impls/filer/FilerUtils");


FilerUtils vs. FileUtils. Missing 'r'

gideonthomas · 2017-04-25T18:51:10Z

One small nit change (FilerUtils vs. FileUtils) that needs to be made, r+ from me otherwise. Really nice work on this @timmoy!

Will leave the final review on @humphd since I contributed to some of this code.

humphd · 2017-05-08T02:04:49Z

+ return;
+ }
+
+ FileSystemCache.refresh(function(err) {


Do you really need this? It seems unnecessary to me, since you're always going to be overwriting the original file. It won't hurt anything, but it's wasteful. Maybe file a bug later to rip it out if I'm right, and it's not needed.

I tried removing the function (but kept the button disabling) and it seems that it works without it on my machine. I'm not sure if it'll impact other sections of the code.
@gideonthomas can you confirm if removing it was okay or that it's needed for some other part of the logic?

I'll debug this a bit today as I try to figure out @flukeout's issue with background-image not updating in the preview.

humphd · 2017-05-08T02:05:31Z


+# Image Filters
+
+IMAGE_FILTER=Add Filter


IMAGE_FILTERS_TITLE would make more sense to me, but it's not really important.

humphd · 2017-05-08T02:05:41Z

+
+ var Caman = require("caman");
+ var FilerFileSystem = require("fileSystemImpl");
+ var FilerUtils = require("filesystem/impls/filer/FilerUtils");


nit: alignment here.

humphd · 2017-05-08T02:07:13Z

+ image.reset();
+ image[fnName].apply(image, args);
+ image.render();
+ $saveBtn.prop("disabled", false);


Should you disable the filter buttons at the top of this function so that people don't click it over and over while it's running? It seems to do odd things for me if I click it multiple times quickly. Again, could be done in follow-up bug.

I agree, it does make the user experience a bit weird. I believe @flukeout has addressed it in his pull request as part of the UI changes. I'll probably merge it in sometime tomorrow evening.

@timmoy we're trying to freeze all the new code on our staging box by Thurs, so it's key that we get this stuff landed ASAP, so we can start testing. If you can't get this done, let us know, and we'll take this over.

@humphd I've taken most of the solution (no spinner) from @flukeout 's pull request.
I've tested it and it works well on my machine. I think this can be (or at least pretty close to being) landed?

humphd · 2017-05-08T02:08:19Z


 var CommandManager = brackets.getModule("command/CommandManager");
 var Commands = brackets.getModule("command/Commands");
+ var fs = brackets.getModule("fileSystemImpl");


Curious why you chose to use this level of abstraction for the filesystem? We don't do caching at this level. Is this why you're doing the FileSystemCache.refresh?

yeah basically! We seem to be using this in most of our bramble extensions so thought we should use it here too. Is there something else we should use that's better?

humphd · 2017-05-08T02:09:31Z

@@ -0,0 +1,244 @@
+/*


How come you didn't just use npm to install this vs. pulling it directly into the tree? Not a blocker, but seems like something you could have done--other parts of Brackets do this now.

Interesting! Yeah I can try to do that.

It's fine if this happens in a follow-up, it doesn't need to block this from landing.

humphd · 2017-05-08T02:10:56Z

I think this is probably good to go. Two things I think we could do in follow-up bugs:

When we apply the filter, and the image's colours change, we should probably re-run the palette extraction code (e.g., on Save or something) so it properly reflects what's actually there vs. the original.

It would be nice to separate the filter buttons from the Save/Reset buttons a bit more. Did @flukeout do any UI review on this? Can be something we iterate on.

Great work on this, it's very cool!

gideonthomas · 2017-05-08T16:44:18Z

this is pending @flukeout's UI review

flukeout · 2017-05-08T19:24:19Z

Okay, checking it out now.

flukeout · 2017-05-08T22:21:35Z

In testing this, I'm unable to actually apply the filter. Here's a screencap gif of my process...

Console is wigging out...

cc @gideonthomas am I missing some steps in how I'm testing this other than just running the code in the PR?

flukeout · 2017-05-09T00:02:55Z

Okay, I submitted a PR against this branch that has my changes - timmoy#3

flukeout · 2017-05-09T00:05:45Z

Okay, I figured out that the reason the filter wasn't working for me in the GIF above is because I'm using the image as a background-image. It looks like Thimble is setting the background image in the preview as a blob, which somehow gets corrupted or broken when the filter is applied...

cc @humphd @gideonthomas it seems to work fine for <img> elements however.

humphd · 2017-05-09T01:26:31Z

@flukeout please put the entire project you're doing online so I can see it. It has to be something specific to the project (e.g., rewriting the image multiple times) or something. I'll try to debug.

…lignment), and removed the filesystemRefresh function (seems stable from my testing)

humphd · 2017-05-10T01:29:22Z

I'm going to defer to @gideonthomas, who I think has been working on this too, today.

gideonthomas · 2017-05-10T04:34:38Z

Moved to #756

updated code to load hidden image and use that url to load our canvas; indentation fixes, added basic error handling for callback of line 24 attempt to take the code from filters.js and put it into ImageViewer.js fixes the swatches not loading and allows canvas to be shown for multiple images, but save problem persists First fixes Abstract base64 to buffer conversion logic Cleanup image filter code Update main.js "added a comma to fix build error" got rid of extra lines that weren't working with current build reverted change to dimensionString fixed a capitalization typo for Image.js require updated caman library with comment block for details of license, url, etc updated base64toBuffer to be in fileUtils fixed a variable name typo [FileUtils] to [FilerUtils] updated to make text fixes (IMAGE_FILTER to IMAGE_FILTERS_TITLE and alignment), and removed the filesystemRefresh function (seems stable from my testing) Some styling updates to the image filter UI. Added a progress spinner De-selecting selected filter after it's applied to the image. Hard to force a style override and fixed a nit. Code style fixes

timmoy mentioned this pull request Apr 6, 2017
[WIP] Adding Instagram Filters to Selfies via Caman.js (fixes issue #492) #656
Closed

humphd suggested changes Apr 6, 2017
View reviewed changes

gideonthomas self-assigned this Apr 6, 2017

timmoy mentioned this pull request Apr 7, 2017
Add support for CSS Instagram Filters #492
Closed

flukeout added the Launch label Apr 13, 2017

timmoyand others added 6 commits April 19, 2017 20:06

initial commit with some bugs; can't save the filtered image to repla…
67e7706
…ce the original, loss of swatches due to script tags being added, the canvas also disappears if you click on a new file in the image tree

updated code to load hidden image and use that url to load our canvas…
a06d020
…; indentation fixes, added basic error handling for callback of line 24

attempt to take the code from filters.js and put it into ImageViewer.…
5a5eb4c
…js fixes the swatches not loading and allows canvas to be shown for multiple images, but save problem persists

First fixes
e910946

Abstract base64 to buffer conversion logic
cb99a2b

Cleanup image filter code
d810ca7

timmoy force-pushed the issue492v3 branch from b645112 to 850fd76Compare April 20, 2017 01:49

Update main.js
d114f66
"added a comma to fix build error"

timmoy force-pushed the issue492v3 branch from 850fd76 to d114f66Compare April 20, 2017 02:03

got rid of extra lines that weren't working with current build
b21d0d1

gideonthomas suggested changes Apr 21, 2017
View reviewed changes

reverted change to dimensionString
54ab828

gideonthomas reviewed Apr 22, 2017
View reviewed changes

updated base64toBuffer to be in fileUtils
ce997fe

gideonthomas reviewed Apr 25, 2017
View reviewed changes

gideonthomas approved these changes Apr 25, 2017
View reviewed changes

gideonthomas assigned humphd and unassigned gideonthomas Apr 25, 2017

fixed a variable name typo [FileUtils] to [FilerUtils]
fa07ede

humphd reviewed May 8, 2017
View reviewed changes

updated to make text fixes (IMAGE_FILTER to IMAGE_FILTERS_TITLE and a…
d75ae26
…lignment), and removed the filesystemRefresh function (seems stable from my testing)

gideonthomas mentioned this pull request May 9, 2017
Refreshing FileSystemCache blob urls does not work for linked CSS files #751
Closed

timmoy added 2 commits May 9, 2017 21:17

added@flukeout's solution for multi-filter processing
2a8d4b9

removed debug messages
15f04d7

gideonthomas mentioned this pull request May 10, 2017
Fix #492 - Add image filters (#694) #756
Merged

gideonthomas closed this May 10, 2017

gideonthomas mentioned this pull request May 10, 2017
Image Filters - Automatically update swatches on image save #757
Closed

Uh oh!

Conversation

timmoy commented Apr 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Initial Commit Summary

Initial Commit To Do Breakdown

Bugs to be fixed (shown in the gif in the order described):

Currently Working:

Other Improvements to be made:

Uh oh!

humphd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flukeout commented Apr 6, 2017

Uh oh!

timmoy commented Apr 9, 2017

Uh oh!

timmoy commented Apr 16, 2017

Uh oh!

flukeout commented Apr 17, 2017

Uh oh!

flukeout commented Apr 17, 2017

Uh oh!

gideonthomas commented Apr 17, 2017

Uh oh!

gideonthomas commented Apr 19, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timmoy commented Apr 19, 2017

Uh oh!

gideonthomas commented Apr 19, 2017

Uh oh!

timmoy commented Apr 20, 2017

Uh oh!

timmoy commented Apr 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gideonthomas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timmoy commented Apr 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gideonthomas commented Apr 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timmoy commented Apr 6, 2017•
edited
Loading

gideonthomas commented Apr 19, 2017•
edited
Loading

timmoy commented Apr 21, 2017•
edited
Loading

gideonthomas left a comment •
edited
Loading

timmoy commented Apr 21, 2017•
edited
Loading

flukeout commented May 8, 2017•
edited
Loading