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

Clear caches when downgrading as well as upgrading QS #1762

Merged
merged 1 commit into from Feb 2, 2014

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 30, 2014

As Rob and I discussed on quicksilver/elements.trigger.event-qsplugin#2, it's also often worth clearing caches when downgrading QS.

Anyhow, rescanning the catalog is hardly a big deal. I think this should go in v1.2 (but I'm pulling against master, I hope that's OK :/)

@@ -1004,11 +1005,11 @@ - (void)startQuicksilver:(id)sender {
if ([defaults boolForKey:kAutomaticTaskViewer])
[QSTaskViewer sharedInstance];

if ( ! (runningSetupAssistant || newVersion) )
if ( ! (runningSetupAssistant || versionChanged) )
Copy link
Member

@skurfer skurfer Jan 30, 2014

Choose a reason for hiding this comment

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

Should this be if (!runningSetupAssistant && versionChanged). The meaning of the BOOL has changed, so I think the test needs to be altered slightly. But maybe I’m missing something, because the old line looks wrong, too.

Copy link
Member Author

@pjrobertson pjrobertson Feb 1, 2014

Choose a reason for hiding this comment

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

Well... the meaning of the BOOL is the same really. If it's a new version (newVersion == YES) of Quicksilver, then the version has changed (versionChanged == YES)

I'm not quite sure what you're getting at. Nothing's changed... anyway, that line is right:

If we're not running the setup assistant or the version number hasn't changed (i.e. this is a normal launch) then just rescan all catalog indexes as usual. Rescanning isn't the same as wiping and scanning (because the scan isn't forced).
If you follow the code that line calls -[QSLibrarian scanCatalogIgnoringIndexes:NO] i.e. - not a forced scan.

Copy link
Member Author

@pjrobertson pjrobertson Feb 1, 2014

Choose a reason for hiding this comment

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

aaaah dammitt, stupid internet meant I lost my post. Here's the jist of it:

The meaning hasn't changed, just expanded. versionChanged == (newVersion || oldVersion)
That line is right: if this is a normal QS launch (not an upgrade/downgrade and the setup assistant isn't running) then just scan the catalog as normal. The next line doesn't force a rescan (follow it down through the code, you'll see a rescan is done with force == NO)

Trying to pick holes in this tiny commit eh? ;-)

Copy link
Member

@skurfer skurfer Feb 2, 2014

Choose a reason for hiding this comment

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

OK, I think you’re right. I hadn’t followed rescanItems: down far enough.

skurfer added a commit that referenced this issue Feb 2, 2014
Clear caches when downgrading as well as upgrading QS
@skurfer skurfer merged commit ca29d2d into master Feb 2, 2014
1 check passed
@skurfer skurfer deleted the downgradeClearCaches branch Feb 2, 2014
@skurfer
Copy link
Member

@skurfer skurfer commented Feb 2, 2014

I think this should go in v1.2 (but I'm pulling against master, I hope that's OK :/)

Forgot to say… Yeah, that’s the right way to go. Until the release goes out, I’m just rebasing the release branch on master whenever it changes and updating the release notes. That should make it smooth when we merge back into master.

skurfer added a commit that referenced this issue Feb 2, 2014
skurfer added a commit that referenced this issue Feb 5, 2014
skurfer added a commit that referenced this issue Feb 11, 2014
skurfer added a commit that referenced this issue Mar 19, 2014
skurfer added a commit that referenced this issue Apr 14, 2014
skurfer added a commit that referenced this issue May 13, 2014
skurfer added a commit that referenced this issue May 30, 2014
skurfer added a commit that referenced this issue Aug 7, 2014
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