Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retina support #1378

Merged
merged 17 commits into from
Feb 23, 2013
Merged

Retina support #1378

merged 17 commits into from
Feb 23, 2013

Conversation

skurfer
Copy link
Member

@skurfer 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.

…in places where it makes sense to do so.
fixes #1044

Previously, it would stay at the default 128x128 unless you manually
changed the interface after every launch.
Some of the code seems to have been based on the assumption that max
icon size will always be 128 x 128.
@pjrobertson
Copy link
Member

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

@skurfer
Copy link
Member Author

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. :-)

@pjrobertson
Copy link
Member

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

@skurfer
Copy link
Member Author

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. :-(

@pjrobertson
Copy link
Member

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//pull/1378#issuecomment-13324627..

@skurfer
Copy link
Member Author

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. :-)

@pjrobertson
Copy link
Member

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//pull/1378#issuecomment-13335234..

@skurfer
Copy link
Member Author

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.

and remove unnecessary code to create a specific representation
@skurfer
Copy link
Member Author

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.

The QSMaxIconSize should only apply to the main interface, not all these other contexts.
makes sure this method has exclusive control of the image's size
@skurfer
Copy link
Member Author

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.

@pjrobertson
Copy link
Member

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//pull/1378#issuecomment-13607707.

@skurfer
Copy link
Member Author

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 2 commits February 15, 2013 11:22
* balance space above and below the content
* remove the titlebar
* center the window with NIB settings
* change deployment target to 10.6
* 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
@pjrobertson
Copy link
Member

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 retina branch February 23, 2013 20:29
@skurfer
Copy link
Member Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants