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

introduse the ability to postpone a downloaded update #4263

Closed
nvaccessAuto opened this Issue Jul 11, 2014 · 31 comments

Comments

Projects
None yet
6 participants
@nvaccessAuto

nvaccessAuto commented Jul 11, 2014

Reported by zahari_bgr on 2014-07-11 04:43
Hi,
currently, when one downloads an update when using installed copy, they have no other choice except to run the update within the current session.
This is not always the desired behaviour, cause the user may want to do other stuff before that and may need to restart either NVDA or the system. Redownloading is not very nice, cause not everyone have a persistant internet connection, not to menssion the bandwith and traffic.
I propose to be introduced a mechanism for postponing the install of the update and executing it on user's demand.

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jul 11, 2014

Comment 1 by zahari_bgr on 2014-07-11 04:59
My proposal is, when download is finished, instead of the current notification, asking the user wether they want to install the update now or postpone it. A pending update then could be run through the last (otherwise hidden) option in the new Exit dialog (and if it is disabled, from a next to last item in the NVDA menu with the same name - "Run pending updates", which is also hidden if there aren't any pending updates).
I currently store the pending update files in the user config directory and delete them if there is a new pending update. They are also deleted after successfull installation - as usual.
This could be made configurable later, as requested in ticket #2255
To achieve this, I extracted the code for update execution from _downloadSuccess() to a separate function outside any class, and created two more functions askInstallUpdateNow() and isPendingUpdate(). isPendingUpdate() returns the path to a pending update (if any) and None otherwise. The path to a pending update is stored in the state list. executeUpdate() starts the actual update (either passed as a parameter or taken with call to isPendingUpdate()).
askInstallUpdateNow() returns the rezult of the message box asking the user wether they want to start the update now. Its text depends wether the exit dialog is enabled or not.
In _downloadSuccess(), I askInstallUpdate(), and if positive - executeUpdate(), else preserv the update installer file (If there is an error - I cover that) and delete old pending update file (if any).
Apart of the GUI elements, on startup, after NVDA's initialization, I notify the user that there is a pending update with askInstallUpdateNow(), and if yes - I do it.
Do you think this startup notification is not approprid and should be removed, or there should be an option in General settings "Notify for updates ready to be installed on startup", or to leave it as it is?
I currently don't cover the situation when an update is pending and the user does manual check for updates, i.e. I don't check the versions. However, an automatic check should not duplicate reports, since last downloaded update is also in state['dontRemindVersion']. In some situations one could want intentionaly to redownload the update though, so that should be also allowed.

I've created a branch named "t4263" in the following repository:
https://bitbucket.org/zahyur/nvda.git

This ticket originates from ticket #4057 and this branch is based on t4057. In ticket #4057 you will find an attached diff with the initial changes for this ticket.

nvaccessAuto commented Jul 11, 2014

Comment 1 by zahari_bgr on 2014-07-11 04:59
My proposal is, when download is finished, instead of the current notification, asking the user wether they want to install the update now or postpone it. A pending update then could be run through the last (otherwise hidden) option in the new Exit dialog (and if it is disabled, from a next to last item in the NVDA menu with the same name - "Run pending updates", which is also hidden if there aren't any pending updates).
I currently store the pending update files in the user config directory and delete them if there is a new pending update. They are also deleted after successfull installation - as usual.
This could be made configurable later, as requested in ticket #2255
To achieve this, I extracted the code for update execution from _downloadSuccess() to a separate function outside any class, and created two more functions askInstallUpdateNow() and isPendingUpdate(). isPendingUpdate() returns the path to a pending update (if any) and None otherwise. The path to a pending update is stored in the state list. executeUpdate() starts the actual update (either passed as a parameter or taken with call to isPendingUpdate()).
askInstallUpdateNow() returns the rezult of the message box asking the user wether they want to start the update now. Its text depends wether the exit dialog is enabled or not.
In _downloadSuccess(), I askInstallUpdate(), and if positive - executeUpdate(), else preserv the update installer file (If there is an error - I cover that) and delete old pending update file (if any).
Apart of the GUI elements, on startup, after NVDA's initialization, I notify the user that there is a pending update with askInstallUpdateNow(), and if yes - I do it.
Do you think this startup notification is not approprid and should be removed, or there should be an option in General settings "Notify for updates ready to be installed on startup", or to leave it as it is?
I currently don't cover the situation when an update is pending and the user does manual check for updates, i.e. I don't check the versions. However, an automatic check should not duplicate reports, since last downloaded update is also in state['dontRemindVersion']. In some situations one could want intentionaly to redownload the update though, so that should be also allowed.

I've created a branch named "t4263" in the following repository:
https://bitbucket.org/zahyur/nvda.git

This ticket originates from ticket #4057 and this branch is based on t4057. In ticket #4057 you will find an attached diff with the initial changes for this ticket.

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jul 11, 2014

Comment 3 by briang1 on 2014-07-11 09:18
I like this idea. What happens if another update comes down while one is still pending? Does the new one over ride the old update, or would we have a kind of queue situation?
As i say I like the idea. I've often also wanted to perhaps share an update to save having to manually do it on several machines I might have. If its known where its stored it would be easy to simply grab the downloaded file and push it over the network to other machines.

nvaccessAuto commented Jul 11, 2014

Comment 3 by briang1 on 2014-07-11 09:18
I like this idea. What happens if another update comes down while one is still pending? Does the new one over ride the old update, or would we have a kind of queue situation?
As i say I like the idea. I've often also wanted to perhaps share an update to save having to manually do it on several machines I might have. If its known where its stored it would be easy to simply grab the downloaded file and push it over the network to other machines.

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jul 17, 2014

Comment 4 by zahari_bgr on 2014-07-17 08:36
Hi,
I've made some improvements here (I hope the following also answers to the @briang1's questions):

  • added an option for enabling/disabling the startup notification for pending updates. It is also disabled when NVDA was started with --launcher.
  • added an option for choosing the directory to store updates (defaults to userConfig/updates, which is created the first time you import this module)
    -- the preserved updates are moved with changing the path
    -- the preserved updates are not copyed into systemConfig along with other files in userConfig when copying userConfig to systemConfig
    -- When setting the path in General Settings, you are forced to choose a valid, writable directory (you could also reset to default); if at the moment of moving the downloaded file it is not writable - you have the option of choosing another one; at last, the pending file is left in the temp directory.
  • added an option keepUpdatesFor which determins after how many days preserved updates should be deleted. This excludes the update which is currently pending to be installed. Defaults to 0, which meens that every applyed or discarded update will be deleted.

I've also added some documentation for the above options.

I now check the pending version against the new version when performing an update check. I've also changed a little the update notification GUI:

  • When there is a new version that is different than the pending version (or there is no pending update): "Download and install update" is now just "Download update"
  • When the latest version is already pending: You can install it from here or re-download.
    The StaticText in this dialog corresponds to the current situation.

Finaly, I removed the checks for isInstalledCopy() and let UpdateDownloader() to be used for all update downloads - I think it is now safe and it will eliminate some confusion and browser issues.

I've created a test build from my testing branch with this and my other changes, which could be downloaded from:
https://bitbucket.org/zahyur/nvda/downloads/nvda_testing-2.exe
or
http://zahari.tk/nvda_testing-2.exe

When you check for updates from this snapshot, you will get nvda_testing-3.exe, which is exactly the same, except for the version number.

nvaccessAuto commented Jul 17, 2014

Comment 4 by zahari_bgr on 2014-07-17 08:36
Hi,
I've made some improvements here (I hope the following also answers to the @briang1's questions):

  • added an option for enabling/disabling the startup notification for pending updates. It is also disabled when NVDA was started with --launcher.
  • added an option for choosing the directory to store updates (defaults to userConfig/updates, which is created the first time you import this module)
    -- the preserved updates are moved with changing the path
    -- the preserved updates are not copyed into systemConfig along with other files in userConfig when copying userConfig to systemConfig
    -- When setting the path in General Settings, you are forced to choose a valid, writable directory (you could also reset to default); if at the moment of moving the downloaded file it is not writable - you have the option of choosing another one; at last, the pending file is left in the temp directory.
  • added an option keepUpdatesFor which determins after how many days preserved updates should be deleted. This excludes the update which is currently pending to be installed. Defaults to 0, which meens that every applyed or discarded update will be deleted.

I've also added some documentation for the above options.

I now check the pending version against the new version when performing an update check. I've also changed a little the update notification GUI:

  • When there is a new version that is different than the pending version (or there is no pending update): "Download and install update" is now just "Download update"
  • When the latest version is already pending: You can install it from here or re-download.
    The StaticText in this dialog corresponds to the current situation.

Finaly, I removed the checks for isInstalledCopy() and let UpdateDownloader() to be used for all update downloads - I think it is now safe and it will eliminate some confusion and browser issues.

I've created a test build from my testing branch with this and my other changes, which could be downloaded from:
https://bitbucket.org/zahyur/nvda/downloads/nvda_testing-2.exe
or
http://zahari.tk/nvda_testing-2.exe

When you check for updates from this snapshot, you will get nvda_testing-3.exe, which is exactly the same, except for the version number.

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jul 18, 2014

Attachment pendingUpdates.diff added by zahari_bgr on 2014-07-18 04:43
Description:

nvaccessAuto commented Jul 18, 2014

Attachment pendingUpdates.diff added by zahari_bgr on 2014-07-18 04:43
Description:

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Oct 3, 2014

Comment 5 by blindbhavya on 2014-10-03 11:56
Hi.
Great idea, hope the code may be reviewed.
I will be CCing myself to this one for updates on this ticket's progress.

nvaccessAuto commented Oct 3, 2014

Comment 5 by blindbhavya on 2014-10-03 11:56
Hi.
Great idea, hope the code may be reviewed.
I will be CCing myself to this one for updates on this ticket's progress.

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto May 29, 2015

Comment 6 by driemer.riemer@... on 2015-05-29 20:19
Editing again because I submitted early on accident.

Code review:

It looks like you disable everything but the edit box for a directory in secure mode. Shouldn't this be disabled as well?

  •                            _("The directory for storing downloaded NVDA updates not exists or is not writable. Please choose another one."),
    
    Shouldn't this be
    •                            _("The directory for storing downloaded NVDA updates does not exist or is not writable. Please choose another one."),
      
      It may also be more logical to explain this to the user in such words as you don't have the privlages to save updates to this location.
  •                                            if file.startswith("nvda_update_") and file.endswith(".exe"): 
    
    My concern for this code is that someone might be able to maliciously put a file like nvda_virus.exe in the folder, with a virus in it. This code would then be run.
    Since nvda now hashes and verifies updates we should check to make sure that nvaccess actually released this update.
  •            _("Update is ready to install.\nYou can install it now, or install it later from the exit dialog.\nInstall now?") if exitDialogEnabled else
    
  •            # Translators: A message presented when the update has been successfully downloaded asking the user to install or postpone.
    
  •            _("Update is ready to install.\nYou can install it now, or install it later from the NVDA menu.\nInstall now?"), 
    
    Could this be reworked to be a bit more readable? I am not sure quite what is going on here because the ternary statement is a bit long winded.
  •                    with wx.DirDialog(gui.mainFrame, _("Store updates directory not writable. Select a writable one"), defaultPath=storeUpdatesDir) as d:
    
    Could you extract the part of the with statement to a variable and use it in the with statement to make the code a bit more readable? It really doesn't matter either way, I don't know what the project recommends.
  •                            _("The downloaded file could not be moved to a safe location and could disappear on the next system restart.\n"
    
  •                                    "NVDA still can use it during this session, but if you want to preserv it, you can copy it yourselv from:\n"
    
  •                                    "%s" % self.destPath), 
    
    Can you replace this with new python formatting syntax like {0} or {name}? I think this eases the translation process a bit from what I have heard. also yourselv should be yourself.
    Also how will gettext treet \n? Why is there a " and then another " within the string?
    +                        if file.startswith("nvda_update_") and file.endswith(".exe") and (f != state["pendingUpdateFile"]) and (os.stat(f).st_mtime < time.time() - keepUpdatesFor * 86400):
    Maybe put a comment here so people know what this number means.
    +There is also a button called "Use default" for reverting this setting to its defaults, which is a folder called updates under the folder with your user configuration.
    +If there are update files which have been stored in this folder and you change it - they will be moved to the new location.
    Should the fact that this is disabled in secure mode be explained in the manual?

Great work.

nvaccessAuto commented May 29, 2015

Comment 6 by driemer.riemer@... on 2015-05-29 20:19
Editing again because I submitted early on accident.

Code review:

It looks like you disable everything but the edit box for a directory in secure mode. Shouldn't this be disabled as well?

  •                            _("The directory for storing downloaded NVDA updates not exists or is not writable. Please choose another one."),
    
    Shouldn't this be
    •                            _("The directory for storing downloaded NVDA updates does not exist or is not writable. Please choose another one."),
      
      It may also be more logical to explain this to the user in such words as you don't have the privlages to save updates to this location.
  •                                            if file.startswith("nvda_update_") and file.endswith(".exe"): 
    
    My concern for this code is that someone might be able to maliciously put a file like nvda_virus.exe in the folder, with a virus in it. This code would then be run.
    Since nvda now hashes and verifies updates we should check to make sure that nvaccess actually released this update.
  •            _("Update is ready to install.\nYou can install it now, or install it later from the exit dialog.\nInstall now?") if exitDialogEnabled else
    
  •            # Translators: A message presented when the update has been successfully downloaded asking the user to install or postpone.
    
  •            _("Update is ready to install.\nYou can install it now, or install it later from the NVDA menu.\nInstall now?"), 
    
    Could this be reworked to be a bit more readable? I am not sure quite what is going on here because the ternary statement is a bit long winded.
  •                    with wx.DirDialog(gui.mainFrame, _("Store updates directory not writable. Select a writable one"), defaultPath=storeUpdatesDir) as d:
    
    Could you extract the part of the with statement to a variable and use it in the with statement to make the code a bit more readable? It really doesn't matter either way, I don't know what the project recommends.
  •                            _("The downloaded file could not be moved to a safe location and could disappear on the next system restart.\n"
    
  •                                    "NVDA still can use it during this session, but if you want to preserv it, you can copy it yourselv from:\n"
    
  •                                    "%s" % self.destPath), 
    
    Can you replace this with new python formatting syntax like {0} or {name}? I think this eases the translation process a bit from what I have heard. also yourselv should be yourself.
    Also how will gettext treet \n? Why is there a " and then another " within the string?
    +                        if file.startswith("nvda_update_") and file.endswith(".exe") and (f != state["pendingUpdateFile"]) and (os.stat(f).st_mtime < time.time() - keepUpdatesFor * 86400):
    Maybe put a comment here so people know what this number means.
    +There is also a button called "Use default" for reverting this setting to its defaults, which is a folder called updates under the folder with your user configuration.
    +If there are update files which have been stored in this folder and you change it - they will be moved to the new location.
    Should the fact that this is disabled in secure mode be explained in the manual?

Great work.

@zahyur

This comment has been minimized.

Show comment
Hide comment
@zahyur

zahyur Aug 19, 2016

I'm so sorry - I haven't seen that there's code review. I don't mind if someone takes this over - I rarely use Windows those days. Same apply for my other contributions, which are still waiting code review. I'll be glad to see my ideas complemented by others and merged in NVDA, since I implemented them with the hope they'll be of help to users. Now while I'm thinking of it, there were also a few other update-related patches, and in one point I was thinking of separate Update preferences dialog. Of course, this is in case NVAccess approves those propositions, and probably should be a subject of another ticket.

zahyur commented Aug 19, 2016

I'm so sorry - I haven't seen that there's code review. I don't mind if someone takes this over - I rarely use Windows those days. Same apply for my other contributions, which are still waiting code review. I'll be glad to see my ideas complemented by others and merged in NVDA, since I implemented them with the hope they'll be of help to users. Now while I'm thinking of it, there were also a few other update-related patches, and in one point I was thinking of separate Update preferences dialog. Of course, this is in case NVAccess approves those propositions, and probably should be a subject of another ticket.

@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Aug 19, 2016

Collaborator

Do you mind putting this into a pr? That way it can be officially review. Sent from a mobile device.
Please disregard errors as this is a smaller device.

On Aug 19, 2016, at 13:48, Zahari Yurukov notifications@github.com wrote:

I'm so sorry - I haven't seen that there's code review. I don't mind if someone takes this over - I rarely use Windows those days. Same apply for my other contributions, which are still waiting code review. I'll be glad to see my ideas complemented by others and merged in NVDA, since I implemented them with the hope they'll be of help to users. Now while I'm thinking of it, there were also a few other update-related patches, and in one point I was thinking of separate Update preferences dialog. Of course, this is in case NVAccess approves those propositions, and probably should be a subject of another ticket.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Collaborator

derekriemer commented Aug 19, 2016

Do you mind putting this into a pr? That way it can be officially review. Sent from a mobile device.
Please disregard errors as this is a smaller device.

On Aug 19, 2016, at 13:48, Zahari Yurukov notifications@github.com wrote:

I'm so sorry - I haven't seen that there's code review. I don't mind if someone takes this over - I rarely use Windows those days. Same apply for my other contributions, which are still waiting code review. I'll be glad to see my ideas complemented by others and merged in NVDA, since I implemented them with the hope they'll be of help to users. Now while I'm thinking of it, there were also a few other update-related patches, and in one point I was thinking of separate Update preferences dialog. Of course, this is in case NVAccess approves those propositions, and probably should be a subject of another ticket.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@zahyur

This comment has been minimized.

Show comment
Hide comment
@zahyur

zahyur Aug 19, 2016

I'm not interested at this time.

zahyur commented Aug 19, 2016

I'm not interested at this time.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Aug 20, 2016

Collaborator

We can't do any review on the current code as the attachment seems to be lost in the migration to github. @@zahyur, do you still have the original code somewhere?

Collaborator

leonardder commented Aug 20, 2016

We can't do any review on the current code as the attachment seems to be lost in the migration to github. @@zahyur, do you still have the original code somewhere?

@zahyur

This comment has been minimized.

Show comment
Hide comment
@zahyur

zahyur Aug 20, 2016

https://bitbucket.org/zahyur/nvda/
Under the t4263 branch.
Note that it's not up-to-date with the current master from NVAccess.

zahyur commented Aug 20, 2016

https://bitbucket.org/zahyur/nvda/
Under the t4263 branch.
Note that it's not up-to-date with the current master from NVAccess.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Aug 20, 2016

Collaborator

Thanks for this. I've pulled in the branch and tried to do a rebase to current master. Some little merge conflicts i've tried to fix as good as possible.
https://github.com/BabbageCom/nvda/tree/t4263

@derekriemer: Do I understand it correctly that you were willing to do a code review? I'd be happy to collaborate on getting this code into good shape, as I belief that it might be a very handy feature.

Collaborator

leonardder commented Aug 20, 2016

Thanks for this. I've pulled in the branch and tried to do a rebase to current master. Some little merge conflicts i've tried to fix as good as possible.
https://github.com/BabbageCom/nvda/tree/t4263

@derekriemer: Do I understand it correctly that you were willing to do a code review? I'd be happy to collaborate on getting this code into good shape, as I belief that it might be a very handy feature.

@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Aug 20, 2016

Collaborator

Note that I'm not actually authorized to do official code review. I was
just I guess thinking I did at the time, but now that we have pull
requests, that's how any changes get pulled in most efficiently.

On 8/20/2016 8:28 AM, Leonard de Ruijter wrote:

Thanks for this. I've pulled in the branch and tried to do a rebase to
current master. Some little merge conflicts i've tried to fix as good
as possible.
https://github.com/BabbageCom/nvda/tree/t4263

@derekriemer https://github.com/derekriemer: Do I understand it
correctly that you were willing to do a code review? I'd be happy to
collaborate on getting this code into good shape, as I belief that it
might be a very handy feature.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4263 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFGivbpY-XBCVBGwSXiOKSJNhBs6ZET-ks5qhw8ngaJpZM4Joy5b.


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.com
Awesome little hand built weather app!
http://django.derekriemer.com/weather/

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

Collaborator

derekriemer commented Aug 20, 2016

Note that I'm not actually authorized to do official code review. I was
just I guess thinking I did at the time, but now that we have pull
requests, that's how any changes get pulled in most efficiently.

On 8/20/2016 8:28 AM, Leonard de Ruijter wrote:

Thanks for this. I've pulled in the branch and tried to do a rebase to
current master. Some little merge conflicts i've tried to fix as good
as possible.
https://github.com/BabbageCom/nvda/tree/t4263

@derekriemer https://github.com/derekriemer: Do I understand it
correctly that you were willing to do a code review? I'd be happy to
collaborate on getting this code into good shape, as I belief that it
might be a very handy feature.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4263 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFGivbpY-XBCVBGwSXiOKSJNhBs6ZET-ks5qhw8ngaJpZM4Joy5b.


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.com
Awesome little hand built weather app!
http://django.derekriemer.com/weather/

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Aug 20, 2016

Collaborator

But if you have questions about or disagree with what I said, just
ignore it as at the time I was a pretty novice coder.

On 8/20/2016 8:28 AM, Leonard de Ruijter wrote:

Thanks for this. I've pulled in the branch and tried to do a rebase to
current master. Some little merge conflicts i've tried to fix as good
as possible.
https://github.com/BabbageCom/nvda/tree/t4263

@derekriemer https://github.com/derekriemer: Do I understand it
correctly that you were willing to do a code review? I'd be happy to
collaborate on getting this code into good shape, as I belief that it
might be a very handy feature.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4263 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFGivbpY-XBCVBGwSXiOKSJNhBs6ZET-ks5qhw8ngaJpZM4Joy5b.


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.com
Awesome little hand built weather app!
http://django.derekriemer.com/weather/

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

Collaborator

derekriemer commented Aug 20, 2016

But if you have questions about or disagree with what I said, just
ignore it as at the time I was a pretty novice coder.

On 8/20/2016 8:28 AM, Leonard de Ruijter wrote:

Thanks for this. I've pulled in the branch and tried to do a rebase to
current master. Some little merge conflicts i've tried to fix as good
as possible.
https://github.com/BabbageCom/nvda/tree/t4263

@derekriemer https://github.com/derekriemer: Do I understand it
correctly that you were willing to do a code review? I'd be happy to
collaborate on getting this code into good shape, as I belief that it
might be a very handy feature.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4263 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFGivbpY-XBCVBGwSXiOKSJNhBs6ZET-ks5qhw8ngaJpZM4Joy5b.


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.com
Awesome little hand built weather app!
http://django.derekriemer.com/weather/

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Aug 24, 2016

Collaborator
Collaborator

leonardder commented Aug 24, 2016

@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Aug 24, 2016

Collaborator

Sure.

Collaborator

derekriemer commented Aug 24, 2016

Sure.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Sep 3, 2016

Collaborator

Have looked into this again. I have the following thoughts:

  • Several gui messages will have to be redone, including those reported by @derekriemer. Example "Start previously downloaded updates, which are still not applyed"
  • To me, the possibility to choose a location for storing updates is a bit overkill. I think an updates directory in the userconfig is sufficient
Collaborator

leonardder commented Sep 3, 2016

Have looked into this again. I have the following thoughts:

  • Several gui messages will have to be redone, including those reported by @derekriemer. Example "Start previously downloaded updates, which are still not applyed"
  • To me, the possibility to choose a location for storing updates is a bit overkill. I think an updates directory in the userconfig is sufficient
@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Sep 3, 2016

Contributor
Contributor

jcsteh commented Sep 3, 2016

@zahyur

This comment has been minimized.

Show comment
Hide comment
@zahyur

zahyur Sep 4, 2016

Hi,
The idea of choosing the location was if there's a problem with the default location (it's not writable, there's not enough disk space, etc.).
The default temp directory on the system is not a good location, cause it's cleaned up on restart. Also - portable copies.

zahyur commented Sep 4, 2016

Hi,
The idea of choosing the location was if there's a problem with the default location (it's not writable, there's not enough disk space, etc.).
The default temp directory on the system is not a good location, cause it's cleaned up on restart. Also - portable copies.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Sep 4, 2016

Contributor
Contributor

jcsteh commented Sep 4, 2016

@zahyur

This comment has been minimized.

Show comment
Hide comment
@zahyur

zahyur Sep 4, 2016

Hi,
No, no - there's no temporary directory in this patch - it's a directory in the user's config directory - I was just clarifying why this is so.
My point was - what if the user's config directory isn't writable or there's not enough free space on the drive, where it recides.
One possibility is showing an error, and then the user is stuck.
I've just decided it's better user experiance to allow the user to change the download directory.
And if you can change the download directory, why you shouldn't be able to do it permanently?
It's just more cool for more people when they have a choice. If someone is confused - they just don't touch the thing they don't understand and use the defaults. And if they don't have a reason to change the defaults - excellent.
At the other end are Apple, for example, which say "we're smarter than you, we know exactly what is best for you, and you should buy it and use it". Well, I would never use an Apple product, not that it's ridiculously expensive, which is also a good reason, too.

Best wishes,
Zahari

zahyur commented Sep 4, 2016

Hi,
No, no - there's no temporary directory in this patch - it's a directory in the user's config directory - I was just clarifying why this is so.
My point was - what if the user's config directory isn't writable or there's not enough free space on the drive, where it recides.
One possibility is showing an error, and then the user is stuck.
I've just decided it's better user experiance to allow the user to change the download directory.
And if you can change the download directory, why you shouldn't be able to do it permanently?
It's just more cool for more people when they have a choice. If someone is confused - they just don't touch the thing they don't understand and use the defaults. And if they don't have a reason to change the defaults - excellent.
At the other end are Apple, for example, which say "we're smarter than you, we know exactly what is best for you, and you should buy it and use it". Well, I would never use an Apple product, not that it's ridiculously expensive, which is also a good reason, too.

Best wishes,
Zahari

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Sep 4, 2016

Collaborator
Collaborator

leonardder commented Sep 4, 2016

@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Sep 4, 2016

Collaborator

I also agree that this is the better way to go.

On 9/4/2016 12:59 PM, Leonard de Ruijter wrote:

I belief things should be as follows:

  1. Downloads should be saved to the temp folder upon download.
  2. If one decides to posphone, copy de download to the updates folder
    under userconfig
  3. If the latter fails, tell the user that the download can't be saved
    and that he either should install directly or download again later.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4263 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFGivVV7zTfomlkEGGQApRAMevC33kv4ks5qmxUqgaJpZM4Joy5b.


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.com
Awesome little hand built weather app!
http://django.derekriemer.com/weather/

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

Collaborator

derekriemer commented Sep 4, 2016

I also agree that this is the better way to go.

On 9/4/2016 12:59 PM, Leonard de Ruijter wrote:

I belief things should be as follows:

  1. Downloads should be saved to the temp folder upon download.
  2. If one decides to posphone, copy de download to the updates folder
    under userconfig
  3. If the latter fails, tell the user that the download can't be saved
    and that he either should install directly or download again later.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4263 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFGivVV7zTfomlkEGGQApRAMevC33kv4ks5qmxUqgaJpZM4Joy5b.


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.com
Awesome little hand built weather app!
http://django.derekriemer.com/weather/

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@zahyur

This comment has been minimized.

Show comment
Hide comment
@zahyur

zahyur Sep 4, 2016

Hi,
As far as I can remember, it was something like that. You just want to remove the manual choosing of the directory.
I don't mind - I'm just sharing my opinion and my motivations. It was never ment to be something final - it's just how I see the world.
So feel free to express your opinion or change it how you think it fits best. It's what the FLOSS world is all about - as well as I understand it.
And BTW, that's one of the reasons I prefer code over an explanation - we all could understand each other. The other is that I prefer seeing the work done rather than just discussing.
And if I could go offtopic (as I do often recently) - I don't feel sorry for not a one peace of code I've ever wrote, cause I always learn something, and it always worths more than the actual thing.
So keep the good work, fight the good fight, and I hope people will love this feature. Just write code - the words doesn't matter in the software world.

Best wishes,
Zahari

zahyur commented Sep 4, 2016

Hi,
As far as I can remember, it was something like that. You just want to remove the manual choosing of the directory.
I don't mind - I'm just sharing my opinion and my motivations. It was never ment to be something final - it's just how I see the world.
So feel free to express your opinion or change it how you think it fits best. It's what the FLOSS world is all about - as well as I understand it.
And BTW, that's one of the reasons I prefer code over an explanation - we all could understand each other. The other is that I prefer seeing the work done rather than just discussing.
And if I could go offtopic (as I do often recently) - I don't feel sorry for not a one peace of code I've ever wrote, cause I always learn something, and it always worths more than the actual thing.
So keep the good work, fight the good fight, and I hope people will love this feature. Just write code - the words doesn't matter in the software world.

Best wishes,
Zahari

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Sep 4, 2016

Contributor
Contributor

jcsteh commented Sep 4, 2016

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Sep 5, 2016

Collaborator
Collaborator

leonardder commented Sep 5, 2016

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Sep 5, 2016

Contributor
Contributor

jcsteh commented Sep 5, 2016

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Sep 5, 2016

Collaborator
Collaborator

leonardder commented Sep 5, 2016

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Sep 5, 2016

Collaborator
How about the possibility to config how long updates are to be kept?
Honestly, I also tend to remove this and simply choose a default.
Probably just keep a postponed update until either a new one is
downloaded or the particular update has been installed.
Collaborator

leonardder commented Sep 5, 2016

How about the possibility to config how long updates are to be kept?
Honestly, I also tend to remove this and simply choose a default.
Probably just keep a postponed update until either a new one is
downloaded or the particular update has been installed.
@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Sep 5, 2016

Contributor
Contributor

jcsteh commented Sep 5, 2016

@bhavyashah

This comment has been minimized.

Show comment
Hide comment
@bhavyashah

bhavyashah Aug 6, 2017

@jcsteh @leonardder Could you please update us about the status of the code review for this feature?

bhavyashah commented Aug 6, 2017

@jcsteh @leonardder Could you please update us about the status of the code review for this feature?

@jcsteh jcsteh removed their assignment Sep 5, 2017

@nvaccessAuto nvaccessAuto added this to the 2018.2 milestone Mar 28, 2018

michaelDCurran added a commit that referenced this issue Mar 28, 2018

Introduse the ability to postpone a downloaded update #4263 (#6355)
* initial work on postponeing updates

* Removed undesired code from the source and userdocs and added some copyright notices

* Review actions

* Some major code updates to make this actually work, hopefully

* Fixed translator comment

* Some more fixes, still some testing to be done

* Support updating of portable copies

* Review actions

* More review actions

* Actions for user guide

* Fixed issue where a right paren was placed at the wrong position

* Properly evaluate whether to show or hide the pending updates menu item on every time the menu is opened. Binding this evaluation to the item itself made the item never to show up.

* Don't use the file variable name when looping through possible old updates. This broke the loading of the state file. Added state file loading debug logging while at it.

* Properly destroy the UpdateResultDialog when installing a postponed update

* When updating a portable copy, run NVDA with the -m command line argument

* Fix KeyError on updating

* make sure that the update state file will be updated to contain pending update info

* When updating a portable copy, place paths that are provided as command line params between quotes to deal with paths containing spaces

* Fix crash after restart with a postponed update

* No longer add removeFile to the update state, as it is used nowhere

* Call prePopup before raising the UpdateAskInstallDialog and make it modal

* Use runScriptModalDialog for the UpdateAskInstallDialog

* Clear the don't remind version when postponing an update. If we do not do this, a user will never be reminded about a postponed update automatically at all
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment