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

show proxy object icons more reliably #2179

Merged
merged 5 commits into from Nov 16, 2016
Merged

show proxy object icons more reliably #2179

merged 5 commits into from Nov 16, 2016

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Jan 26, 2016

Fixes #2069, which was the original motivation, but I also put this out there to get your opinion on the changes to the observers. I’ve long wondered what would happen if we did this.

(My motivation for trying was to get the mini-icons in a comma trick collection to update. It was either this, or another complicated mess of adding and removing observers for individual objects.)

I think it’s OK because we can tell the OS a billion times that a view needs to be redrawn, but it won’t actually redraw unless it decides it’s a good time to do so. Is that correct? In any case, icon changes don’t come in too rapidly. The only “problem” I see is that all three panes get the notice and ask to be redrawn.

Before this is merged, I’d also like to see if we can get other places to observe icon changes. (Like every view in the Preferences window, and the optional zooming trigger window.)

@skurfer
Copy link
Member Author

skurfer commented Jan 26, 2016

Redid, rebased, and force-pushed to make Travis happy.

@tiennou
Copy link
Member

tiennou commented Jan 27, 2016

Is that correct?

It is.

but I also put this out there to get your opinion on the changes to the observers.

It seems to make all currently visible QSSearchObjectView handle every QSObjectIconModified that gets sent, right ? Taking into account that only objects that are currently showing in these load icons, I don't think it is that much performance though, but I think your calculation might be off by # of results 😉 (which is actually what the index-wrangling thingy that you're removing from QSResultController checks).

I’d also like to see if we can get other places to observe icon changes.

I think it's because the "icon change notification" thingy is made at the wrong level — IIRC most views in prefs are Tables, and those tables use QSObjectCell, which doesn't listen to the notification at all. Maybe overriding -setRepresentedObject: to do the notification-dance and a quick [[self controlView] setNeedsDisplay] could do the trick ?

Well, it doesn't (or at least it does the trick for a completely unrelated fix 😆). I remember trying to fix that, so here's the explanation : most of the tables in preferences use bindings, which don't cause icon loads because bindings. I'm not sure if it's because of the cell change above unrelated, but if you cause a icon load from the main interface then at dismissal you'll have your icon. Now I know why QSIconLoader exists...

Cell change (because I'll hard reset in 3, 2, 1...)

- (void)setRepresentedObject:(id)representedObject {
    [[NSNotificationCenter defaultCenter] removeObserver:self name:QSObjectIconModified object:self.representedObject];
    [super setRepresentedObject:representedObject];
    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(objectIconModified:) name:QSObjectIconModified object:representedObject];
}

- (void)objectIconModified:(NSNotification *)notification {
    [[self controlView] setNeedsDisplay:YES];
}

@skurfer
Copy link
Member Author

skurfer commented Jan 27, 2016

It seems to make all currently visible QSSearchObjectView handle every QSObjectIconModified that gets sent, right ?

Actually, it's all of them. Not just the ones that are visible. But that's 3 at most.

Taking into account that only objects that are currently showing in these load icons, I don't think it is that much performance though, but I think your calculation might be off by # of results 😉

That's what I was thinking. There aren't that many of these notifications being fired at any given time. And I didn't forget that the results are included in that. (I had actually set a "Log Message" break point to show me the count when the notification was received.)

(which is actually what the index-wrangling thingy that you're removing from QSResultController checks).

I know what that code is supposed to do (I wrote it), but it doesn't seem to be working reliably. For what seems like a year, I've only gotten icons for the first 4 or so results. After making this change yesterday, I finally see them all again.

I’d also like to see if we can get other places to observe icon changes.

I think it's because the "icon change notification" thingy is made at the wrong level — IIRC most views in prefs are Tables, and those tables use QSObjectCell, which doesn't listen to the notification at all. Maybe overriding -setRepresentedObject: to do the notification-dance and a quick [[self controlView] setNeedsDisplay] could do the trick ?

Handling it in QSObjectCell would catch a lot of things, but not everything. For example, the reason I started messing with this (again) is that mini icons for things in a collection don't get redrawn because nothing is watching for them. Maybe you're thinking "the icon's already loaded by the time you hit comma", but I made some enhancements in #2092 so combined objects would be shown in their original form whenever possible. To see what I mean, build from that branch, select a few images in Finder and hit ⌘⎋.

Anyway, does the code above seem to work?

@skurfer
Copy link
Member Author

skurfer commented Jan 27, 2016

As mentioned in #2069:

Expire the cache when the interface is dismissed.

The last commit attempts to do this. It was actually releasing the thing too fast, so the command ended up resolving again. To fix that, I moved the delay and hard-coded it to the default (2 seconds).

(FYI, a good way to test that the object in the interface is the one the command executes with is Random Track ⇥ Reveal in iTunes. Just wait 10 or so seconds between calling it up, and running the command. It should reveal the same track you saw in QS.)

That is, once the proxy has been "used", make sure it gets resolved fresh the next time it's requested.
@skurfer
Copy link
Member Author

skurfer commented Nov 4, 2016

Planning to merge this soon if there are no objections. To sum it up:

  • The cache time for proxy objects is now basically obsolete. A proxy should get resolved the first time you ask for it, and retain its value until the interface is dismissed (or a command is executed in the case of triggers and other ways to run commands).
  • Because proxies don’t expire, the value won’t disappear or change if you leave it selected in the interface past the expiration.
  • Because proxies don’t expire, they can watch for icon changes on the resolved object and update their own icon in response. (Most noticeable with the Latest Download proxy.)
  • When an icon is updated, all three panes are told to redraw, which is wasteful but minimally so. And previously, we were looping over things and furiously adding/removing observers in the results list. I’m guessing that was more overhead (and it never seemed to work correctly anyway).

@skurfer
Copy link
Member Author

skurfer commented Nov 4, 2016

Also of note…

The correct cache time for a proxy is somewhere in between “too short for the user to use the thing” and “so long that the wrong thing is still there when the user grabs the proxy a second time”. In other words, it’s a complete guess that can never be correct 100% of the time.

Developers no longer have to do this guessing.

@skurfer skurfer merged commit 7c7c313 into master Nov 16, 2016
2 checks passed
@skurfer skurfer deleted the proxyIcon branch Nov 16, 2016
skurfer added a commit that referenced this issue Nov 16, 2016
@pjrobertson
Copy link
Member

pjrobertson commented Nov 16, 2016

Sorry Rob I’ve been so quiet lately. I do read your messages and nod my head in agreement, so take that as me chiming in! :P

On 16 Nov 2016, at 21:55, Rob McBroom notifications@github.com wrote:

Merged #2179 #2179.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub #2179 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAJLn37Pmd4rm-14uhjbyHvz966c7u5lks5q-wtSgaJpZM4HMm_o.

@skurfer
Copy link
Member Author

skurfer commented Nov 16, 2016

Good to know. Thanks.

dispatch_time_t popTime = dispatch_time(DISPATCH_TIME_NOW, (int64_t)interval * NSEC_PER_SEC);
dispatch_after(popTime, dispatch_get_main_queue(), ^(void){
[cache removeObjectForKey:QSProxyTargetCache];
});
}
Copy link
Member

@pjrobertson pjrobertson Feb 16, 2017

Choose a reason for hiding this comment

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

@skurfer so it seems this is already getting called after a command has executed so like you I'm stumped as to why we need the timer at all. Perhaps for abbreviations? But if the command execution, abbreviation setting, proxy releasing is all run on the same thread (it looks like it is) then it should already be synchronous and there shouldn't be a need for this.

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