Karelia changes #4

Merged
merged 12 commits into from Dec 22, 2013

Conversation

Projects
None yet
3 participants
Contributor

mikeabdullah commented Jul 12, 2012

Figured I may as well fire up a pull request so you can grab the changes we've made at Karelia at some point if suits you.

I don't understand why we have this code:

  • [NSImage imageNamed:NSImageNameApplicationIcon] works for all apps, not just those that happen to name their icon "AppIcon"
  • There shouldn't be any need to resize the image; particularly since it's potentially being shared with other bits of the app

Mike -- Point 1, yes, that's true. But the problem is that it returns a 128 pixel image! So if we want a 256 image version, we need to somehow get at the source file and get an NSImage from that.
Point 2 -- I found that because the image's first representation is 128 pixels, that if I did the composting without explicitly saying I wanted the image to be 256 pixels, it would use the 128 pixel representation, and of course that didn't match well. Also, sApplicationIconImage is not shared with other parts of the application.

If you don't believe me, try this yourself. I wouldn't have written this code if I didn't need to :-)

Collaborator

mikeabdullah replied Sep 16, 2012

In my quick test, [NSImage imageNamed:NSImageNameApplicationIcon] gives a 128x128 image, containing representations for all different sizes we might be interested in. My understanding is that drawing the image at 256px will pick the 256px representation, but I might well be wrong. Even so, we could ask the appropriate rep to draw itself directly, rather than asking the image itself.

-[NSApplication applicationIconImage] on the other hand, I found returns a 128x128 image containing a single CGImage-based rep.

Yes, the sApplicationIconImage variable isn't shared, but we're plucking the image out of the +imageNamed: cache, where it might be in use by another bit of the app, either now or in future. As ever, as a general rule, don't mutate an image unless you've just created it.

ok, fixed. I copied the image (rather than retained it) so I could adjust its size, which was necessary.

@danwood danwood Use imageNamed:NSImageNameApplicationIcon as generic name for app ico…
…n. Copy image so that we can modify its size without any other potential clients being messed up.
07ae624

This is definitely an improvement over my busted old code :-) That said, shouldn't it be 512 px these days?

@uliwitness uliwitness added a commit that referenced this pull request Dec 22, 2013

@uliwitness uliwitness Merge pull request #4 from karelia/master
Accept Karelia changes to UKDockProgressIndicator.
df0f57e

@uliwitness uliwitness merged commit df0f57e into uliwitness:master Dec 22, 2013

Contributor

mikeabdullah commented Dec 22, 2013

Yeah, think you're right at 512px. Of course it really ought to do the right thing for retina screens too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment