Add-on translations can not be used from installTasks

[nvaccessAuto]

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]

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]

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]

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]

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]

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]

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]

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

[nvaccessAuto]

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]

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