Closed
Bug 1061438
Opened 10 years ago
Closed 10 years ago
[Ringtones] The wording is incorrect for dialog of share and delete alerts
Categories
(Firefox OS Graveyard :: Gaia::Ringtones, defect)
Tracking
(b2g-v2.1 verified, b2g-v2.2 verified)
VERIFIED
FIXED
2.1 S4 (12sep)
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+
bajaj
:
approval-gaia-v2.1+
|
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"
Reporter | ||
Comment 1•10 years ago
|
||
[Blocking Requested - why for this release]:
Obviously error
blocking-b2g: --- → 2.1?
QA Whiteboard: [COM=Gaia::Ringtones]
Reporter | ||
Comment 2•10 years ago
|
||
Besides, deleting alert also show "Ringtone deleted" message
Assignee | ||
Comment 3•10 years ago
|
||
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)
Reporter | ||
Comment 5•10 years ago
|
||
Another incorrect wording, show "Saving ringtone..." while saving alert
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
Please ask for approval once the patch has landed on master.
Flags: needinfo?(fabrice)
Comment 8•10 years ago
|
||
(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)
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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-
Assignee | ||
Comment 11•10 years ago
|
||
(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+
Assignee | ||
Comment 12•10 years ago
|
||
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)
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
Gaia-Try reported some test failures, so I'm waiting to see a green run and then I'll land this.
Assignee | ||
Comment 16•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Comment 17•10 years ago
|
||
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 18•10 years ago
|
||
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+
Comment 19•10 years ago
|
||
status-b2g-v2.1:
--- → fixed
status-b2g-v2.2:
--- → fixed
Reporter | ||
Comment 20•10 years ago
|
||
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.
Description
•