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

Optimisations #1335

Merged
merged 15 commits into from Jan 22, 2013
Merged

Optimisations #1335

merged 15 commits into from Jan 22, 2013

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 20, 2013

Most of the information is in each commit message.

The reason I decided to GZip the requests to QSApp is to reduce bandwidth usage.
We can save quite a bit, for example on the plugins info .xml (from here)

http://qs0.qsapp.com/plugins/info.php is gzipped
Original Size: 218.07 KB
Compressed Size: 41.06 KB
Data Savings: 81.17%

GZip support isn't documented, bit I've used it fine in iOS.

The other changes:

  • Better getter/setter methods, hopefully to avoid some crashes (see #1331 for an explanation)
  • Fix this problem
  • Reduce the number of calls to details, after Rob and I discussed it on IRC
  • Pops history off the stack when browsing ←. Again, discussed on IRC. You'll notice at the moment if you dance up/down in a folder with →,←,→,← it doesn't work indefinitely. My changes fix this.

Lots of these commits are from a stash made a while back, and 'optimisations' can always be too optimistic, so it's worth keeping a close eye when running with this pull

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 20, 2013

Looks like good stuff. One small thing I've seen in brief testing is that the action icon goes dim after a few seconds. Going to the second pane and hitting ↓ makes it look "normal" again.

If you're bored, here are some other small fixes we could add to this:

  • The count at the bottom of the results doesn't always update correctly. Actually, I can only reproduce this by searching for "in" or "ni". The count is updated when I type "i", or if I type another letter after "in", like "ind". Weird. I may have to debug locally if you're not seeing this.
  • If the last thing you had up was SearchEngine ⇥ Search For… ⇥ term, when you reinvoke QS, the "term" string in the third pane appears as the details, and the usual label area is blank.

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 20, 2013

The count at the bottom of the results doesn't always update correctly.

Figured this one out. The status string is only updated if the selection changes. We might need to define a method to generate that string and call it for both selection changes and array changes (by overriding the setter for currentResults).

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 22, 2013

I'll fix the two things I mentioned in another branch. Are you going to do as @tiennou suggested:

Change all accessors that use meta to go through setObject:forMeta unconditionally

I haven't looked to see if there are any places where that needs to be done.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 22, 2013

I checked through them yesterday, but couldn't see any extra calls to setObject:forMeta:, but I of course forgot to look for calls to [meta setObject:forKey:].

Another commit with a few more changes.

There are 1 or 2 methods which I haven't touched (setIdentifier and setName) as they do more funky things that I think we should leave alone

pjrobertson added 2 commits Jan 22, 2013
It's argued that this is easier to read
Metadata is only ever stored in the 'meta' dict (not the 'data' dict), so only check there for object properties
skurfer added a commit that referenced this issue Jan 22, 2013
@skurfer skurfer merged commit f9da8dd into quicksilver:master Jan 22, 2013
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

3 participants