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

Fixes for details #1151

Closed
wants to merge 8 commits into from
Closed

Fixes for details #1151

wants to merge 8 commits into from

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Oct 3, 2012

This takes care of the problems described by #1148 as well as some other small things.

The first couple of commits arose from the question “Why is details being called so many times?” and had nothing to do with the original issue. The rest were about getting the correct details to show up in the correct places.

One thing I’m still seeing, but haven’t investigated: The details for actions aren’t returned from the cache in a lot of cases. I’m wondering if this could be by design. Are there situations where the action’s details can change based on the direct/indirect objects?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 4, 2012

Something to keep an eye on before you merge this: I've started to see intermittent freezing as soon as I invoke QS. It'll display, then sit unresponsive for a few seconds. It might be more likely if the last thing selected was a Web Search or with a Web Search in the 3rd pane, but that's not certain. If anything, these changes should make it more responsive so I don't think this is the cause, but I first noticed the problem right after, so…

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 4, 2012

On the mystery freezes, I've also noticed that the action's icon is faded (and it's the default gear, even when it shouldn't be), so it's almost certainly something with the icon loading branch.

As mentioned on IRC, the whole process of "displaying" the object is repeated when you dismiss QS:

Dismiss QS Trace

My first thought was that the animation needed to redraw everything first, but disabling "superfluous effects" doesn't seem to make a difference.

skurfer added 7 commits Oct 16, 2012
This was getting called immediately after `makeKeyAndOrderFront:`, causing a lot of code to be run twice. The documentation says it generally shouldn't be necessary and I don't see any difference without it.
The call to `nextEventMatchingMask:` seemed to be "replaying" the event, causing a lot of code to be run again.
The details were being appended to the `status` string *before* figuring out which result was currently selected.
Previously, the `details` method would need to be invoked *twice* to yield the correct result. (The first invocation would cache the details, and the second would return it from the cache.)

close #1148
Whether or not the details can fit in the results list has nothing to do with whether or not they can fit in the main interface. In the context of the results list, the details string is already being handled appropriately by `QSResultController`.
reproduces work done by @pjroberston - fixes #525
@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 16, 2012

I had to commit evil and rebase this. The changes to Bezel conflict with the new Bezel that's been merged to master. Conflicts in a NIB are basically impossible to deal with, so I started over.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 16, 2012

Where are all my lovely comments?! 😒

P.S. GH supports fancy smilies now if you're MSN messenger inclined... just type a colon ':' to see the list

I think this was meant to be a fallback value, but since primaryType is *always* a string, it was returning the fallback the first time this method was run. After that, the cached value would be returned.
@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 25, 2012

Found (and fixed) another small bug while messing with the Contacts plug-in. This should be ready now.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 26, 2012

All of these changes are included in #1163 now.

@skurfer skurfer closed this Oct 26, 2012
pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on b0a31c1 Oct 27, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skurfer
I think the reasoning behind doing this was so that when the line height is < 34, the details aren't shown at all in the results list.

If you change the results list height to say 26 you'll notice that the details still show, even though they're half chopped off. But for some reason this happens in ß70 so I don't think it was something you added. Still something we should fix.

Also - BezelHUD never shows details in the results list (no matter what the line height) so really details should always show in the gutter, not just when lineHeight < 34... but this is an anomaly which I don't think we should bother working around

skurfer
Copy link
Member Author

@skurfer skurfer commented on b0a31c1 Oct 27, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reasoning behind doing this was so that when the line height is < 34, the details aren't shown at all in the results list.

But this isn't the results list, so it shouldn't be checked here.

If you change the results list height to say 26 you'll notice that the details still show, even though they're half chopped off.

What the…? I've never seen that with any version, and I played with the setting a lot when working on this. But it definitely happens. Anyway, that's a bug in the results list, not here.

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on b0a31c1 Oct 27, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skurfer
Copy link
Member Author

@skurfer skurfer commented on b0a31c1 Oct 29, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The results list uses QSObjectCells to display the object

Oh, well that explains a lot. I'll look into 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.

None yet

2 participants