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

Status: RESOLVED FIXED

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

Comment 1

[Jim Porter (:squib)]

Taking.

Comment 2

[Jim Porter (:squib)]

Attachment: 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.

Comment 3

[Jim Porter (:squib)]

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!

Comment 4

[Jim Porter (:squib)]

Oh, and the relevant file is apps/ringtones/js/share.js.

Comment 5

[David Flanagan [:djf]]

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 6

[David Flanagan [:djf]]

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.

Comment 7

[Jim Porter (:squib)]

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.)

Comment 8

[Jim Porter (:squib)]

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.)

Comment 9

[Sri Kasetti]

Adding Rob since Jacqueline is out this week

Comment 11

[Rob MacDonald [:robmac]]

Hi there - I'm expecting Jacqueline back tomorrow so I'll defer this until then if that's ok. - Rob

Comment 12

[Jacqueline Savory [:jsavory] UX]

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?

Comment 13

[Jim Porter (:squib)]

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.

Comment 14

[ankit93040]

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

Comment 15

[Jim Porter (:squib)]

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.

Comment 16

[Jim Porter (:squib)]

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.

Comment 17

[Jim Porter (:squib)]

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.

Comment 18

[Carol Huang [:Carol]]

Attachment: 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.

Comment 19

[Jim Porter (:squib)]

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).

Comment 20

[Jim Porter (:squib)]

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.

Comment 21

[Jim Porter (:squib)]

Comment on attachment 8373748 [details] [review]
Pull request

Switching to a full review, since this is basically done.

Comment 22

[Jim Porter (:squib)]

Tests are red for the PR right now because of bug 1009368.

Comment 23

[Tiffanie Shakespeare [:tif]]

FYI - UX spec can be found here: https://github.com/mozilla-b2g/gaia-specs/blob/master/%5B1.5%5D%20Ringtones.pdf

Comment 24

[Tiffanie Shakespeare [:tif]]

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!

Comment 25

[Jim Porter (:squib)]

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

Comment 26

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8373748 [details] [review]
Pull request

Adding Carol to take a look from the visual side.

Comment 27

[Tiffanie Shakespeare [:tif]]

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!

Comment 28

[Jim Porter (:squib)]

(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...

Comment 29

[Jim Porter (:squib)]

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!

Comment 30

[David Flanagan [:djf]]

In the PR, it looks like apps/ringtones/style/images/[e-mail] is a busted image.

Comment 31

[David Flanagan [:djf]]

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.

Comment 32

[Jim Porter (:squib)]

(In reply to David Flanagan [:djf] from comment #30)
> In the PR, it looks like
> apps/ringtones/style/images/[e-mail] is a busted image.

Carol: could you upload a fixed version of this image, please?

Comment 33

[Jim Porter (:squib)]

(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.

Comment 34

[Jim Porter (:squib)]

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.

Comment 35

[Tiffanie Shakespeare [:tif]]

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

Comment 36

[Jim Porter (:squib)]

(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?

Comment 37

[Jim Porter (:squib)]

(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

[Dominic Kuo [:dkuo]]

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.

Comment 39

[David Flanagan [:djf]]

(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.)

Comment 40

[Tiffanie Shakespeare [:tif]]

(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! :)

Comment 41

[Tiffanie Shakespeare [:tif]]

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.

Comment 42

[Tiffanie Shakespeare [:tif]]

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.)

Comment 43

[Tiffanie Shakespeare [:tif]]

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!

Comment 44

[Jim Porter (:squib)]

(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...

Comment 45

[Jim Porter (:squib)]

Ok, I think I've addressed all the code review comments.

Comment 46

[Jim Porter (:squib)]

(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.

Comment 47

[Carol Huang [:Carol]]

Attachment: Ringtons 2.0_Visual Spec.zip

Hi Jim,
I updated the image for "[e-mail]".
Please check the attachment again and let me know if you have any other question,thanks!!!

Comment 48

[Carol Huang [:Carol]]

Attachment: 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!

Comment 49

[Jim Porter (:squib)]

(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.

Comment 50

[David Flanagan [:djf]]

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.

Comment 51

[Francesco Lodolo [:flod]]

(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.

Comment 52

[David Flanagan [:djf]]

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!)

Comment 53

[David Flanagan [:djf]]

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.

Comment 54

[Jim Porter (:squib)]

(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 :)

Comment 55

[Tiffanie Shakespeare [:tif]]

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!!

Comment 56

[Tiffanie Shakespeare [:tif]]

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.)

Comment 57

[Jim Porter (:squib)]

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!

Comment 58

[Jim Porter (:squib)]

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.

Comment 59

[David Flanagan [:djf]]

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 60

[David Flanagan [:djf]]

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.

Comment 61

[Jim Porter (:squib)]

(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.

Comment 62

[Jim Porter (:squib)]

(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!

Comment 63

[Jim Porter (:squib)]

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

[ankit93040]

Hi Jim

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

thanks!

Comment 65

[Jim Porter (:squib)]

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...)

Comment 66

[Jim Porter (:squib)]

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>.

Comment 67

[Jim Porter (:squib)]

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 68

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8373748 [details] [review]
Pull request

looks nice, r=yurenju

also feedback? Albert since he implemented customzatation per sim card.

Comment 69

[Albert [:albert]]

Comment on attachment 8373748 [details] [review]
Pull request

The customization part is fine.

Comment 70

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8373748 [details] [review]
Pull request

forgot changing review flag :-/

Comment 71

[Carol Huang [:Carol]]

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!

Comment 72

[Carol Huang [:Carol]]

Attachment: manage_list_press.png

manage ringtones need a press hint when clicking.

Comment 73

[Jim Porter (:squib)]

(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.

Comment 74

[Tiffanie Shakespeare [:tif]]

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.

Comment 75

[Tiffanie Shakespeare [:tif]]

(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.

Comment 76

[Jim Porter (:squib)]

Landed!

Comment 77

[Francesco Lodolo [:flod]]

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.

Comment 78

[Jim Porter (:squib)]

Because "\n" results in the literal string "\n", not a newline.

Comment 79

[Francesco Lodolo [:flod]]

(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

Comment 80

[Jim Porter (:squib)]

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.

Comment 81

[Francesco Lodolo [:flod]]

(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 ;-)

Comment 82

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

list-title-ringtone = Firefox Ringtones

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

Comment 83

[ankit93040]

Follow up bug 1018804. https://bugzilla.mozilla.org/show_bug.cgi?id=1018804

you may also see comment#65.

Comment 84

[ankit93040]

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.

Comment 85

[Jim Porter (:squib)]

(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.

Comment 86

[Jim Porter (:squib)]

(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.

Comment 87

[Tiffanie Shakespeare [:tif]]

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.