-
Notifications
You must be signed in to change notification settings - Fork 26
Check for update on application start #28
base: master
Are you sure you want to change the base?
Conversation
@lognaturel Thanks for updating me. I am glad to know that it looks good. |
When will this be merged? Just a followup. |
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. |
Hey @lognaturel, Based on your comment I noticed that the gif has About button in focus. Its a bit misleading. Here is the code which triggers Update check: https://github.com/opendatakit/xlsform-offline/pull/28/files#diff-0c81b9c94c68ea34c497c1fa52daee56R258 |
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:
@yanokwa, perhaps we could bring QA in for this? |
@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 Looks like you have Windows, so you can confirm this by setting As far as what to do next, we sort of have two options.
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. |
@yanokwa Thanks for the explanation. I appreciate it. I like the idea of working on pyxform as well. |
This PR aims to resolve issue #13