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

Check for downloads - inconsistencies #640

Closed
pjrobertson opened this issue Jan 16, 2012 · 8 comments · Fixed by #641
Closed

Check for downloads - inconsistencies #640

pjrobertson opened this issue Jan 16, 2012 · 8 comments · Fixed by #641

Comments

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 16, 2012

There are a couple of things I noticed whilst being on a non-web connected computer for a while, and clicking the 'check for updates now' button in the prefs

  • Quicksilver occasionally reported that an update was available when there was no internet connection, and would give a blank version number
  • If there's no internet connection, Quicksilver can also report that 'everything is up to date'. There should really be another popup saying 'Unable to connect to the internet'

I know @tiennou has recently overhauled the Update system, does this line [1] just need modifying to store the error and to check it.

[1]

NSData *data = [NSURLConnection sendSynchronousRequest:theRequest returningResponse:nil error:nil];

@tiennou
Copy link
Member

@tiennou tiennou commented Jan 16, 2012

I've seen it too. I've even seen it propose updates while offline (!). And it will cause a hang at launch if there's no network (and everything goes fine) since it's a synchronous call.

I guess you're right about the line you've pointed out, we don't check the error that could be returned... I'd say another value to the enum above would take care of that ;-).

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 16, 2012

I've even seen it propose updates while offline (!)

That's what I've seen, referring to a version with no version number.

Are you happy to work on this, as you know it best (the enums stuff etc.) or are you too busy atm? :)

@tiennou
Copy link
Member

@tiennou tiennou commented Jan 16, 2012

Sadly, I'm busy, and I will be for at least 1 month... But it's not too hard, so I'm giving directions :

  • add a kQSUpdateCheckNoNetwork to the enum
  • return that if -[NSURLConnection sendSynchronousRequest:] returns nil
  • handle that in the switch here
  • check, check, check ;-).

This will fix the no-version-number dialog, but not the stall at startup if there's no network. This would require changing the NSURLConnection call to +connectionWithRequest:delegate or +sendAsynchronousRequest:queue:completionHandler: and massage the rest back in good shape. Slightly harder ;-).

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 16, 2012

If you've got a second:

This
line seems completely ludicrous! (made in this commit.

It seems like it's trying to do what we're talking about, but checking for
a string to have no length then a length > 10 must be wrong :/

On 16 January 2012 20:59, Etienne Samson <
reply@reply.github.com

wrote:

Sadly, I'm busy, and I will be for at least 1 month... But it's not too
hard, so I'm giving directions :

  • add a kQSUpdateCheckNoNetwork to the enum
  • return that if -[NSURLConnection sendSynchronousRequest:] returns nil
  • handle that in the switch here
  • check, check, check ;-).

This will fix the no-version-number dialog, but not the stall at startup
if there's no network. This would require changing the NSURLConnection call
to +connectionWithRequest:delegate or
+sendAsynchronousRequest:queue:completionHandler: and massage the rest
back in good shape. Slightly harder ;-).


Reply to this email directly or view it on GitHub:
#640 (comment)

@tiennou
Copy link
Member

@tiennou tiennou commented Jan 16, 2012

That's what I thought too ;-). But that's how it was first. This checks that the data wasn't nil (like you would expect when network is down/the update system fails, which is the case we need now) and that it's not too large (like if someone accidentally puts a 10Mb pdf instead of a plain 4 character string).

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 17, 2012

Hmm... but you've changed it to be not quite the same. Previously it was if([checkVersionString length] ...
but you've changed it to if(![checkVersionString length] ...

The ! needs to go round the whole expression, not just the first part (seeing as you've reversed the if/else.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Feb 28, 2012

Wasn't this fixed with #641?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Feb 28, 2012

I believe so, yes. I'm not so sure about the bug with the blank version number, but I'll close and keep testing

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 a pull request may close this issue.

3 participants