1043556 - Switch shared custom dialogs from using strings to using l10nIds

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Comment 1

[Giovanni Charles]

Attachment: Changes custom dialog to "decorate" it's nodes with the l10n attributes. Changes to all the calling apps included

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8461756 [details] [review]
Changes custom dialog to "decorate" it's nodes with the l10n attributes. Changes to all the calling apps included

I have some minor feedback comments that I posted in the PR. Those should be easy to fix.

One architectural question is if it makes sense to do { id: l10nId } everywhere.

It does seem clean, so that when we have l10nArgs we just do { id: l10nId, args: l10nArgs }, but it also means that for 90% of cases we create a one element object that we pass around.

Alternative would be to use strings for cases with l10nId only, and objects when l10nId/l10nArgs is present and detect the type inside dialogs.js code.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Etienne, what do you think?

Comment 4

[Giovanni Charles]

The rationale was so that, apps could pass strings to the dialogs that they do not wish to be translated. But after going through the use cases, none of the apps required that. So I would now agree.

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 8461756 [details] [review]
Changes custom dialog to "decorate" it's nodes with the l10n attributes. Changes to all the calling apps included

forwarding...

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8461756 [details] [review]
Changes custom dialog to "decorate" it's nodes with the l10n attributes. Changes to all the calling apps included

The change makes sense to me. I don't have strong opinion on object or string about the parameters. We will need app owner's eye on those changes.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8461756 [details] [review]
Changes custom dialog to "decorate" it's nodes with the l10n attributes. Changes to all the calling apps included

I'm not reviewer of any of these files.
Better ask alive for the system app and find whoever owns ringtones, comms and camera.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8461756 [details] [review]
Changes custom dialog to "decorate" it's nodes with the l10n attributes. Changes to all the calling apps included

r=me for system app part

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8461756 [details] [review]
Changes custom dialog to "decorate" it's nodes with the l10n attributes. Changes to all the calling apps included

I'm setting sr+ here because this seems like a valuable change to be making and I approve of that.  Please note, however, that I am about to go on PTO and do not have the time to actually test the code or ensure that all uses of the custom dialog have been converted.

I am also setting r- because:

1) I think there's a bug in one of the system app modules where this code changes the callback functions that are being called.

2) I'd like you to update the CustomDialog.show() documentation since you're changing the api of that shared modules. On this point I'm asserting myself in my official capacity as module owner of shared/js.

3) The place where you add a div inside a button should change. See my github comments.

Thanks for asking me to look this over. Overall this seems like a great change. Ask individual app owners for more specific reviews if you feel like that is necessary.  Some of the system app changes seem like they might be hard to test... Ask Alive or someone for help figuring out how to bring up some of those obscure system app dialogs if you need to.

Comment 10

[Giovanni Charles]

Thank you for the comments, the PR has been updated to include the suggested changes.

I have tested this patch for the system update dialog and the camera's preview manager. I will add reviewers who can confirm that the Bluetooth transfer, sound manager and dialer changes are ok.

Comment 11

[Michael Henretty [:mikehenrty][:mhenretty]]

Hi Giovanni, just an FYI: I got a full plate this week but I will take a look at your patch as soon as I can next week.

Thanks!

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8461756 [details] [review]
Changes custom dialog to "decorate" it's nodes with the l10n attributes. Changes to all the calling apps included

Looks fine to me. Please rebase and make sure your tests are still green before continuing.

Comment 13

[Giovanni Charles]

Great, I have rebased and it passes. This looks ready to go.

Comment 14

[Michael Henretty [:mikehenrty][:mhenretty]]

Great job Giovanni!

master: https://github.com/mozilla-b2g/gaia/commit/c7de02ef1e0ad97d86e5bbef2d19828a236aea27

Comment 15

[Giovanni Charles]

Woooop!

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Excellent work Giovanni! Thanks so much! 68 mozL10n.gets gone and a major API refactored to handle proper l10n. :)

Comment 17

[[e-mail], [e-mail]]

Hi Michael and Giovanni, shall we uplift the patch here for v2.1? Looks like the patch also fixed a blocking issue for no device name of confirmation dialog via Bluetooth receiving files.

Otherwise, you have to create anther patch for fixing the regression only.

https://bugzilla.mozilla.org/show_bug.cgi?id=1082935#c4

Comment 18

[:Eli Perelman]

Regarding what Ian said, we either need to uplift this patch, or create a new patch to resolve an issue with the device name introduced with bug 1082935. Michael, could you advise here?

Comment 19

[Michael Henretty [:mikehenrty][:mhenretty]]

Ah, this is unfortunate. If I understand correctly, we have an uplifted fix in bug 1058675 that depends on the non-uplifted patch in this bug. Looking at that patch, it's not exactly trivial and seems somewhat risky for me to uplift. On the other hand, the change is relatively straightforward but affects a lot of files.

My initial thought is that fixing bug 1082935 to work with the old string custom dialogs is less risky and than uplifting this. Eli, what do you think? Can you estimate how much effort and risk a one-off fix would be here?

Comment 20

[:Eli Perelman]

The fix would literally take less than 5 minutes. I'll prepare a PR for that bug.