1041404 - Switch ConfirmDialog and ConfirmDialogHelper from using strings to using l10nIds

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

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

Three dialog classes in shared code:
 - CustomDialog - https://github.com/mozilla-b2g/gaia/blob/456a2e59c12f18c22a694b4042e33c607b3ce740/shared/js/custom_dialog.js
 - ConfirmDialog - https://github.com/mozilla-b2g/gaia/blob/456a2e59c12f18c22a694b4042e33c607b3ce740/shared/js/confirm.js
 - ConfirmDialogHelper - https://github.com/mozilla-b2g/gaia/blob/456a2e59c12f18c22a694b4042e33c607b3ce740/shared/js/homescreens/confirm_dialog_helper.js

have API that operates on strings passed to them. That makes them not retranslatable.

We should switch it to carry l10nIds.

What it means is that we need to replace code like that:

var MyDialog({title: mozL10n.get('dialogTitle'});

MyDialog.buildDOM = function() {
  element.textContent = options.title;
};

with:

var MyDialog({titleId: 'dialogTitle'});

MyDialog.buildDOM = function() {
  element.setAttribute('data-l10n-id', options.title);
};

Comment 1

[Aarizkuren]

Hi, 
it is my first collaboration at mozilla, if i do something wrong, please say me.
I do pull request with this issue:
https://github.com/mozilla-b2g/gaia/pull/22649

thanks for all

Comment 2

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

Attachment: pull request

Comment 3

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

Hi Aarizkuren!

That's a good start. A few notes I have:

 - in ConfirmDialog, you need to also removeAttribute('data-l10n-id') in hide to prevent retranslation of the nodes on language change.
 - ConfirmDialog is called from multiple places like:
    - shared/js/contacts/import/importer_ui.js
    - apps/communications/contacts/js/contacts.js and all calls to Contacts.confirmDialog
    - apps/communications/dialer/js/call_log.js
    - apps/communications/dialer/js/telephony_helper.js
    - apps/email/js/cards/compose.js
    - apps/email/js/cards/message_list.js
    - apps/email/js/cards/message_reader.js
    - apps/email/js/cards/settings_account.js
    - apps/email/js/mail_app.js
    - apps/email/js/mail_common.js (not sure about it, it may use custom ConfirmDialog)
    - apps/ftu/js/memcard_manager.js
    - apps/ftu/js/sim_manager.js
    - apps/ftu/js/ui.js
    - apps/homescreen/js/grid.js

You can grep for it using:
cd ./gaia
grep -r "ConfirmDialog" ./apps
grep -r "ConfirmDialog" ./shared

and

grep -r "Contacts.confirmDialog" ./apps

Start by fixing all code in apps/*/js, but you may have to fix apps/*/tests as well if they rely on textContent instead of element.getAttribute('data-l10n-id')

---------------------

in CustomDialog:
 - yesText and noText should setAttribute('data-l10n-id, l10nId) instead of createTextNode.
 - decorateWithOptions calls from within CustomDialog.show should pass l10nId as `options` or `options[type]`
 - the signature of the `show` function should be changed to function dialog_show(titleId, msgId, cancel, confirm) where cancel and confirm are {titleId, callback}
 - all calls to CustomDialog must be updated:
   - shared/js/dialer/keypad.js
   - apps/ringtones/js/actions_menu.js
   - apps/ringtones/js/share.js
   - apps/system/js/bluetooth_transfer.js
   - apps/system/js/sound_manager.js
   - apps/system/js/updatable.js
   - apps/system/js/update_manager.js

You can find all uses by:

cd ./gaia
grep -r "CustomDialog" ./apps
grep -r "CustomDialog" ./shared

----------------------

In ConfirmDialogHelper:
  - you should change all relevant attributes from config.title to config.titleId, config.body to config.bodyId and config.confirm.title to config.confirm.titleId and config.cancel.titleId.
  - you will have to update all use cases:
    - shared/elements/gaia_grid/js/items/mozapp.js
    - apps/verticalhome/js/app_manager.js

You can find all uses by:

cd ./gaia
grep -r "ConfirmDialogHelper" ./apps
grep -r "ConfirmDialogHelper" ./shared

-----------------------


Because each class has so many use cases, it may be reasonable for you to create pull request separately for each of the three classes, so that we can easier test the patch and revert if needed.

Let me know if you need more help!

Comment 4

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

Also, it seems that papples covers CustomDialog in bug 1043556. Feel free to file a follow up bug to this one for either of the other two - ConfirmDialogHelper or ConfirmDialog.

Comment 5

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

Assigning to Achal

Comment 6

[Achal]

Attachment: PR to switch ConfirmDialog from using strings to l10ids

Hi Zibi,

Please review this PR

Comment 7

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

Achal: it looks great! 


A few things need to be addressed, I believe:

 - Contacts.confirmDialog calls that originate here: https://github.com/mozilla-b2g/gaia/blob/9f79ffc8f284cda736d2ed64b45c11e9ac578e6a/apps/communications/contacts/js/contacts.js#L1029
 - Email seems to be using ConfirmDialog and you didn't cover that in your patch
 - Tests - I don't see you updating any tests yet. Let's see what TBPL will say but I'm worried that you will have to update calls to ConfirmDialog in tests

Once you get those three things covered, I believe this patch is close to be complete! Awesome job! :)

Comment 8

[Achal]

Email is not using shared(shared/js/confirm.js) ConfirmDialog.
Ex. calls are :-
1. apps/email/js/cards/message_reader.js https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/message_reader.js#L25
2. apps/email/js/cards/settings_account.js https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/settings_account.js#L16
3. https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/message_list.js#L22

and few others. All are using https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/mail_common.js#L1110

Tests: I am working on tests. Give me sometime.

Contacts.confirmDialog : I didn't understand your point.

Correct me if I am wrong.

Comment 9

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

(In reply to Achal from comment #8)
> Email is not using shared(shared/js/confirm.js) ConfirmDialog.

You are right! :)

> Tests: I am working on tests. Give me sometime.

Great!

> Contacts.confirmDialog : I didn't understand your point.

contact.js has a class Contacts which has a method - confirmDialog, which uses shared ConfirmDialog. All calls to Contacts.confirmDialog are passing resolved l10n strings, like this:

https://github.com/mozilla-b2g/gaia/blob/9f79ffc8f284cda736d2ed64b45c11e9ac578e6a/apps/communications/contacts/js/views/settings.js#L609

When you're changing the API you need to update calls to that as well.

Comment 10

[Achal]

> contact.js has a class Contacts which has a method - confirmDialog, which
> uses shared ConfirmDialog. All calls to Contacts.confirmDialog are passing
> resolved l10n strings, like this:
> 
> https://github.com/mozilla-b2g/gaia/blob/
> 9f79ffc8f284cda736d2ed64b45c11e9ac578e6a/apps/communications/contacts/js/
> views/settings.js#L609
> 
> When you're changing the API you need to update calls to that as well.

Done, I squashed with the earlier commit. Tests are in the next commit, though.

Comment 11

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

Comment on attachment 8478433 [details] [review]
PR to switch ConfirmDialog from using strings to l10ids

David, similarly to bug 1043556 which you're reviewing. This is about switching CustomDialog API to use l10nIds.

You have a lot in your review queue, so if you have someone with a shorter queue on your mind, who can review this, feel free to reassign :)

Comment 12

[Achal]

Attachment: Switching ConfirmDialogHelper from using strings to using l10nIds.

Comment 13

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

Comment on attachment 8478433 [details] [review]
PR to switch ConfirmDialog from using strings to l10ids

Alexandre, can you review this patch?

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8478433 [details] [review]
PR to switch ConfirmDialog from using strings to l10ids

I'm not reviewer of anything in gaia but its build system and helper addons.
Hopefully vivien can review this patch of guess who can really do it.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8478433 [details] [review]
PR to switch ConfirmDialog from using strings to l10ids

r+ with nits on the ellipsis character and the string id.

I would like Gandalf to confirm the r+ too, because of the mentioned nits.

Comment 16

[Achal]

Hi Vivien,

I have added a commit on the basis of your suggestions.

Comment 17

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

Comment on attachment 8478433 [details] [review]
PR to switch ConfirmDialog from using strings to l10ids

Looks great!

Comment 18

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

Comment on attachment 8478433 [details] [review]
PR to switch ConfirmDialog from using strings to l10ids

After a bit more debugging, it seems that you are switching `msg` attribute of ConfirmDialog.show in many places from string to l10nId, but you do not switch it here: https://github.com/Achal-Aggarwal/gaia/blob/9936940e201c888066c436ad396e0ddd3b14282c/shared/js/confirm.js#L95

If all ConfirmDialog.show calls send l10nIds as msg, then just switch it inside _show to setAttribute instead of textContent.

If there are calls to ConfirmDialog which require ability to send a hardcoded string, then maybe you need to do similar duality: string vs {id: l10nId} object?

Comment 19

[Achal]

I don't know how I missed that one because on dialog hide I am removing the l10id from the message node https://github.com/Achal-Aggarwal/gaia/blob/9936940e201c888066c436ad396e0ddd3b14282c/shared/js/confirm.js#L39 .

I checked all instances of ConfirmDialog.show and didn't found any using hardcoded string, I am switching the messageNode to use l10id (as I intended to do, but missed somehow).

https://tbpl.mozilla.org/php/getParsedLog.php?id=47358667&tree=Gaia-Try&full=1#error1 error is now fixed.

I think https://tbpl.mozilla.org/php/getParsedLog.php?id=47353128&tree=Gaia-Try&full=1#error0 is not related with the concerned issue.

All test are passing now. https://tbpl.mozilla.org/?rev=7fcd4095b04d5ab4300f762248f9b19dab66589f&tree=Gaia-Try

Comment 20

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

Great!

I'll merge the patch tomorrow morning so that I can monitor if it causes any regressions during the day.

Comment 21

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

Comment on attachment 8478433 [details] [review]
PR to switch ConfirmDialog from using strings to l10ids

Sweet, now it looks good and green. Landing!

Comment 22

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

Commit: https://github.com/mozilla-b2g/gaia/commit/7ce555f51d5f50613722752d4c40307e03ec9ad4
Merge: https://github.com/mozilla-b2g/gaia/commit/a64e1fd938299e291c92ba3ec41c345ed063a1c4

Great work and an awesome patch :)
Thanks so much Achal!

Comment 23

[Achal]

Vivien & Zibi thanks a lot for mentoring and reviewing the patch. :)