Skip to content
This repository has been archived by the owner on Apr 30, 2021. It is now read-only.

Check for update on application start #28

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

NizamLZ
Copy link

@NizamLZ NizamLZ commented Aug 2, 2019

This PR aims to resolve issue #13

  1. In a background thread, it will hit the github releases api to get the latest release
  2. If new version is greater, based on the OS (MacOS or windows), correct asset url is picked and displayed in a dialog box as a link.
  3. Also along with the link, github release description will be converted from markdown to html and displayed in the update dialog box.

update_checker_4

@lognaturel
Copy link
Member

This is looking really fantastic, @NizamLZ! Apologies for the silence on the maintainer side -- there've been some big releases on other tools recently and various vacations.

I think @yanokwa will need to take a final look and decide on a merge but I'm really happy with how it looks.

@NizamLZ
Copy link
Author

NizamLZ commented Aug 10, 2019

@lognaturel Thanks for updating me. I am glad to know that it looks good.

@NizamLZ
Copy link
Author

NizamLZ commented Sep 4, 2019

When will this be merged? Just a followup.

@lognaturel
Copy link
Member

Thanks again for your patience. @yanokwa and I have both been out of town but are back.

I do think this is really important to get in given that users are running into issues like this one. Consider linking to this PR in a response to the post to get some more eyes on it.

On closer look, I now see that the UI doesn't show that there is an update available until the About button is tapped. I think it's important that the fact that there's an update is immediately visible on launch. This could be added in a separate PR.

Maybe related, it strikes me as a bit odd that the update text replaces the about text entirely. One way to address both this concern and the prior one would be to change the button text to "Update available" or something like that. But if the button continues to be labeled "About", it seems like there should still be some about text available. Maybe the new screen could be linked to from the top of the about screen, for example.

I haven't been able to build the release because of versioning issues but another thing I'd like to check is how much this increases the binary size by.

@NizamLZ
Copy link
Author

NizamLZ commented Sep 12, 2019

Hey @lognaturel,

Based on your comment I noticed that the gif has About button in focus. Its a bit misleading.
Clicking About button does not perform the update check. Update check happens automatically on launch. About dialog is still intact. No changes in that.

Here is the code which triggers Update check: https://github.com/opendatakit/xlsform-offline/pull/28/files#diff-0c81b9c94c68ea34c497c1fa52daee56R258

@lognaturel
Copy link
Member

Thanks for that extra information, @NizamLZ, I should have pieced that together from the code! I think that behavior is fine. I think it would be good to also add a clickable notice in case someone can't do the update right away but that can come later.

I believe the next two things to verify are:

  • change to binary size
  • correct behavior on various platforms

@yanokwa, perhaps we could bring QA in for this?

@lognaturel
Copy link
Member

I have talked to @yanokwa. He says that given other ongoing projects, he thinks he will have time to look at this when the next pyxform release is ready.

@yanokwa
Copy link
Member

yanokwa commented Sep 21, 2019

@lognaturel It's a big lift for QA to test this and I don't think that's going to be a good use of their time.

@NizamLZ thanks for sending in this PR. I've taken a look and the biggest problem I see is that the dependencies that you have are making it very difficult to package with pyinstaller. This isn't a mistake on your part. It's more that pyinstaller can be miserable to work with.

On macOS, the binary goes up 65 MB to 155 MB. This is probably fixable if we can solve #31.

On Windows, the binary size is much better, but I believe it doesn't actually connect to GitHub because the pkg\xlsform-online-win.spec doesn't have access to a TLS CA certificate bundle to make HTTPS work.

Looks like you have Windows, so you can confirm this by setting console=True in the spec file, running make-win.bat, and then running the dist\win\ODK XLSForm Offline. You'll then see the error message: Could not find a suitable TLS CA Certificate bundle, invalid path... My guess is that you'll have to make some changes in the spec file to include that bundle.

As far as what to do next, we sort of have two options.

  1. You do some research on pyinstaller bundling and see if you can fix the Windows issue. That might give me insight to do the fix on a Mac. Once you find solutions, I’ll review them, but the caveat is that I’m often busy.

  2. We stop working on this PR and have you work on solving issues upstream in https://github.com/xlsform/pyxform.

I bias towards the second option because I think you've been writing good code and I want to make sure that we are aren't wasting your time and instead helping you maximize your impact.

@NizamLZ
Copy link
Author

NizamLZ commented Sep 21, 2019

@yanokwa Thanks for the explanation. I appreciate it.

I like the idea of working on pyxform as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants