Bug 824117 - [music] use a single player to open the received file #7234

Merged
merged 1 commit into from Dec 30, 2012

Conversation

Projects
None yet
2 participants
Contributor

dominickuo commented Dec 28, 2012

To open the received file via bluetooth transferring, this PR separate the PlayerView from music.js and create three new files called Player.js, open.js and open.html, so that Music app now can:

  1. Use PlayerView without initialising a music MediaDB.
  2. PlayerView can play records from MediaDB, also files(blob).
Contributor

dominickuo commented Dec 28, 2012

@davidflanagan Can you please review this? thanks.

apps/music/js/Player.js
+
+ stop: function pv_stop() {
+ this.pause();
+ this.audio.src = null;
@davidflanagan

davidflanagan Dec 29, 2012

For videos, I've found that setting src to null causes a warning in the console.
To avoid that, you can do this:

this.audio.removeAttribute('src');
this.audio.load();

This is spec-compliant and causes gecko to release all media resources.

Not required for this bug, but something you should know.

@dominickuo

dominickuo Dec 30, 2012

Contributor

Good to know this! so I changed to this way when the player need to release the sources.

apps/music/js/Player.js
+ next: function pv_next(isAutomatic) {
+ if (this.sourceType === TYPE_BLOB) {
+ // When the player ends, repeat it again the dataSource is a blob
+ this.setAudioSrc(this.dataSource);
@davidflanagan

davidflanagan Dec 29, 2012

For the video view activity I've just implemented, I just call postResult() when the video ends so the inline activity ends and we return to the calling application. Would that be better here? If the user wants to listen again, they can always launch the music app.

apps/music/js/Player.js
+ for (var i = 0; i < buttons.length; i++) {
+ buttons[i].disabled = (buttons[i] != this.playControl);
+ }
+
@davidflanagan

davidflanagan Dec 29, 2012

If you are going to auto-repeat this song, call setRepeat() here so the UI indicates that it is in repeat mode. (If you can do that without altering the user's saved settings.)

@dominickuo

dominickuo Dec 30, 2012

Contributor

Actually the saved repeat modes doesn't apply to the single player, so I just used the static style "repeat-list" in HTML and set it to disabled. And I don't have to use the js code to disabled the buttons.

apps/music/js/Player.js
+
+ this.setAudioSrc(this.dataSource);
+ }.bind(this));
+ return;
@davidflanagan

davidflanagan Dec 29, 2012

I'm confused that this else clause separates the code in the if() clause from the call to this.audio.play(). Could you move the this.audio.play() into the if clause above? Or move this else clause into an if clause of its own at the top of this function?

@dominickuo

dominickuo Dec 30, 2012

Contributor

Looks like these if and else clauses confuses you... so I have changed to if > else if > else, I believe this is more clear!

+ // target is the seek bar, and evt.layerX is the moved position
+ var seekTime = x / target.clientWidth * this.seekBar.max;
+ this.setSeekBar(this.audio.startTime, this.audio.duration, seekTime);
+ }
@davidflanagan

davidflanagan Dec 29, 2012

In reviewing Dale's video player, I've noticed that he pauses the video while seeking and then resumes playing on mouseup. I've noticed that you have a comment in here about too much seeking causing problems. Maybe pausing here would help.

In bug 823619, the way mouse events are delivered may change substantially. I've written a shim for apps like yours that need to get mousemove events like yours does. It doesn't work for the Music app. And now that I see this code, I know why: my shim generates synthetic mouse events from touch events, but the synthetic events don't have the (non-standard) layerX property. You should use clientX or pageX I think (but I never remember which is right).

Also, I don't know whether the shim will work with setCapture() or not.

It is a separate bug, but if the mouse event change in 823619 goes ahead, this code will have to change

@dominickuo

dominickuo Dec 30, 2012

Contributor

I used layerX is because I just want to get the x position of my target elements. For clientX or pageX, I have to do some calculation like clientX - style.left, I am afraid that it might get wrong result if one of the value of clientX and style.left is not correct.

I also found that Music app didn't apply the shim for apps but still got the correct result, so maybe after bug 823619 is resolved, let's see if we have to change the seeking of Music.

@dominickuo

dominickuo Dec 30, 2012

Contributor

I forget to mention that I have changed the seeking behaviour in last patch.
Now the music player only seek audio (set currentTime) when the user's finger is away from the panel, that means the seek indicator will follow the finger but the audio will be seek until mouseup fires. This is something like the video player did, but without pausing the media content, and users can still listen to the songs when they are seeking to their favourite part.

apps/music/js/open.js
+
+window.onload = function() {
+ navigator.mozSetMessageHandler('activity', handleOpenActivity);
+};
@davidflanagan

davidflanagan Dec 29, 2012

handleOpenActivity() will probably be invoked before the localized event fires. Maybe you can just call mozSetMessageHandler from the onlocalized function above... That will slow your startup time a little bit, but you'll be sure that your unknown strings are defined.

@dominickuo

dominickuo Dec 30, 2012

Contributor

Yes, what you say is correct! I have changed it, thanks.

+ // it will pass the file to music player via web activity
+ // but we still got a blob which cannot be accepted by audio element
+ // so we use the received filename to get the file again from deviceStorage
+ var storage = navigator.getDeviceStorage('music');
@davidflanagan

davidflanagan Dec 29, 2012

Have you tried again to directly use the blob that was sent? Bug 812098 is waiting on an answer from you about whether the bug has been resolved or not.

@dominickuo

dominickuo Dec 30, 2012

Contributor

I have tested again and found, Bug 812098 is resolved, but the file which passed by web activity still become a blob, cause I tried to see if file.name exists of not, still no name for the file. And the file caused an error for metadata parser so I have to use this workaround first until I found what's wrong with the passed file.

+ PlayerView.init();
+ PlayerView.setSourceType(TYPE_BLOB);
+ PlayerView.dataSource = blob;
+ PlayerView.play(); // Do we need to play for users?
@davidflanagan

davidflanagan Dec 29, 2012

Hmm. I think that for blobs, PlayerView.play() doesn't actually start playing the song. Its a UX decision, but I would have thought that the user's tap to open the file should be enough to know that they want to hear the song, and that you should start playing automatically.

+var unknownAlbum;
+var unknownArtist;
+var unknownTitle;
+
@davidflanagan

davidflanagan Dec 29, 2012

What happens if this activity is invoked while the music app is already running and playing a song? I don't know anything about the new audio API and how it handles contention. Do you have to do anything here to ensure that this song overrides any music that is already playing?

@dominickuo

dominickuo Dec 30, 2012

Contributor

Here is the link of the new AudioChannels API
https://wiki.mozilla.org/WebAPI/AudioChannels

And it says: "If two apps try to use the "content" channel at the same time, the foreground app wins. If both of the apps are background apps, then the last app to try to use the channel wins."

So I think the mechanism of AudioChannels should handle the condition you said correctly, or that should be a bug.

+
+ function done() {
+ PlayerView.stop();
+ request.postResult({});
@davidflanagan

davidflanagan Dec 29, 2012

If music was already playing, when this activity started, do you have to do anything to restore it now?

apps/music/open.html
+ <button class="rating-star" data-rating="4"></button>
+ <button class="rating-star" data-rating="5"></button>
+ </div>
+ <button id="player-album-shuffle" class="player-cover-button"></button>
@davidflanagan

davidflanagan Dec 29, 2012

Should you just remove the ratings and shuffle buttons?

Maybe remove the repeat button too, unless you are going to use it to indicate to the user that they are in perpetual repeat mode.

@dominickuo

dominickuo Dec 30, 2012

Contributor

Actually I don't know what is the correct UI for opening a bluetooth-transferred audio file, let's keep the current UI which normal Music player is using, and I will ask UX or visual designers to define this.

apps/music/open.html
+ <button id="player-controls-previous" class="player-controls-button"></button>
+ <button id="player-controls-play" class="player-controls-button is-pause"></button>
+ <button id="player-controls-next" class="player-controls-button"></button>
+ </div>
@davidflanagan

davidflanagan Dec 29, 2012

Would it be better to just remove the next button instead of disabling it? Or if that causes problems in the code, maybe you could hide it?

@davidflanagan

davidflanagan Dec 29, 2012

Or, if you keep all of these controls, I think it would be better to disable them here in the HTML rather than having special code to disable them in Player.js

@dominickuo

dominickuo Dec 30, 2012

Contributor

I like your idea to disable the buttons in the HTML, so I did it!

Overall, the code looks good. Most of my comments are suggestions, not requirements. The only one I think that really needs to be fixed before landing is to register the activity handler from the onlocalized callback rather than the onload callback. If playing the received blob really still doesn't work, please update the bugzilla bug with that information. Otherwise remove the device storage code. r+ if you fix those things.

In testing the code, I forgot that I have to do a make reset-gaia to get new activity handlers recognized, so I was transferring music and then finding that it would not play. But that wasn't your bug, and once I figured it out, it works well.

In testing it, I see that you're not auto-repeating songs when they finish, just going back to the beginning so the user has the choice to play again or cancel the activity. That seems like good UX, so you can ignore my comments about starting to play automatically and ending the activity when the song ends.

I notice that my .m4a music files don't play anymore. I think they used to work. All the metadata is parsed, but when I try to play them it just skips from the start to the end. Maybe a gecko issue. But it happens even without this patch applied, so it is unrelated to this bug.

Contributor

dominickuo commented Dec 30, 2012

@davidflanagan thanks for the reviewing.

I have changed mostly all the parts that you mentioned in the comments, especially to register the activity handler after the localized event fired. But I found there seems still some bug of the file which passed by web activity, so I kept the workaround of using deviceStorage to get the file again.

Since this patch got a r+ and it's bb+, I am merging this.

dominickuo added a commit that referenced this pull request Dec 30, 2012

Merge pull request #7234 from dominickuo/music-open-received-file
Bug 824117 - [music] use a single player to open the received file, r=djf, a=bb+

@dominickuo dominickuo merged commit ac6440d into mozilla-b2g:master Dec 30, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment