Closed Bug 1061438 Opened 6 years ago Closed 6 years ago

[Ringtones] The wording is incorrect for dialog of share and delete alerts

Categories

(Firefox OS Graveyard :: Gaia::Ringtones, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S4 (12sep)
Tracking Status
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: ashiue, Assigned: squib)

Details

(Keywords: late-l10n)

Attachments

(2 files)

600.53 KB, image/jpeg
Details
46 bytes, text/x-github-pull-request
djf
: review+
kcaldwell
: ui-review+
Details | Review
Gaia      e7d31f0e9b6b19d9b484eeec8fb980718bc40d79
Gecko     https://hg.mozilla.org/mozilla-central/rev/532b5fb77ba1
BuildID   20140901160203
Version   34.0a1

STR:
1. Insert a SD card with some songs in /Notifications folder
2. Go to Settings -> Sound
3. Under the "Tones" category, select "Manage Tones"
4. Select the "..." button to select a song under “My Alerts” section
5. Press the "Delete Alert" button 

Expect result:
1. In step 4, a dialog opens at the bottom of the screen allowing you to share the alert, wordings on dialog should be "Share alert" and "Delete alert"
2. In step 5, a dialog opens with message: "Delete Alert | Are you sure you want to delete <Filename>?" 

Actual result:
1. In step 4, the wordings on dialog are "Share ringtone" and "Delete ringtone"
2. In step 5, the wording on dialog is "Delete Ringtone"
[Blocking Requested - why for this release]:
Obviously error
blocking-b2g: --- → 2.1?
QA Whiteboard: [COM=Gaia::Ringtones]
Besides, deleting alert also show "Ringtone deleted" message
Katie: Do you have any specific ideas for strings here?

(Note that if this is blocking 2.1, we'll obviously need late-l10n for it.)
Flags: needinfo?(kcaldwell)
String should be as specific as possible to confirm the users choice (as noted above in bug description for 'expected result').
For My Alerts:
• Delete alert
• Share alert

For My Ringtones:
• Delete ringtone
• Share ringtone
Flags: needinfo?(kcaldwell)
Attached image Saving_alerts.jpg
Another incorrect wording, show "Saving ringtone..." while saving alert
Not a critical functional issue but user experience/confusion if saving and deleting alerts are addressed as Ringtones. 

Fix is straightforward -- if it can be accommodated with late l10n for 2.1; NI Bhavana/Fabrice for change approval.
Assignee: nobody → squibblyflabbetydoo
Flags: needinfo?(fabrice)
Flags: needinfo?(bbajaj)
Keywords: late-l10n
Target Milestone: --- → 2.1 S4 (12sep)
Please ask for approval once the patch has landed on master.
Flags: needinfo?(fabrice)
(In reply to Hema Koka [:hema] from comment #6)
> Not a critical functional issue but user experience/confusion if saving and
> deleting alerts are addressed as Ringtones. 
> 
> Fix is straightforward -- if it can be accommodated with late l10n for 2.1;
> NI Bhavana/Fabrice for change approval.

Agree with comment#7 once this is ready, but please keep the string freeze date in mind (9/14) after which this cannot land on 2.1
Flags: needinfo?(bbajaj)
Attached file Fix it
Ok, this fixes it. I feel like there might be a smarter way to change the L10N IDs in my code, but this is probably fine.
Attachment #8484540 - Flags: ui-review?(kcaldwell)
Attachment #8484540 - Flags: review?(dflanagan)
Comment on attachment 8484540 [details] [review]
Fix it

r- because I think the code that returns the type of an sdcard ringtone might fail on devices that have only a single storage area when there are files in subdirectories...  If you have a filename like /Ringtones/happy/song.mp3, will you try to base type type on "song"?  Or perhaps the code works as it is, but if so, I think it depends on the presence or absence of a leading slash, which seems like a kind of brittle thing to depend on. I think you'd be better off if your SDCardRingtone() constructor just took the type as its second argument.

Also, you have not fixed the issue brought up in comment #5. That looks like it will be a change in the settings app. Please either fix that here or file a followup bug for it. In that case, since the string is transient, I'd suggest just removing the type completely and changing the string to "Saving..."

Other than those two items, this patch looks solid. I have not tried running it though.
Attachment #8484540 - Flags: review?(dflanagan) → review-
(In reply to David Flanagan [:djf] from comment #10)
> Comment on attachment 8484540 [details] [review]
> Fix it
> 
> r- because I think the code that returns the type of an sdcard ringtone
> might fail on devices that have only a single storage area when there are
> files in subdirectories...  If you have a filename like
> /Ringtones/happy/song.mp3, will you try to base type type on "song"?  Or
> perhaps the code works as it is, but if so, I think it depends on the
> presence or absence of a leading slash, which seems like a kind of brittle
> thing to depend on.

I'm doing essentially the same thing as the music app does to *hide* SD card ringtones, so I think I'm safe here. I had talked to Dave Hylands about how to structure the regex to match paths properly. The only time we should have no leading slash is on desktop. On devices, we should always have "/volume-name/Ringtones/.../blah.mp3".

> I think you'd be better off if your SDCardRingtone() constructor just took the type as its second
> argument.

I can't do that unless I change how IDs are built for SD card ringtones, since their format is "sdcard:/path/to/file.mp3". I *could* change it now, since we don't have an actual release that supports SD card ringtones yet, but it'd screw things up for dogfooders, and I'm not totally sure we need to worry about it.
 
> Also, you have not fixed the issue brought up in comment #5. That looks like
> it will be a change in the settings app. Please either fix that here or file
> a followup bug for it. In that case, since the string is transient, I'd
> suggest just removing the type completely and changing the string to
> "Saving..."

Oops, I forgot about that part.
Attachment #8484540 - Flags: ui-review?(kcaldwell) → ui-review+
Comment on attachment 8484540 [details] [review]
Fix it

Ok, I changed things around a bit. I'm still using a regex to get the ringtone type from the pathname, but I could theoretically change that so that SD card ringtone IDs look like "sdcard:ringtone//path/to/ringtone.mp3", which is similar to how I store built-in ringtone IDs. That (slightly) breaks things for dogfooders though.
Attachment #8484540 - Flags: review- → review?(dflanagan)
Blocking: Not making it a blocker, but we will address the issue and request for uplift

Jim, once this lands on master. Please ask Bhavana or Fabrice for 2.1 approval.

(Note this work needs to land by end of sprint 4 - sep 12 due to string freeze deadline)
blocking-b2g: 2.1? → ---
Comment on attachment 8484540 [details] [review]
Fix it

Looks good. Please check the indentation levels in the new pathToType() function before landing, however.
Attachment #8484540 - Flags: review?(dflanagan) → review+
Gaia-Try reported some test failures, so I'm waiting to see a green run and then I'll land this.
Landed: https://github.com/mozilla-b2g/gaia/pull/23743
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 8484540 [details] [review]
Fix it

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1027995
[User impact] if declined: Users may be confused by unclear/inaccurate wording when working with alert tones
[Testing completed]: Manually tested with marionette tests to back things up
[Risk to taking this patch] (and alternatives if risky): Low-risk aside from late-l10n
[String changes made]: See here: https://github.com/mozilla-b2g/gaia/pull/23743/files#diff-5
Attachment #8484540 - Flags: approval-gaia-v2.1?
Comment on attachment 8484540 [details] [review]
Fix it

just in time to manage the l10n risk !
Attachment #8484540 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Verified on KK builds

[2.2]
aia-Rev        3c898380b47f298cd3b7a0dacb3a6529e94322d4
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/4cdc4b9e5832
Build-ID        20140922184244
Version         35.0a1

[2.1]
Gaia-Rev        3742913e11f69e789dcb0aa0dedf2e5572da0129
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/df42b05782aa
Build-ID        20140922185144
Version         34.0a2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.