Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Retina support #1378

Merged
merged 17 commits into from Feb 23, 2013

Conversation

Projects
None yet
2 participants
Owner

skurfer commented Feb 8, 2013

This adds the @2x icons supplied by Dan so far and allows preview icons to be rendered at the maximum quality on retina displays, along with some other fixes.

Specifically

  • There were a lot of places that used explicit NSSize() calls. I changed them to use the pre-defined QSSizeXXX values instead. Makes no difference really, but it could. Instead of doubling the size claimed by the interface controllers, we could instead just double all the QSSize definitions. So everywhere in the code that refers to QSSize128, it would actually get a 256x256 representation.
  • The max icon size is supposed to be defined by the current interface, but it wasn't asking the interface for the max size on startup. Only if the interface was changed by the user. I fixed that, but I'm not sure if I went about it in the best way. b4bb1bc
  • I think some of the references to 128x128 were made with the assumption that this was the max icon size, so I changed them to use the actual max icon size instead. d0af3ab

Things I didn't address, but we should talk about:

  • It seems wasteful to call createIconRepresentations in some of the places we're using it. If I understand correctly, we usually only need to generate the size used by the QSObjectCell, and the size used in the results list (and their 2x counterparts) for the main interface. For prefs and things where the icons are smaller, it should be able to request the appropriate sizes as well instead of getting them all.
  • Related to the above, since the sizes in createIconRepresentations are hard-coded, you could very well end up not generating the necessary sizes. For example, an interface might have a max icon size of 400x400 or something. So the "size used by the QSObjectCell" referenced above needs to be determined at runtime.

We can't just change createIconRepresentations because it's a category on NSImage and has no knowledge of the interface's current size requirements. But I'd hate for every place that currently calls it with a bunch of code to do the same thing. (There are some plug-ins that use it.) Perhaps we could create a method in the interface controller to generate icons instead of using the NSImage category.

skurfer added some commits Jan 25, 2013

replace hard-coded sizes with QSMaxIconSize
…in places where it makes sense to do so.
get the max icon size from the active interface
fixes #1044

Previously, it would stay at the default 128x128 unless you manually
changed the interface after every launch.
make some hard-coded values relative
Some of the code seems to have been based on the assumption that max
icon size will always be 128 x 128.

could this just be changed to [QSObject interfaceChanged]?

It doesn't seem like there are any other interface changed notif observers but it's possible that down the line there might be and it might cause some hiccups no?

Personally, I'd move the interfaceChanged method into QSController. It shouldn't really be in the QSObject class!

-(void)interfaceChanged {
    QSMaxIconSize = [self interfaceController] maxIconSize];
    [QSObject purgeAllImagesAndChildren];
}
Owner

skurfer replied Feb 8, 2013

It appears to be set up so anything that knows about QSObject can refer to the max size, and moving it to the interface controller might not work. (It'll take some doing anyway):

(null): "_QSMaxIconSize", referenced from:
(null): -[QSCommandObjectHandler saveCommand:toPath:] in QSCommand.o
(null): -[QSFileSystemObjectHandler previewIcon:] in QSObject_FileHandling.o
(null): -[QSFileSystemObjectHandler prepareImageforIcon:] in QSObject_FileHandling.o
(null): -[QSURLObjectHandler buildWebSearchIconForObject:] in QSObject_URLHandling.o
(null): -[QSPlugInManager plugInWasInstalled:] in QSPlugInManager.o

Owner

pjrobertson commented Feb 8, 2013

Just documenting a few things said in IRC:

It seems like a lot of the NSImage code in QS could likely be unneeded now, after many of the NSImage API changes in 10.5/10.6.
Since we're cleaning this up, we should probably look at removing that - already being done :)

We may potentially still need to createRepresentationOfSize: for some images, but if we test and things work fine, then perhaps not - it's quite possible that the NSImage API will create representations as it sees fit now.

One thing I just noticed: when we do clean this up, can we remove QSObject.m:L55
[NSImage imageNamed:@"Question"] is always nil

Owner

skurfer commented Feb 8, 2013

Try that last commit. I've tested on a non-retina screen. Seems fine. I wonder how much of what I see is from cached Quick Look stuff, but I've brought up "new" things and they all seem to render correctly.

Assuming it works, it sure is easier than trying to figure out which representations we need. :-)

Owner

pjrobertson commented Feb 8, 2013

Yep, everything seems fine :)

The menu interface is funny (what you said about not all the text showing up) but everything else is fine.

Another couple of points:

  • We should maybe look at using some of the predefined images from NSImage.h. See this clever guy's app for examples of some of them ;-)
  • Changing the results table row height in the command prefs crashes QS after having changed the interface. Not sure if it's related to this
Owner

skurfer commented Feb 9, 2013

Changing the results table row height in the command prefs crashes QS after having changed the interface. Not sure if it's related to this

I was able to reproduce this with B72. Our first hotfix. :-(

Owner

pjrobertson commented Feb 9, 2013

I was able to reproduce this with B72. Our first hotfix. :-(

Have you already done a git bisect? Sounds like you know where it's from.
If not I'll go ahead and figure it out :)

On 9 February 2013 02:28, Rob McBroom notifications@github.com wrote:

Changing the results table row height in the command prefs crashes QS *
after* having changed the interface. Not sure if it's related to this

I was able to reproduce this with B72. Our first hotfix. :-(


Reply to this email directly or view it on GitHubhttps://github.com/quicksilver/Quicksilver/pull/1378#issuecomment-13324627..

Owner

skurfer commented Feb 9, 2013

No, but I'm pretty sure I know what causes it. The result window NIB is observing the row height. I think the result window gets released when the interface changes, so changing the pref in that state causes a crash. If you invoke the interface and make the result window appear, there's no crash (because new instance is created I assume).

So there's the problem. No idea how to fix it though. :-)

Owner

pjrobertson commented Feb 10, 2013

OK, so turns out this is nothing new in ß72. It's been around since forever
:)
See the latest pull.

On 9 February 2013 17:58, Rob McBroom notifications@github.com wrote:

No, but I'm pretty sure I know what causes it. The result window NIB is
observing the row height. I think the result window gets released when the
interface changes, so changing the pref in that state causes a crash. If
you invoke the interface and make the result window appear, there's no
crash (because new instance is created I assume).

So there's the problem. No idea how to fix it though. :-)


Reply to this email directly or view it on GitHubhttps://github.com/quicksilver/Quicksilver/pull/1378#issuecomment-13335234..

Owner

skurfer commented Feb 10, 2013

ImageNamed is cool, BTW. Seems to be some overlap with the stuff in CoreTypes, but some are unique. All come through nice and crisp on a retina display.

retain quality for shrunken images
and remove unnecessary code to create a specific representation
Owner

skurfer commented Feb 14, 2013

See if commenting lines 97 and 98 in QSObject_URLHandling.m fixes your web search icons, @pjrobertson. It doesn't break them for me.

skurfer added some commits Feb 14, 2013

ignore interface icon size, start big and let NSImage do the right thing
The QSMaxIconSize should only apply to the main interface, not all these other contexts.
use a copy of the find icon
makes sure this method has exclusive control of the image's size
Owner

skurfer commented Feb 15, 2013

4 mote commits. The only other weird thing I'm aware of is the appearance of the "Hold ⌘Q to Quit" window. The text doesn't quite fit, and just before the quit happens, the image jumps to a larger size.

Owner

pjrobertson commented Feb 15, 2013

I don't know why the unsure quit dialog uses a weird button cell thingy.

See my latest commit on r2 which uses an image well instead - try that.
out this afternoon, but the bank account is set up all fine.

Out

On 15 February 2013 14:03, Rob McBroom notifications@github.com wrote:

4 mote commits. The only other weird thing I'm aware of is the appearance
of the "Hold ⌘Q to Quit" window. The text doesn't quite fit, and just
before the quit happens, the image jumps to a larger size.


Reply to this email directly or view it on GitHubhttps://github.com/quicksilver/Quicksilver/pull/1378#issuecomment-13607707.

Owner

skurfer commented Feb 15, 2013

Looks perfect now, but once it's on screen, it never goes away (whether you hold the keys or not). Never quits either. I'll see if I can figure it out. Thanks for getting it that far.

skurfer and others added some commits Feb 15, 2013

Quit confirm window tweaks
* balance space above and below the content
* remove the titlebar
* center the window with NIB settings
* change deployment target to 10.6
quit confirmation fixes
* make sure everything refuses first responder
* cast the window to QSBorderlessWindow
* remove unnecessary code

pjrobertson added a commit that referenced this pull request Feb 23, 2013

@pjrobertson pjrobertson merged commit 040dc3b into quicksilver:master Feb 23, 2013

Owner

pjrobertson commented Feb 23, 2013

I'm assuming you'll deal with getting this into release and stuff. Seems like you know what you're doing with the branches :P

@skurfer skurfer deleted the skurfer:retina branch Feb 23, 2013

Owner

skurfer commented Feb 23, 2013

Seems like it. :-)

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