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

Add-on translations can not be used from installTasks #2715

Closed
nvaccessAuto opened this issue Oct 13, 2012 · 9 comments
Closed

Add-on translations can not be used from installTasks #2715

nvaccessAuto opened this issue Oct 13, 2012 · 9 comments
Labels
Addon/management In NVDA management of addons by users. bug
Milestone

Comments

@nvaccessAuto
Copy link

Reported by ragb on 2012-10-13 11:05
The add-on translation machhinery can not be used from installTasks module, due to the add-on beeing not present in available add-ons on installTasks.onInstall execute time. This means that the current add-on can not be found, so addonHandler.initTranslation can not instanciate the corrent gettext translation.

Attached patch solves this issue, moving the insertion to addonHandler._availableAddons before installTasks.onInstall execution. I can commit if agreed.

@nvaccessAuto
Copy link
Author

Comment 1 by jteh on 2012-10-14 05:47
I understand the need for this. However, failure of installTasks.onInstall should cause the installation to fail, so it can't be registered as an available add-on until that function has returned. We need to find another solution.

@nvaccessAuto
Copy link
Author

Comment 2 by ragb (in reply to comment 1) on 2012-10-14 10:42
Replying to jteh:

I understand the need for this. However, failure of installTasks.onInstall should cause the installation to fail, so it can't be registered as an available add-on until that function has returned. We need to find another solution.

I understand your point but (semantic arguing coming): I explicitly remove the add-on on the except block for that reason, from the _availableAddons dictionary. Beeing in the pending-install state the add-on is not running, and not considered as such (see Addon.isRunning) açtjpigj beeomg available. For me, having a pending-install add-on running an installation task (and as it is executing something, it must available somehow) is not that bad. At least that is the cleanest solution I could find.

Another solution would be to have another variable, say pending-install-and-executing-install-tasks add-ons, (not with this naming for sure) and check also that in addonHandler.getCodeAddon().

P.S. There is a strange log call on my patch that is not suposed to be there :).

@nvaccessAuto
Copy link
Author

Comment 3 by ragb (in reply to comment 2) on 2012-10-14 10:45
Replying to ragb:

Another solution would be to have another variable, say pending-install-and-executing-install-tasks add-ons, (not with this naming for sure) and check also that in addonHandler.getCodeAddon().

Actualy only one add-on can be in this state at the same time :).

@nvaccessAuto
Copy link
Author

Comment 4 by jteh on 2012-10-15 00:36
Fair enough argument. :) I'm happy for you to commit this given the following:

  • Remove the log message.
  • I see you've added an abspath where there wasn't one before. Please confirm this is necessary and remove if not.
  • Please add an entry to the Changes for Developers section of the What's New describing the bug fix.
    Changes:
    Milestone changed from None to 2012.3

@nvaccessAuto
Copy link
Author

Comment 5 by ragb (in reply to comment 4) on 2012-10-15 20:19
Replying to jteh:

Fair enough argument. :) I'm happy for you to commit this given the following:

  • Remove the log message.

Sure! :)

  • I see you've added an abspath where there wasn't one before. Please confirm this is necessary and remove if not.

Oh, I forgot that on my last comment. addonHandler.getCodeAddon() relies on absolute paths. The Addon constructor forces the path to be absolute for that porpose. I'll use the path value returned by the Addon instance just to be consistent with the remaining code, instead of forcing the absolute path in the installation functions.

  • Please add an entry to the Changes for Developers section of the What's New describing the bug fix.

Ok.

@nvaccessAuto
Copy link
Author

Comment 6 by ragb on 2012-10-16 10:07
This is getting a bit huglier. We also must have in acount the add-on removal process, which implies installTasks loading and addonHandler.initTranslation call, although no GUI code is allowed. (addonHandler.initTranslation is called in the module scope).

To solve this I also temporarely add the add-on to _availableAddons when executing the onUninstall task. See updated patch.

@nvaccessAuto
Copy link
Author

Attachment addons-installTasks.patch added by ragb on 2012-10-16 10:08
Description:

@nvaccessAuto
Copy link
Author

Comment 7 by jteh on 2012-10-16 23:24
Urg. It's not amazingly elegant, but having another variable isn't really much better. Please go ahead and commit with one more change: put a comment above the insertions into _availableAddons explaining why we're doing it. Something like this:

# #2715: The add-on must be added to _availableAddons here so that translations can be used in installTasks.

Thanks.

@nvaccessAuto
Copy link
Author

Comment 8 by ragb on 2012-10-17 13:19
Fixed in changeset:main-5578.
Changes:
State: closed

@nvaccessAuto nvaccessAuto added bug Addon/management In NVDA management of addons by users. labels Nov 10, 2015
@nvaccessAuto nvaccessAuto added this to the 2012.3 milestone Nov 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Addon/management In NVDA management of addons by users. bug
Projects
None yet
Development

No branches or pull requests

1 participant