Closed sonic2kk closed 1 year ago
I was thinking of something similar as well. Thanks :tada:
The default host is google.com [...] ping github.com instead
I think it should ping GitHub as it is the "most important website" for ProtonUp-Qt. If GitHub isn't reachable for some reason, it will return False. Though it doesn't indicate whether GitHub is down or the user has no internet, but I don't think that's a problem for now.
The advantage of https://google.com
is, that the page is only 224 Bytes (because it says the page has been moved to www.google.com, intersting :smile:), so we would need to find a GitHub page that is small.
EDIT: https://api.github.com/repos/
is only 88 Bytes.
The default timeout passed to requests is 10
I wonder in how many cases a response time above 1 second will occur. In that case, the app will freeze for the response or timeout time. We can easily make it async though if that is a problem.
[...] so I replaced it with a generic set_default_statusbar
That's good. In case we want to add some other message later, we can easily do it there.
[...] it will re-check if we're offline and update the message properly
Makes sense, it could go offline later on
I agree that GitHub is probably the "most important", part of why I had the host option was with a check to make sure GitHub was up before trying to download a compatibility tool. I wasn't sure if it made sense to always ping GitHub by default but I agree it is probably the most practical, since a lot of ProtonUp-Qt relies on GitHub being up.
EDIT:
https://api.github.com/repos/
is only 88 Bytes.
I checked the URL out of curiosity of why a response for a list of repos would be so small and had a chuckle when it said "not found" :laughing: Good thinking though, that keeps it small.
I wonder in how many cases a response time above 1 second will occur. In that case, the app will freeze for the response or timeout time.
I reduced the timeout to 3 seconds but I can reduce it to 1 second if it's preferable, I just like leaving a bit of headroom where possible but I'm no expert here. And if there are cases where other timeout values are needed the timeout
parameter can be defined per-call.
Hooray for re-use :-)
I checked the URL out of curiosity
Yeah, I tried some URLs and found that a not found API response is the shortest 🤣
I reduced the timeout to 3 seconds
I guess that's fine.
I think https://github.com/DavidoTek/ProtonUp-Qt/pull/163 and #164
can be merged now.
Thanks!
Had this idea as part of my comments on #163.
Adds an offline check and indicates when the user is offline in the StatusBar by showing a message like
ProtonUp-Qt 2.7.6 (Offline)
. It does this with a newly createdutil
method calledis_online
. This pings a given host with a specified timeout usingrequests
, and determines if a user is offline by returning False if either aConnectionError
orTimeout
exception is raised. Otherwise we returnTrue
.The default host is
google.com
because it has fairly reliable uptime and a short response time. The default timeout passed to requests is10
. Howeveris_online
can take ahost
andtimeout
argument, to help keep it flexible (for example, if we need to pinggithub.com
instead).The logic for resetting the statusbar message back to just
ProtonUp-Qt 1.2.3
was repeated in a few places, so I replaced it with a genericset_default_statusbar
which currently has an if/else. However, based on my comments in #163, this could have anelif
to show if the GitHub Rate Limit is reached.Using this function also means when the user interacts with a part of the code that updates this statusbar message (such as trying to download a compatibility tool, where it changes to "Fetching releases..." and then back to the default statusbar message), it will re-check if we're offline and update the message properly.
This was an off-the-cuff idea I had when writing the comment on the mentioned PR and some of the foundation of this PR could help with the suggestions I gave there too, and I figured I could try to help out with implementing this. As usual I'm always open to feedback :-)
Thanks!