search

thieman / github-selfies

Everything is better with selfies. Yes, even pull requests. Trust me.
MIT License
1.14k stars 76 forks source link

Adds gif functionality to Chrome #33

Closed joeLepper closed 10 years ago

joeLepper commented 10 years ago

So this is definitely a larger PR than I had originally hoped. Let me explain why.

I made one pass at this about a week ago and ended up so deep in spaghetti code that I couldn't dig myself out. So I ended up going back to square one and refactoring things to encapsulate function and state a bit more and eliminate all of the this / that / self shenanigans in lieu of a global config object (this has the added advantage of eliminating the long list of positional arguments that the GitHubSelfies object needed in favor of a more explicit object with named attributes).

I noted that there was a video element that seemed like we wanted to display, though it was not being shown. I went ahead and got that displayed, as it proved pretty hard to take a reasonable gif without being able to see myself moving. It would be nice in the future to make this a mirror so that movement is more natural.

After I cleaned that up I moved on to pulling styles out of JS as much as possible – taking advantage of the route-conditional stylesheets that Chrome extensions allow. This helps us keep our HTML strings more manageable in the JS, though because of the way that route-matching works I did have to add !important (eww) to a couple of the new-issue rules.

Once I was there I got back to adding in the gif functionality. You'll see a commit that includes a handful of files related to this. Beyond those it was a simple matter of adding in the dynamicSelfie functionality and adding a little bit of visual feedback about the progress of the recording.

I'm still dissatisfied with the way that some of the UI works – the progress bar doesn't move left-to-right on all routes, for example. And there are a handful of bugs surrounding the way that we add the buttons / videos to issues / PRs that are closed.

But I think that this PR constitutes a pretty good first version of the feature.

joeLepper commented 10 years ago

Oh, and I almost forgot:

selfie-0

groundwater commented 10 years ago

selfie-3

thieman commented 10 years ago

"Neural-Net Quantization Algorithm"

Excited to review this! Should get around to it soon.

joeLepper commented 10 years ago

Should've pointed out, this is the repo from whence a lot of the gif-generating plumbing came from: https://github.com/antimatter15/jsgif

It's something that got ported over from AS3, as you can tell from all of the commented-out Flash import statements. There's another library, called gifjs that utilizes web workers to build the gif more quickly, which would let us build higher-quality images, but integrating that would involve some serious re-architecting (more than I've already done) to the extension.

On Wed, Apr 2, 2014 at 5:23 AM, Travis Thieman notifications@github.comwrote:

"Neural-Net Quantization Algorithm"

https://camo.githubusercontent.com/33609e6058886747c5868ef5b57575878f5c7e81/687474703a2f2f7777772e74686573696c2e63612f77702d636f6e74656e742f75706c6f6164732f323031342f30312f6b656c2e676966

Excited to review this! Should get around to it soon.

Reply to this email directly or view it on GitHubhttps://github.com/thieman/github-selfies/pull/33#issuecomment-39323386 .

groundwater commented 10 years ago

@joeLepper I hacked on using git.js for a night once, and I could never get the web workers to run within the Chrome extension. There was always a security exception. I was in unfamiliar territory however, but I thought I'd add my experience as a data point.

joeLepper commented 10 years ago

Yeah, that's why I ultimately abandoned it. The workers would have to run within the context of the "background page" that the extension launches from. All of github-selfie's functionality is actually run in the context of the page you're viewing, which is where the security exception comes in. An extension that runs thusly can't call out for external scripts, for obvious reasons.

There is a mechanism available to pass messages back and forth, so it's technically possible to have the web workers waiting on the background page, and just pass in binary strings for each frame, then post to imgur from there. But IIRC, when I was looking at doing that it would have meant re-writing a lot more of this project as well as hacking at gif.js.

thieman commented 10 years ago

I'm gonna start hammering out the bugs. Identified so far:

  1. Doesn't work on closed issues/PRs, presumably because it doesn't have the fixes from 1.3 yet
  2. I can't get it to work on new PRs at all, maybe I'm doing something wrong. The video never shows up. It works on new issues, though.
  3. Non-GIF selfies are sized incorrectly and seem to only capture the top-left quadrant of the picture.
thieman commented 10 years ago

@joeLepper Gonna close this PR and merge my gif branch once I get all my bug fixes in. Thanks again for this, this is awesome and I'm super excited to use it.

selfie-2

thieman commented 10 years ago

Merged!

joeLepper commented 10 years ago

Fantastic!

Glad to help out.

joeLepper commented 10 years ago

Also, i noticed that I broke the staticSelfie method today while I was monkeying with it. I have a fix, if you haven't already applied one.

thieman commented 10 years ago

@joeLepper I think I cleaned up most of it, but if you see a bug in 1.4 (which is live!) then file it and/or submit a PR. Thanks!