[User story] Set a ringtone from a song in the music app

RESOLVED FIXED in 2.0 S3 (6june)

Status

Firefox OS
Gaia::Ringtones
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Sri Kasetti, Assigned: squib)

Tracking

unspecified
2.0 S3 (6june)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap ?

Firefox Tracking Flags

(feature-b2g:2.0)

Details

(Whiteboard: [ucid:media118, 1.4:p2, ft:media] [fxos:media] interaction-design)

User Story

As a user, I want to set a ringtone by using a song from the Music app and I want to have that ringtone appear in the list of available ringtones on my phone

Acceptance Criteria:

1. The user navigates to the sound menu of settings. When The user taps on the change option of the ringer in the sound menu he/she should see a list of ringtones and '+' button on the top right corner
2. Tapping the '+' button takes the user to a menu with 'My Music"" listed as one of the options
3. Select the 'My Music' option will take you to a list of songs aranged in alphabetical order
4. Select a song and that song will be chosen as your ringtone
5. Test if the chosen song does become the device's ringtone and also check if the song has been added to the list of ringtones

Attachments

(4 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
User Story: (updated)
Summary: [User story] Create/Set a ringtone from a song in the music app → [User story] Set a ringtone from a song in the music app

Updated

4 years ago
Blocks: 963897
Flags: in-moztrap?(mozillamarcia.knous)

Updated

4 years ago
Whiteboard: [ucid:media118, 1.4:p2, ft:media] → [ucid:media118, 1.4:p2, ft:media] [fxos:media]
Target Milestone: --- → 1.4 S1 (14feb)
(Assignee)

Comment 1

4 years ago
Taking.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Created attachment 8373748 [details] [review]
Pull request

Here's a work-in-progress pull request if anyone wants to take a look at what I'm doing. It's pretty close to finished and just needs a few things cleaned up.
(Assignee)

Comment 3

4 years ago
Comment on attachment 8373748 [details] [review]
Pull request

David: I'm having a weird issue where setting the ringtone by sharing a song from the music app makes the music app unable to play the song when it returns. Since you worked on setting the ringtone, and you know about indexedDB, could you take a quick look to see if I'm screwing up anything obvious? Thanks!
Attachment #8373748 - Flags: feedback?(dflanagan)
(Assignee)

Comment 4

4 years ago
Oh, and the relevant file is apps/ringtones/js/share.js.
Jim: I can't think of anything in Gaia that you could be doing wrong to prevent music from playing, unless it is an audio competing policy thing, and you know that area better than I do.

There have been a number of bugs involving sharing blobs via activities.  The share used to work when we passed the music file blob to the setringtone app, and it got saved in the settings db. But now we're doign that and passing it to indexeddb, which probably involves another process or something. So maybe something is going wrong in gecko and the blob is getting corrupted back in the music app somehow.

I'd try testing this by making a copy of the blob and passing that copy in the share activity. If that fixes the problem then we need to scream and shout and make bent and khuey fix it for us.  Note that to copy a blob, you'll need to use FileReader to read the entire blob contents into an array buffer and then create a blob from that.  Of course using a memory-backed blob instead of a file-backed blob for the share activity could cause other problems, I suppose.
Comment on attachment 8373748 [details] [review]
Pull request

Clearing the feedback flag.

Jim: I didn't actually look at most of your code.  Let me know if you'd like me to.
Attachment #8373748 - Flags: feedback?(dflanagan)
(Assignee)

Comment 7

4 years ago
Thanks for taking a look, David! I don't think it's an audio competing thing, since it stutters a bit and finally dies. The blob idea seems like a good place to start looking, though. (I also need to double-check this without my patch, but it worked *before* my patch; maybe something broke in the past few days.)
(Assignee)

Comment 8

4 years ago
Jacqueline: I have a couple of UX questions.

1) I think for 1.4, we're not going to manage purchased ringtones via the ringtones app (so they won't show up in the list). Would it be ok to show a "Set purchased ringtone" button in the settings app, below the main ringtones button, that just opens the purchased content pick activity? I can provide some screenshots if that would help.

2) Right now, when we set a tone via the share activity (e.g. click "share" on a song in the music app and share it with the ringtones app), we can only set a ringtone, not an alert tone. Should we add an option to set the song as an alert tone? Also, should we treat user-created ringtones as *only* ringtones, or should the same tone show up in both ringtones and alert tones? I think we should treat user-created tones as ringtones or alert tones, not both; most ringtones make bad alert tones and vice-versa. (Obviously, the difference between user-created ringtones and alert tones won't really matter until we can crop songs.)
Flags: needinfo?(jsavory)
(Reporter)

Comment 9

4 years ago
Adding Rob since Jacqueline is out this week
Flags: needinfo?(rmacdonald)

Updated

4 years ago
QA Contact: marcia
(Reporter)

Updated

4 years ago
Duplicate of this bug: 960330
Hi there - I'm expecting Jacqueline back tomorrow so I'll defer this until then if that's ok. - Rob
Flags: needinfo?(rmacdonald)
(Reporter)

Updated

4 years ago
User Story: (updated)
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
Hi Jim,

1) I worry that adding another button on the Sound settings menu may be a bit confusing for users. I think we want to remain as integrated with the ringtones selection as we can. We could explore other options, however, lets discuss on vidyo to see if we can figure out another solution?

2) I don't believe alert tones were scheduled for 1.4 and were not considered in this round of wires. However, I think that alert tones was one of the stories for the next release so they will be included then. Does this work?
Flags: needinfo?(jsavory) → needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 13

4 years ago
1) I'm not sure we'll have time to modify the locked content app to support the new ringtone app (although that's partly due to my unfamiliarity with locked content). Maybe we can come up with something simple though.

2) Code-wise, alert tones are pretty trivial to handle. I could disable user-created alert tones for 1.4, but I'd only do that if UX doesn't have time to spec things out.

We could meet on Vidyo after the media standup (so around 11 AM PST). Does that work for you? I should also be relatively free the rest of the day too.
Flags: needinfo?(squibblyflabbetydoo)

Updated

4 years ago
Target Milestone: 1.4 S2 (28feb) → ---

Comment 14

4 years ago
Hi Jim

I merged the patch (Locally) to v1.4 and found the following inconsistency:-

1. As in comment#0 as said - Acceptance Criteria: No.1 is still not there. + button is not seen. Also functionality of listing ‘My Music’ not present.

2. When I added a ringtone from my music with long file name it is getting overlapped in settings. This is not your issue (it exists even without your patch).

3. When I add the same ringtones two times duplicate entry is seen in custom ringtones.

4. When I go to settings -> sound -> Manage ringtones -> share a ringtone -> via Message (MMS) -> The ringtone is not appended to the message.

Rest all looked well.

I was able to fix the issue no - 4 (MMS one). It requires a little change in the action_menu.js file from ringtone where you probably missed a name parameter that is required for MMS because of which file shared was not appended. Here is the code - 

    var tone = this.tone;
    var names = [];//add it here

      tone.getBlob(function(blob) {
        names.push(tone.name);//add it here
          type: 'audio/*', //if it is not this way then it should be this way
            number: 1,
            blobs: [blob],
            filenames: names,//add it here
            metadata: [{

Thanks!

Ankit
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 15

4 years ago
1) That's because someone changed the acceptance criteria from under me. I'm actually not working on this bug right now; I'm working on the other parts of the ringtones work, which needs to come first.

2) That's a building blocks bug, I think. I've seen it all over.

3) I doubt we're going to check for that for v2.0, since we're already committing to too much as it is (and in v 2.1, we'll probably let users crop ringtones, making deduplication a lot harder).

4) That's because I haven't tested that part, but thanks for the patch.
Flags: needinfo?(squibblyflabbetydoo)

Updated

4 years ago
Target Milestone: --- → 2.0 S1 (9may)
(Assignee)

Comment 16

4 years ago
Comment on attachment 8373748 [details] [review]
Pull request

David: It'd probably be good for you to start taking a look at this PR, since it's a big one. It's mostly done (finally!). There are just a few things left:

* Show artist info for custom ringtones in the ringtone list (needs some discussion with Tif)
* Probable string changes here and there
* Changes to visuals

Tif: Now's also probably a good time for you to make sure I actually implemented things according to spec! If you need help pulling my branch from GitHub, just let me know and I'll help point you in the right direction.
Attachment #8373748 - Flags: ui-review?(tshakespeare)
Attachment #8373748 - Flags: feedback?(dflanagan)
(Assignee)

Comment 17

4 years ago
Ok, I fixed the first bullet point, so now we should show artist info for ringtones made from songs. I *believe* that was the last significant UX spec thing to do.
Created attachment 8420902 [details]
Ringtons 2.0_Visual Spec.zip

Hello Jim,
The attachment is the Ringtones Visual spec for "create_Ringtones".
Please let me know if you have any questions.
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 19

4 years ago
Looks good, thanks! I mostly finished updating everything to the new style, and just have a couple of questions for Tif about strings (which I'll ask on IRC later).
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 20

4 years ago
One thing I think we still need is an updated icon for the ringtones app. It looks like we're still using the old icon style.
(Assignee)

Comment 21

4 years ago
Comment on attachment 8373748 [details] [review]
Pull request

Switching to a full review, since this is basically done.
Attachment #8373748 - Flags: feedback?(dflanagan) → review?(dflanagan)
(Assignee)

Comment 22

4 years ago
Tests are red for the PR right now because of bug 1009368.
Depends on: 1009368

Updated

4 years ago
Blocks: 982949
Comment on attachment 8373748 [details] [review]
Pull request

Nice job Jim! 

UI-Review - for now while we work out a few issues that Jim and I are discussing over IRC and he is addressing as I go through the UI.

Thanks!
(Assignee)

Comment 25

4 years ago
Another thing we can do for comment 20 is just remove the icon entirely. Thoughts?

Updated

4 years ago
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Comment on attachment 8373748 [details] [review]
Pull request

Adding Carol to take a look from the visual side.
Attachment #8373748 - Flags: ui-review?(chuang)
(Assignee)

Updated

4 years ago
No longer depends on: 1009368
For the create screen, the string next to the checkbox - I talked it out and I think we should change it to "Use as ringtone" and eliminate the second line of text. This is a lot clearer and shorter, which will help with localization. Will make spec update!

Updated

4 years ago
Depends on: 999611
Whiteboard: [ucid:media118, 1.4:p2, ft:media] [fxos:media] → [ucid:media118, 1.4:p2, ft:media] [fxos:media] interaction-design
(Assignee)

Comment 28

4 years ago
(In reply to Jim Porter (:squib) from comment #25)
> Another thing we can do for comment 20 is just remove the icon entirely.
> Thoughts?

I realized we can't do this: then we'd have no icon for the Ringtones app when we're starting a share activity! I think we just need to make updated icons for this...
(Assignee)

Comment 29

4 years ago
Comment on attachment 8373748 [details] [review]
Pull request

Dominic: Do you mind taking a look at the changes I made to the music app here? Thanks!
Attachment #8373748 - Flags: review?(dkuo)
In the PR, it looks like apps/ringtones/style/images/create_ringtone_play@2x.png is a busted image.
Jim,

Sorry I've been slow on reviewing this. Its at the top of my queue now. I'm surprised at how big it is. Would you add a README file (or just add a comment here) giving a basic outline of how it works?  Having a basic orientation to what you've done will help me review it better.
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 32

4 years ago
(In reply to David Flanagan [:djf] from comment #30)
> In the PR, it looks like
> apps/ringtones/style/images/create_ringtone_play@2x.png is a busted image.

Carol: could you upload a fixed version of this image, please?
Flags: needinfo?(squibblyflabbetydoo) → needinfo?(chuang)
(Assignee)

Comment 33

4 years ago
(In reply to David Flanagan [:djf] from comment #31)
> Sorry I've been slow on reviewing this. Its at the top of my queue now. I'm
> surprised at how big it is. Would you add a README file (or just add a
> comment here) giving a basic outline of how it works?  Having a basic
> orientation to what you've done will help me review it better.

Yeah, I'm not too happy about how big this PR became, but due to scheduling on 1.4, it ended up expanding, since I wasn't able to land it bit-by-bit (and trying to manage several branches seemed like overkill at the time).

I've added some more comments explaining various APIs, but here's the gist: there are 3 entry points (one for each .html file): management (from Settings -> Sounds -> Manage Ringtones), pick (from Settings -> Sounds -> Ringer), and share (from Music -> Share).

Management lists all the ringtones (not alert tones) and lets you create/share/delete them. Pick lists all the tones and returns some info about them in the activity. Share creates a new ringtone (and is used by management as a "fake" activity - I just open the window manually).

window.builtInRingtones lets you get the system-provided tones, and window.customRingtones lets you get the user-created ones. All ringtones are represented by an object with, at minimum, the following members: name, id, url, and getBlob().

There are also some generic helpers: ToneList, which represents a single category of tones, and TonePlayer, which handles previewing audio (and validating that the audio isn't corrupt) in the Manage and Pick flows.

The general flow of code for Pick/Manage is to hook up the main UI buttons, then get lists of all the ringtones and push them to the ToneLists.

Hopefully that answers the main questions. The comments in the code should help too.
(Assignee)

Comment 34

4 years ago
Note to self for tomorrow: I need to fix the case where you're deleting a tone that's used as both the default ringer and the default alert tone.
QA Contact: marcia → mozillamarcia.knous
woot! Finally got a Flame! Here are some things I saw (sorry if we have already talked about them!)

- action menu for a ringtone -> share ringtone. it's a little weird that ringtone is listed there
- can you check the tap area for the save button during the create flow? it feels like I have to tap slightly to the left of the word to get it to save. tapping more on the right doesn't do anything. 
- in the manager, playing a sample and then tapping create button should pause the sample. otherwise we're going to have overlapping audio files when playing the song during ringtone creation. I noticed the same thing from Music when creating a ringtone
- do we use "saving ringtone..." in the header because it take a bit to save? I haven't seen this anywhere else. We might want to do something else.
- in the ringer selector, when selecting a different custom ringtone and hitting done, the app seemed to hang for many seconds
- also in the ringer selector, the custom section should be above the alert section
- there's an audible click when opening the selector or manager

Let me know if any of these should be a new bug
(Assignee)

Comment 36

4 years ago
(In reply to Tiffanie Shakespeare from comment #35)
> woot! Finally got a Flame! Here are some things I saw (sorry if we have
> already talked about them!)
> 
> - action menu for a ringtone -> share ringtone. it's a little weird that
> ringtone is listed there

Well, that's what the UX specs ask for. :)

> - can you check the tap area for the save button during the create flow? it
> feels like I have to tap slightly to the left of the word to get it to save.
> tapping more on the right doesn't do anything. 

That's a Haida issue, since the edges of the screen are reserved for edge-swipes.

> - in the manager, playing a sample and then tapping create button should
> pause the sample. otherwise we're going to have overlapping audio files when
> playing the song during ringtone creation. I noticed the same thing from
> Music when creating a ringtone

Fixed. (Music should be fine, since we'll actually pause the music app if you start playing the ringtone preview.)

> - do we use "saving ringtone..." in the header because it take a bit to
> save? I haven't seen this anywhere else. We might want to do something else.

Yes. I'm open to suggestions.

> - in the ringer selector, when selecting a different custom ringtone and
> hitting done, the app seemed to hang for many seconds

Hm, so it turns out that Gecko is super busted and doesn't fire the "canplay" event for Blob URLs for a very long time (after the media is already playing, in fact!). I will find someone to yell at about this.

> - also in the ringer selector, the custom section should be above the alert
> section

I kind of like the custom section being at the bottom since it's easier to get at. Although maybe the custom section should be *first*. That sort of makes sense to me, since you'd only have a custom ringtone if you wanted to *use* it.

> - there's an audible click when opening the selector or manager

I think this happens on gaia/master too. Do you see it there as well?
(Assignee)

Comment 37

4 years ago
(In reply to Jim Porter (:squib) from comment #34)
> Note to self for tomorrow: I need to fix the case where you're deleting a
> tone that's used as both the default ringer and the default alert tone.

Fixed this.

Comment 38

4 years ago
Comment on attachment 8373748 [details] [review]
Pull request

Jim,

The music part looks good to me, for both the code and tests, I only tested it roughly and found the audio competing issue I have mentioned on github, but you have fixed right after my comment so the part also looks good, also I have manually tested sharing to sms and email, it works as usually.
Attachment #8373748 - Flags: review?(dkuo) → review+

Updated

4 years ago
feature-b2g: --- → 2.0
(In reply to Jim Porter (:squib) from comment #36)
> > - action menu for a ringtone -> share ringtone. it's a little weird that
> > ringtone is listed there
> 
> Well, that's what the UX specs ask for. :)

That is kind of weird. I recommend setting some sort of custom property on that share activity request that says "i'm sharing a ringtone" and then put a custom filter in the ringtone manifest so it won't respond to shares that it initiates itself. Actually maybe it would be better to file a gecko bug about this to not allow the same app to be both the initiator and the handler of an activity. That seems guaranteed to cause weird bugs.

I'm trying this out and created my first custom ringtone.  The "Use as ringtone" checkbox confuses me. At first I thought that if I unchecked it, it would make this an alert sound instead of a ringer sound. But I think that it must mean is "Set this as the current ringtone".  If so, can we clarify that string, please?  (Tif, that is probably a question for you.)
Flags: needinfo?(tshakespeare)
(In reply to Jim Porter (:squib) from comment #36)
> (In reply to Tiffanie Shakespeare from comment #35)
> > woot! Finally got a Flame! Here are some things I saw (sorry if we have
> > already talked about them!)
> > 
> > - action menu for a ringtone -> share ringtone. it's a little weird that
> > ringtone is listed there
> 
> Well, that's what the UX specs ask for. :)
> 
Let me clarify - not the more menu, but once you tap "Share ringtone" and see the action menu with email, blue tooth etc. There it has ringtone listed as an option. Check out the v1.6 spec pg 15 screen #4 - that's what I'm talking about.

> > - can you check the tap area for the save button during the create flow? it
> > feels like I have to tap slightly to the left of the word to get it to save.
> > tapping more on the right doesn't do anything. 
> 
> That's a Haida issue, since the edges of the screen are reserved for
> edge-swipes.
> 
I feel like it's more extreme here; I have to tap totally to the left of the word save to get it to work. Whereas in the Manager screen I can tap on the + and it works. I can try to take a video if that would help.

> > - in the manager, playing a sample and then tapping create button should
> > pause the sample. otherwise we're going to have overlapping audio files when
> > playing the song during ringtone creation. I noticed the same thing from
> > Music when creating a ringtone
> 
> Fixed. (Music should be fine, since we'll actually pause the music app if
> you start playing the ringtone preview.)
> 
I'm wondering if we should throw in a tap to stop kind of idea. So tap once to play, tap again to stop. As I've been playing with this, I've realized that it's kind of obnoxious that there's no way to stop the sound other than leaving the screen....

> > - do we use "saving ringtone..." in the header because it take a bit to
> > save? I haven't seen this anywhere else. We might want to do something else.
> 
> Yes. I'm open to suggestions.
> 
Cool - on my list of things to-do.

> > - in the ringer selector, when selecting a different custom ringtone and
> > hitting done, the app seemed to hang for many seconds
> 
> Hm, so it turns out that Gecko is super busted and doesn't fire the
> "canplay" event for Blob URLs for a very long time (after the media is
> already playing, in fact!). I will find someone to yell at about this.
> 
Seems good. LMK if we can't get it fixed as we'll have to throw up a spinner or something.

> > - also in the ringer selector, the custom section should be above the alert
> > section
> 
> I kind of like the custom section being at the bottom since it's easier to
> get at. Although maybe the custom section should be *first*. That sort of
> makes sense to me, since you'd only have a custom ringtone if you wanted to
> *use* it.
> 
Since the user can make a ton of ringtones themselves, we'd want to keep the finite list of ringtones, ours, first and then the "infinite" ringtone list second in the list. Since choosing an alert is less likely, it's ok that the user has to scroll a bit more to get down there. I think it would also be weird to have that section pop in and out at the top of the list as you create the first ringtone or delete the last one. It depends on the delay of the refresh. 

> > - there's an audible click when opening the selector or manager
> 
> I think this happens on gaia/master too. Do you see it there as well?

I've only noticed this with your patch. I didn't hear it when changing the ringer from other patches or master.

Thanks Jim! :)
A few more things as I've been messing around:

- I've noticed that if you are mid-play and you lock your phone. When you unlock it, the ringtone starts playing again. I've also noticed that if you go off and share via like email. When you come back it starts playing again. It's a little startling when you aren't expecting it. I wonder if we should have a semi-blanket rule that leaving the Manager list screen halts the play like with creation?

- In the manager, tapping back transitions forward instead of backward to sounds setting page and there is now an X in the left corner. I'm almost positive I didn't see this yesterday. It seems to consistently happen and doesn't go back to "normal" unless I force close the settings app.
And finally - David's comment!

*sigh * I know, that string has been really hard to write in a concise, clear way. I originally had "default ringtone" but there was some confusion around that. Current might be a better adjective(?) - "Use as current ringtone". 

(In reply to David Flanagan [:djf] from comment #39)

> I'm trying this out and created my first custom ringtone.  The "Use as
> ringtone" checkbox confuses me. At first I thought that if I unchecked it,
> it would make this an alert sound instead of a ringer sound. But I think
> that it must mean is "Set this as the current ringtone".  If so, can we
> clarify that string, please?  (Tif, that is probably a question for you.)
Flags: needinfo?(tshakespeare)
More comments! Aren't you happy I can finally test your patch? :D

- check out pg 21 screen #2: for the action menu from music, can we only list the name of the app instead of saying "Create Ringtone". I don't know how to test the Download Manager, but I assume the question applies for that too (pg 22).

Thanks Jim!
(Assignee)

Comment 44

4 years ago
(In reply to David Flanagan [:djf] from comment #39)
> (In reply to Jim Porter (:squib) from comment #36)
> > > - action menu for a ringtone -> share ringtone. it's a little weird that
> > > ringtone is listed there
> > 
> > Well, that's what the UX specs ask for. :)
> 
> That is kind of weird. I recommend setting some sort of custom property on
> that share activity request that says "i'm sharing a ringtone" and then put
> a custom filter in the ringtone manifest so it won't respond to shares that
> it initiates itself. Actually maybe it would be better to file a gecko bug
> about this to not allow the same app to be both the initiator and the
> handler of an activity. That seems guaranteed to cause weird bugs.

Sorry, at first I thought this was referring to how the actions menu says "Share Ringtone" instead of just "Share".

But I think this is a Gecko bug. An app shouldn't be able to launch an activity that it handles itself.

(In reply to Tiffanie Shakespeare from comment #40)
> > > - can you check the tap area for the save button during the create flow? it
> > > feels like I have to tap slightly to the left of the word to get it to save.
> > > tapping more on the right doesn't do anything. 
> > 
> > That's a Haida issue, since the edges of the screen are reserved for
> > edge-swipes.
> > 
> I feel like it's more extreme here; I have to tap totally to the left of the
> word save to get it to work. Whereas in the Manager screen I can tap on the
> + and it works. I can try to take a video if that would help.

I've had lots of problems everywhere lately. (I really don't like how we implement edge gestures at the moment...) I'll take a look and see if I'm doing something weird, though.

> > > - in the manager, playing a sample and then tapping create button should
> > > pause the sample. otherwise we're going to have overlapping audio files when
> > > playing the song during ringtone creation. I noticed the same thing from
> > > Music when creating a ringtone
> > 
> > Fixed. (Music should be fine, since we'll actually pause the music app if
> > you start playing the ringtone preview.)
> > 
> I'm wondering if we should throw in a tap to stop kind of idea. So tap once
> to play, tap again to stop. As I've been playing with this, I've realized
> that it's kind of obnoxious that there's no way to stop the sound other than
> leaving the screen....

Don't we already do this? If you tap a ringtone, it'll start playing, and if you tap it again, it *should* stop.

> Since the user can make a ton of ringtones themselves, we'd want to keep the
> finite list of ringtones, ours, first and then the "infinite" ringtone list
> second in the list.

A user *can* make infinite ringtones, but I'm not sure they would(?). It seems to me that a ringtone you explicitly created is more important than even the built-in ones, but I'm not the type of person who'd make dozens of custom ringtones...

(In reply to Tiffanie Shakespeare from comment #43)
> - check out pg 21 screen #2: for the action menu from music, can we only
> list the name of the app instead of saying "Create Ringtone". I don't know
> how to test the Download Manager, but I assume the question applies for that
> too (pg 22).

We already do this, I think...
(Assignee)

Comment 45

4 years ago
Ok, I think I've addressed all the code review comments.
(Assignee)

Comment 46

4 years ago
(In reply to Tiffanie Shakespeare from comment #41)
> A few more things as I've been messing around:
> 
> - I've noticed that if you are mid-play and you lock your phone. When you
> unlock it, the ringtone starts playing again. I've also noticed that if you
> go off and share via like email. When you come back it starts playing again.
> It's a little startling when you aren't expecting it. I wonder if we should
> have a semi-blanket rule that leaving the Manager list screen halts the play
> like with creation?

Do you think it would make sense to stop playing immediately once you hit the "create" or "more actions" button? Or should we wait until you've actually left the management window?

> - In the manager, tapping back transitions forward instead of backward to
> sounds setting page and there is now an X in the left corner. I'm almost
> positive I didn't see this yesterday. It seems to consistently happen and
> doesn't go back to "normal" unless I force close the settings app.

You should be able to hit the "X", since I think it's just changing the icon. I'm not sure how we'd fix this, but I've asked about it.
Created attachment 8427575 [details]
Ringtons 2.0_Visual Spec.zip

Hi Jim,
I updated the image for "create_ringtones_play@2x.png".
Please check the attachment again and let me know if you have any other question,thanks!!!
Attachment #8420902 - Attachment is obsolete: true
Flags: needinfo?(chuang) → needinfo?(squibblyflabbetydoo)
Created attachment 8427607 [details]
manage_ringtons_list.JPG

Hi Jim,
There's still some pages that need you to refresh the UI Style, like the "Create Ringtones" page :)

I was looking at the "manage ringtones" list view (see attachment screenshot), please switch the sun icon to "options.png" that in Ringtons 2.0_Visual Spec.
It needs a divider between these two action as well. thank you!
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 49

4 years ago
(In reply to chuang from comment #48)
> There's still some pages that need you to refresh the UI Style, like the
> "Create Ringtones" page :)
> 
> I was looking at the "manage ringtones" list view (see attachment
> screenshot), please switch the sun icon to "options.png" that in Ringtons
> 2.0_Visual Spec.
> It needs a divider between these two action as well. thank you!

Make sure you're on the latest version of my branch, since I've already fixed these things. I've been rebasing my code against gaia/master, so you may need to add the "-f" flag when fetching my code to force it to overwrite the old version of my branch.
I'm leaving lots of comments on github, but few of them are serious issues. A couple do seem more serious, so I want to note them here before I forget them, since I'm worried about them being lost among the nits.

1) I'm worried that we won't be able to hear things like SMS notifications and alarm clock ringers while previewing a ringtone. Can we do that at a lower priority?

2) You deleted all the ringtone names from the english locale file, and I think that means that they won't be localized into other languages.  I suspect we need to consult with Axel about ringtone name localization and at least give him a heads up about the removal of the file type suffix from the l10n id.
(In reply to David Flanagan [:djf] from comment #50)
> 2) You deleted all the ringtone names from the english locale file, and I
> think that means that they won't be localized into other languages.  I
> suspect we need to consult with Axel about ringtone name localization and at
> least give him a heads up about the removal of the file type suffix from the
> l10n id.

Some teams are localizing those names. Take for example "Wind Chimes"
http://transvision.mozfr.org/string/?entity=apps/ringtones/ringtones.properties:ringer_loud_windchimes_opus&repo=gaia

Or "Minimal Bands"
http://transvision.mozfr.org/string/?entity=apps/ringtones/ringtones.properties:notifier_minimal_bands_ogg&repo=gaia

So yes, we definitely need those strings back.
Jim: since the settings db properties have changed slightly (from name to id) what happens when the user upgrades from 1.3 or 1.4 to 2.0?  Will everything work as before, or will settings lose the name of the current ringtone until they set it again? I actually think it is fine if settings forgets the current name. IIRC, the current code forgets the name if the user switches languages, so if it happens on upgrade that's fine with me.  But I do want to know that you've thought this case through and that there won't be any worse consequences (like losing the ringtone blob!)
If we're going to merge ringtones and alert tones into a single view, that means that the user will be able to set a long song as their alert tone.  If I create a ringtone from a 10 minute long song, and then accidentally set that as my alert tone, then when I get a text, I think I'll have to listen to the entire song or reboot my phone, won't I?

So if we're going to allow ringtones to be picked for alert tones, then I think we need to modify the system app so that it automatically stops playing the alert tone after 5 seconds (or something).  Maybe Tif can comment on how that should work.
(Assignee)

Comment 54

4 years ago
(In reply to David Flanagan [:djf] from comment #50)
> I'm leaving lots of comments on github, but few of them are serious issues.
> A couple do seem more serious, so I want to note them here before I forget
> them, since I'm worried about them being lost among the nits.
> 
> 1) I'm worried that we won't be able to hear things like SMS notifications
> and alarm clock ringers while previewing a ringtone. Can we do that at a
> lower priority?

That's just the status quo for the ringtones app now, but we can file a new bug on this.

> 2) You deleted all the ringtone names from the english locale file, and I
> think that means that they won't be localized into other languages.  I
> suspect we need to consult with Axel about ringtone name localization and at
> least give him a heads up about the removal of the file type suffix from the
> l10n id.

See https://github.com/jimporter/gaia/blob/ringtones-merge/shared/locales/sounds/sounds.en-US.properties :)
Consolidating multiple feedback things into one thing...

(In reply to Jim Porter (:squib) from comment #44)
> (In reply to David Flanagan [:djf] from comment #39)
> > (In reply to Jim Porter (:squib) from comment #36)
> > > > - action menu for a ringtone -> share ringtone. it's a little weird that
> > > > ringtone is listed there
> > > 
> > > Well, that's what the UX specs ask for. :)
> > 
> > That is kind of weird. I recommend setting some sort of custom property on
> > that share activity request that says "i'm sharing a ringtone" and then put
> > a custom filter in the ringtone manifest so it won't respond to shares that
> > it initiates itself. Actually maybe it would be better to file a gecko bug
> > about this to not allow the same app to be both the initiator and the
> > handler of an activity. That seems guaranteed to cause weird bugs.
> 
> Sorry, at first I thought this was referring to how the actions menu says
> "Share Ringtone" instead of just "Share".
> 
> But I think this is a Gecko bug. An app shouldn't be able to launch an
> activity that it handles itself.
> 
Do we need to file a new bug for this item? 

> (In reply to Tiffanie Shakespeare from comment #40)
> > > > - can you check the tap area for the save button during the create flow? it
> > > > feels like I have to tap slightly to the left of the word to get it to save.
> > > > tapping more on the right doesn't do anything. 
> > > 
> > > That's a Haida issue, since the edges of the screen are reserved for
> > > edge-swipes.
> > > 
> > I feel like it's more extreme here; I have to tap totally to the left of the
> > word save to get it to work. Whereas in the Manager screen I can tap on the
> > + and it works. I can try to take a video if that would help.
> 
> I've had lots of problems everywhere lately. (I really don't like how we
> implement edge gestures at the moment...) I'll take a look and see if I'm
> doing something weird, though.

Again LMK if we need a new bug, this one is pretty bad because tapping on the item doesn't actually do anything.
> 
> > > > - in the manager, playing a sample and then tapping create button should
> > > > pause the sample. otherwise we're going to have overlapping audio files when
> > > > playing the song during ringtone creation. I noticed the same thing from
> > > > Music when creating a ringtone
> > > 
> > > Fixed. (Music should be fine, since we'll actually pause the music app if
> > > you start playing the ringtone preview.)
> > > 
> > I'm wondering if we should throw in a tap to stop kind of idea. So tap once
> > to play, tap again to stop. As I've been playing with this, I've realized
> > that it's kind of obnoxious that there's no way to stop the sound other than
> > leaving the screen....
> 
> Don't we already do this? If you tap a ringtone, it'll start playing, and if
> you tap it again, it *should* stop.

Uh ya sure - that was a test to make sure you're actually reading my feedback... ;)
> 
> > Since the user can make a ton of ringtones themselves, we'd want to keep the
> > finite list of ringtones, ours, first and then the "infinite" ringtone list
> > second in the list.
> 
> A user *can* make infinite ringtones, but I'm not sure they would(?). It
> seems to me that a ringtone you explicitly created is more important than
> even the built-in ones, but I'm not the type of person who'd make dozens of
> custom ringtones...

Ain't no telling - I'm not much of a ringtone user either. If we do place it first, I want to make sure there aren't any weird animation issues or it doesn't look odd when a new category is placed/removed at the top of the list.
> 
> (In reply to Tiffanie Shakespeare from comment #43)
> > - check out pg 21 screen #2: for the action menu from music, can we only
> > list the name of the app instead of saying "Create Ringtone". I don't know
> > how to test the Download Manager, but I assume the question applies for that
> > too (pg 22).
> 
> We already do this, I think...

After discussion in IRC, I understand now that we are limited to only using the app name in the share activity as well. I will update the spec.

And more replies to other comments...

(In reply to Tiffanie Shakespeare from comment #41)
> A few more things as I've been messing around:
> 
> - I've noticed that if you are mid-play and you lock your phone. When you
> unlock it, the ringtone starts playing again. I've also noticed that if you
> go off and share via like email. When you come back it starts playing again.
> It's a little startling when you aren't expecting it. I wonder if we should
> have a semi-blanket rule that leaving the Manager list screen halts the play
> like with creation?

Jim: Do you think it would make sense to stop playing immediately once you hit the "create" or "more actions" button? Or should we wait until you've actually left the management window?

Tif: I think once you hit create or more actions we can stop playing.

> - In the manager, tapping back transitions forward instead of backward to
> sounds setting page and there is now an X in the left corner. I'm almost
> positive I didn't see this yesterday. It seems to consistently happen and
> doesn't go back to "normal" unless I force close the settings app.

Jim: You should be able to hit the "X", since I think it's just changing the icon. I'm not sure how we'd fix this, but I've asked about it.

Tif: It's more like a bug than something purposeful I think. I actually just repro'd it now. Let me chat with you on IRC about it.


And two more new items! I talked with Harly (Building Blocks) to confirm) - one is a "you're bad" and one is a "my bad" so we're even ;)
- On the setting -> sound page, the sub-header should be Mange Sound (title case). Before you say "Other sounds" isn't title case, I filed a bug to get the rest of the sub-headers fixed Bug 1015377 :)
- The button "Manage ringtones" should also be title case. Sorry about that, I'll fix the spec.

Did I reach a page yet?? Thanks Jim!!
Did some brainstorming with Rob today  - I think "Use as default ringtone" is going to be fine. 

Sorry Jim, can you update that string one more time pretty please?

(In reply to Tiffanie Shakespeare from comment #42)
> And finally - David's comment!
> 
> *sigh * I know, that string has been really hard to write in a concise,
> clear way. I originally had "default ringtone" but there was some confusion
> around that. Current might be a better adjective(?) - "Use as current
> ringtone". 
> 
> (In reply to David Flanagan [:djf] from comment #39)
> 
> > I'm trying this out and created my first custom ringtone.  The "Use as
> > ringtone" checkbox confuses me. At first I thought that if I unchecked it,
> > it would make this an alert sound instead of a ringer sound. But I think
> > that it must mean is "Set this as the current ringtone".  If so, can we
> > clarify that string, please?  (Tif, that is probably a question for you.)
(Assignee)

Comment 57

4 years ago
Comment on attachment 8373748 [details] [review]
Pull request

Yuren: since you looked at bug 982949, could you take a look at the build system changes I made here? Most of these changes are trying to do the same thing as in bug 982949. Thanks!
Attachment #8373748 - Flags: review?(yurenju.mozilla)
(Assignee)

Updated

4 years ago
Depends on: 1015513
(Assignee)

Updated

4 years ago
Depends on: 1015519
(Assignee)

Comment 58

4 years ago
Ok, so I think I've fixed pretty much everything and filed bugs for most of the stuff I can't fix here. The remaining bits I can think of are:

* Figure out when we should close the Ringtone Manager (currently, it closes whenever it's hidden).
* Bug people to support transient Notifications.
* Make sure that Haida edge gestures aren't causing problems with the buttons in our headers.
What about comment 53? We can't allow really long sounds to be played as alert sounds... We at least need a followup bug for that.
Comment on attachment 8373748 [details] [review]
Pull request

This is a huge amount of code, and it looks really solid, Jim.  You've been responding to the comments I have as fast as I write them, so I'm not entirely certain where we stand on the various issues I've raised, but I'm quite comfortable with the overall quality of the code and as far as I'm concerned you should land it when you're done making changes.

If there are any changed bits that you'd like me to re-review, just let me know.
Attachment #8373748 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 61

4 years ago
(In reply to David Flanagan [:djf] from comment #59)
> What about comment 53? We can't allow really long sounds to be played as
> alert sounds... We at least need a followup bug for that.

I tested this out, and you can stop the alert sound in its tracks by hitting the volume down or power buttons, so I'm not too worried about this anymore.
(Assignee)

Comment 62

4 years ago
(In reply to Jim Porter (:squib) from comment #61)
> (In reply to David Flanagan [:djf] from comment #59)
> > What about comment 53? We can't allow really long sounds to be played as
> > alert sounds... We at least need a followup bug for that.
> 
> I tested this out, and you can stop the alert sound in its tracks by hitting
> the volume down or power buttons, so I'm not too worried about this anymore.

Ok, and it cuts itself off automatically after something like 3-5 seconds. Hooray for the comms people!
(Assignee)

Comment 63

4 years ago
David: Oh also, for my own sake, I've been trying to make sure that I've replied to every comment you made on Github to verify to myself that I addressed everything you brought up. I'll probably doublecheck that I didn't miss anything sometime between now and Tuesday.

Comment 64

4 years ago
Hi Jim

I have sent you a mail could you please take a look at it!

thanks!
(Assignee)

Comment 65

4 years ago
Ok, so the email is about Bluetooth share not working. The issue is that Bluetooth expects a real file, but (for custom ringtones), we don't *have* a real file. I'd argue that this is a bug in the Bluetooth code, and either the Gecko part needs to accept arbitrary Blobs, or the Gaia app needs to make temp files when we send arbitrary Blobs.

(I assume that Blobs from indexedDB storage are file-backed somewhere, but I'm not sure we can get this info...)
(Assignee)

Comment 66

4 years ago
Ok, I've gone through all of David's comments and either addressed them, explained why I did what I did, or made a note to follow up on an open issue.

Tif: Here are some questions for you!

1) If you go to change the ringtone in Settings -> Sound, the "Done" button is currently disabled in my branch until you pick a *different* ringtone. Should it still be enabled if we automatically selected one by default? I'm not convinced either way (although leaving "Done" disabled is less work).

2) David noted that there seem to be a lot of subheadings in Settings -> Sound. Is there a way to merge any of them? See here: <https://github.com/mozilla-b2g/gaia/pull/16151#discussion_r13023635>.
Flags: needinfo?(tshakespeare)
(Assignee)

Updated

4 years ago
Blocks: 1015296
(Assignee)

Comment 67

4 years ago
Another question, Tif: I verified that setting a ringtone (which is long) to be the default alert tone cuts the ringtone off after 3-5 seconds. Since that happens, does it really make sense for us to let the user pick a built-in ringtone to use as their alert tone?

I think it's fine to let users set a custom ringtone as their alert tone, since we don't know if that would work well or not (maybe the song is really short, or the user just cares about the very beginning of it), but I guess that's debatable too.

In the longer term, maybe when we support cropping custom ringtones, we can have two different crop modes: one for ringtones and one for alert tones.

This is definitely something I think we should try to handle now, since if we get it right the first time, there'll be less work required for migrating settings when we add cropping.
Comment on attachment 8373748 [details] [review]
Pull request

looks nice, r=yurenju

also feedback? Albert since he implemented customzatation per sim card.
Attachment #8373748 - Flags: feedback?(acperez)

Comment 69

4 years ago
Comment on attachment 8373748 [details] [review]
Pull request

The customization part is fine.
Attachment #8373748 - Flags: feedback?(acperez) → feedback+
Comment on attachment 8373748 [details] [review]
Pull request

forgot changing review flag :-/
Attachment #8373748 - Flags: review?(yurenju.mozilla) → review+
Comment on attachment 8373748 [details] [review]
Pull request

Hi Jim,
I updated the patch and ui looks good. Just that in the manage ringtones list page, it might need a press hint like those in setting list. (The press status will be on top of the divider.)
please check the file attached.
thanks!
Attachment #8373748 - Flags: ui-review?(chuang) → ui-review-
Flags: needinfo?(tshakespeare) → needinfo?(squibblyflabbetydoo)
Created attachment 8429328 [details]
manage_list_press.png

manage ringtones need a press hint when clicking.

Updated

4 years ago
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
(Assignee)

Comment 73

4 years ago
(In reply to chuang from comment #71)
> Comment on attachment 8373748 [details] [review]
> Pull request
> 
> Hi Jim,
> I updated the patch and ui looks good. Just that in the manage ringtones
> list page, it might need a press hint like those in setting list. (The press
> status will be on top of the divider.)
> please check the file attached.
> thanks!

Ok, I've updated this. I also fixed an issue where I misread the UI specs; I was making the "more actions" button smaller than it should be. It's now the right size.
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Updated

4 years ago
Attachment #8373748 - Flags: ui-review- → ui-review?(chuang)
(Assignee)

Updated

4 years ago
Depends on: 1016280

Updated

4 years ago
Attachment #8373748 - Flags: ui-review?(chuang) → ui-review+
Comment on attachment 8373748 [details] [review]
Pull request

Nice job Jim! \w/

I see a couple of bugs still but I'm going to open new issues for them so you can land this.
Attachment #8373748 - Flags: ui-review?(tshakespeare) → ui-review+
(In reply to Jim Porter (:squib) from comment #66)
> Ok, I've gone through all of David's comments and either addressed them,
> explained why I did what I did, or made a note to follow up on an open issue.
> 
> Tif: Here are some questions for you!
> 
> 1) If you go to change the ringtone in Settings -> Sound, the "Done" button
> is currently disabled in my branch until you pick a *different* ringtone.
> Should it still be enabled if we automatically selected one by default? I'm
> not convinced either way (although leaving "Done" disabled is less work).
> 
As Jim and I discussed today, the done button should be disabled until the user changes the selection. In the next release, we'll actually be updating the < with X and this is the behaviour it will have. I'd rather keep a little bit of weirdness than remove the done button only to bring it back a release later.

> 2) David noted that there seem to be a lot of subheadings in Settings ->
> Sound. Is there a way to merge any of them? See here:
> <https://github.com/mozilla-b2g/gaia/pull/16151#discussion_r13023635>.

You know I'm not actually opposed to that idea. Let me discuss with Rob and the new Settings designer Jenny.
Depends on: 1017416
(Assignee)

Comment 76

4 years ago
Landed!
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Flags: in-testsuite+

Updated

4 years ago
Blocks: 982951
Jim, is there a specific reason for using \u000A instead of \n?

I find the strings pretty hard to read with those two Unicode characters in the middle, and these are the only two strings currently using this format.

At least we need some localization comment explaining what those things are.
(Assignee)

Comment 78

4 years ago
Because "\n" results in the literal string "\n", not a newline.
(In reply to Jim Porter (:squib) from comment #78)
> Because "\n" results in the literal string "\n", not a newline.

Have you tried this (I haven't)? Because if you're right we have a problem

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/locales/system.en-US.properties#L488
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/locales/dialer.en-US.properties#L13-L23
(Assignee)

Comment 80

4 years ago
If you can figure out how to get that to work, please post a pull request and I'll be happy to review it. However, it doesn't work in my case, and it's not clear to me what these apps are doing to make it work.
Depends on: 1018322
(In reply to Jim Porter (:squib) from comment #80)
> If you can figure out how to get that to work, please post a pull request
> and I'll be happy to review it. However, it doesn't work in my case, and
> it's not clear to me what these apps are doing to make it work.

OK, I tested and it doesn't work at least in the MMI code case, so we have a problem. Thanks ;-)
list-title-ringtone = Firefox Ringtones

Shouldn't that be either "Firefox OS Ringtones" (better: "{{brandShortName}} Ringtones") or "Built-in Ringtones"?

Comment 84

4 years ago
Hey Jim

one thing i feel with could had been even better. Once you go to manage ringtone -> share -> ringtone -> set as ringtone.

Sound is heard with a little popup.

This sound by default is the one set in the alert (Settings->alert) instead i feel it would had been better to play the same ringtone that the user has set as ringtone.

One confusion else the user might feel is that which ringtone got set? because of the sound heard is something different.
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 85

4 years ago
(In reply to ankit93040 from comment #84)
> one thing i feel with could had been even better. Once you go to manage
> ringtone -> share -> ringtone -> set as ringtone.

The ability to do that is a bug and will be removed. See bug 1015513.
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 86

4 years ago
(In reply to ankit93040 from comment #84)
> This sound by default is the one set in the alert (Settings->alert) instead
> i feel it would had been better to play the same ringtone that the user has
> set as ringtone.

Also, this can't be changed, but when the Notification API supports transient notifications, the sound will probably go away.
Hi there!

I wanted to take a second to respond as well. We know the notification with the sound is not ideal, but unfortunately what we did want to use, status, was not possible due to some limitations to the Building Block that I'm hoping will be addressed in the near future. 

For the release, the only other option was to present a full screen confirmation, which I felt was more disruptive to the user as it forced them to tap "ok".

Hopefully that makes sense - thanks!

(In reply to ankit93040 from comment #84)

> Sound is heard with a little popup.
> 
> This sound by default is the one set in the alert (Settings->alert) instead
> i feel it would had been better to play the same ringtone that the user has
> set as ringtone.
> 
> One confusion else the user might feel is that which ringtone got set?
> because of the sound heard is something different.
No longer depends on: 1016280
(Assignee)

Updated

4 years ago
Depends on: 1028048
Duplicate of this bug: 1024331
(Assignee)

Updated

4 years ago
Depends on: 1063236
Depends on: 1067889
You need to log in before you can comment on or make changes to this bug.