Animate NSImageView if gif was given as screenshot.

OdNairy commented 8 years ago

Added support for gifs as screenshot image.

OdNairy commented 8 years ago

PR can be improved by checking real NSData for 0x47 header flag but was ignored in a reason of architecture and noncausal case with nongif content for gif-named image.

guillaumealgis commented 8 years ago

Thank you for the contribution @OdNairy!

While I think this is a good addition to Alcatraz, I'm afraid of what the plugin list will look like if too many maintainers add animated screenshots. This could be messy 🤔. I think having some sort of play button (like Tweetbot does it in its timeline) could look good. What do you think?

OdNairy commented 8 years ago

@guillaume-algis this PR doesn't enable GIF on package list but on screenshot select screen.

guillaumealgis commented 8 years ago

Oh, right. Sorry, ignore my comment on the messy plugin list then 👍 Still, I think using the file extension to determine if the image is a gif is a bit unreliable.

guillaumealgis commented 8 years ago

@OdNairy I'd be happy to have this feature in Alcatraz, would you have time to address the points we discussed a few months ago? Namely:

Renaming -displayImage:package: to -displayImage:forPackage:;
Checking the image data to identify gifs instead of relying on the extension, as you proposed.

And also rebasing on top of master so this can be merged easily :)

If you can't or don't want to for any reason, let me know, and I'll take over 🙌

Thank you for your work so far 👍

OdNairy commented 8 years ago

@guillaumealgis thx you for up this thread. It dropped out of my mind. I will check this PR on this week

OdNairy commented 8 years ago

I'm stacked on issue with gif detection. Technically there is way to detect NSData as image/gif (prefix 0x47) but when we are working with NSImage there are no such information. The only NSData property NSImage have is TIFFRepresentation which one have no special markers for image/gif. For my perpective there are only solution to detect gif on download stage on cache some wrap class instead of NSImage only. @guillaumealgis can you advice something? Shall I improve image cache?

OdNairy commented 8 years ago

Oh, i'm totally missed your comment:

I'm a little uneasy relying on the file extension to identify gifs. I'd prefer we look into the image data to be sure the file is indeed an animated gif. Something like https://gist.github.com/keefo/5344890 isGifImage.

It's seems like suitable solution.

OdNairy commented 8 years ago

@guillaumealgis PR is ready for code-review and merge

guillaumealgis commented 8 years ago

Thanks for the quick work @OdNairy! I just tested it, this is awesome 👍

Here's a list of minor things I'd like review with you before we can merge this:

A few minor code change, see my inline comments;
In -[ATZPluginWindowController displayImage:forPackage:], please replace occurences of true and false by YES and NO respectively;
Not super sure how we're supposed to handle that, but for consistency purpose I'd keep the same header format than the other files in the project (your thoughts @supermarin?):

// Foo.h
//
// Copyright (c) 2016 Marin Usalj | supermar.in
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

Given that a good part of the code in NSImage+Alcatraz.m is copied from https://gist.github.com/keefo/5344890, I'd also add proper attribution for the work. I guess a simple comment before the method should suffise ¯_(ツ)_/¯;
Also, @keefo, if you could tell us under which license you're distributing your code in https://gist.github.com/keefo/5344890, that'd be super nice, because if I'm not mistaken without any explicit licensing we're not allowed to use this;
Finally, @OdNairy, please add a line detailing the change (and crediting yourself 😉) in the CHANGELOG, under a ## Unreleased tag (which we'll later replace with a version number).

keefo commented 8 years ago

Thank you for reaching out to me.

It is under MIT License.

Feel free to use it.

On Aug 25, 2016, at 11:53 AM, Guillaume Algis notifications@github.com wrote:

Thanks for the quick work @OdNairy https://github.com/OdNairy! I just tested it, this is awesome 👍

Here's a list of minor things I'd like review with you before we can merge this:

A few minor code change, see my inline comments; In -[ATZPluginWindowController displayImage:forPackage:], please replace occurences of true and false by YES and NO respectively; Not super sure how we're supposed to handle that, but for consistency purpose I'd keep the same header format than the other files in the project (your thoughts @supermarin https://github.com/supermarin?): // Foo.h // // Copyright (c) 2016 Marin Usalj | supermar.in // // Permission is hereby granted, free of charge, to any person obtaining a copy // of this software and associated documentation files (the "Software"), to deal // in the Software without restriction, including without limitation the rights // to use, copy, modify, merge, publish, distribute, sublicense, and/or sell // copies of the Software, and to permit persons to whom the Software is // furnished to do so, subject to the following conditions: // // The above copyright notice and this permission notice shall be included in // all copies or substantial portions of the Software. // // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR // IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, // FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE // AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. Given that a good part of the code in NSImage+Alcatraz.m is copied from https://gist.github.com/keefo/5344890 https://gist.github.com/keefo/5344890, I'd also add proper attribution for the work. I guess a simple comment before the method should suffise ¯(ツ)/¯; Also, @keefo https://github.com/keefo, if you could tell us under which license you're distributing your code in https://gist.github.com/keefo/5344890 https://gist.github.com/keefo/5344890, that'd be super nice, because if I'm not mistaken without any explicit licensing we're not allowed to use this; Finally, @OdNairy https://github.com/OdNairy, please add a line detailing the change (and crediting yourself 😉) in the CHANGELOG, under a ## Unreleased tag (which we'll later replace with a version number http://keepachangelog.com/en/0.3.0/). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alcatraz/Alcatraz/pull/432#issuecomment-242499573, or mute the thread https://github.com/notifications/unsubscribe-auth/AAxadJvaKjESRk3mzH3lqLNtULGr6nRsks5qjeS8gaJpZM4HqG5t.

OdNairy commented 8 years ago

Thanks for your feedbacks. Code was updated to fulfil your recommendations.

guillaumealgis commented 8 years ago

Thank you @OdNairy, I caught a few more coding style issues, sorry I didn't get them on first reading :/

OdNairy commented 8 years ago

Will be ready in 2 minutes

On 26 Aug, 2016, at 13:33, Guillaume Algis notifications@github.com wrote:

Thank you @OdNairy https://github.com/OdNairy, I caught a few more coding style issues, sorry I didn't get them on first reading :/

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alcatraz/Alcatraz/pull/432#issuecomment-242695301, or mute the thread https://github.com/notifications/unsubscribe-auth/AAs3yjXUkIv04qhuME3l7w6C7xnaXkC_ks5qjsDsgaJpZM4HqG5t.

OdNairy commented 8 years ago

@guillaumealgis PR is update again. Actually Alcatraz repo needs .clang-format to correctly handle code style Also there is a question from me - does we really need link for gist with simple AppKit classes use? I'm not about ignoring copyright but seems not legit in this case as for me

guillaumealgis commented 8 years ago

👍 Looks good to me

Thanks again @OdNairy, that was quick! I agree we should have some sort of coding style automatic checking tool instead of reviewing by-hand. Regarding linking to the gist, I have no idea from a legal standpoint, but given we copied the code verbatim and the snippet was helpful to solve our problem, linking to the source and thus crediting the author can't hurt :)

OdNairy commented 8 years ago

Yeah, sure. Just a bit of curiosity.

supermarin commented 8 years ago

👍

guillaumealgis commented 8 years ago

Thanks @OdNairy and @supermarin!

alcatraz / Alcatraz

Animate NSImageView if gif was given as screenshot. #432