Merge lp:~headbangerkenny/ubuntuone-android-music/branch into lp:ubuntuone-android-music

Proposed by Joe Simpson
Status: Work in progress
Proposed branch: lp:~headbangerkenny/ubuntuone-android-music/branch
Merge into: lp:ubuntuone-android-music
Diff against target: 163 lines (+32/-8)
9 files modified
res/values/styles.xml (+2/-2)
res/xml/appwidget_info.xml (+1/-1)
src/net/sourceforge/subsonic/androidapp/activity/DownloadActivity.java (+8/-2)
src/net/sourceforge/subsonic/androidapp/activity/MainActivity.java (+1/-1)
src/net/sourceforge/subsonic/androidapp/service/CachedMusicService.java (+2/-0)
src/net/sourceforge/subsonic/androidapp/service/DownloadService.java (+2/-0)
src/net/sourceforge/subsonic/androidapp/service/DownloadServiceImpl.java (+2/-1)
src/net/sourceforge/subsonic/androidapp/util/FileUtil.java (+2/-1)
src/net/sourceforge/subsonic/androidapp/util/ShufflePlayBuffer.java (+12/-0)
To merge this branch: bzr merge lp:~headbangerkenny/ubuntuone-android-music/branch
Reviewer Review Type Date Requested Status
Chad Miller (community) Needs Fixing
Review via email: mp+71364@code.launchpad.net

Commit message

Shuffle Songs improvements -- headbangerkenny

Description of the change

Basically improves the UX a little, so that new users get "Buffering" while the app is loading the shuffled playlist and automatically plays after "Shuffle Songs" is selected.

To post a comment you must log in.
Revision history for this message
Chad Miller (cmiller) wrote :

This is good, we definitely need feedback when waiting for the shuffle-queue to download. The shuffle buffer already starts playing, though.

MainActivity starts DownloadActivity, which checks the received intent for whether this is shuffle-play, and if it is, calls setShufflePlayEnabled(), which calls checkDownloads(), which calls checkShufflePlay(), which calls then play(0) IF it is the first time a shuffle-play has been started (id est, if shuffle play hasn't been stopped in the past).

On subsequent shuffle-play taps, should the music player do anything other than return the user to the place they were before? I need convincing it should do more.

I welcome the informational change for the user. With that only that change, I'd approve. Marking "Needs Fixing" to indicate that I so far don't want the other changes.

review: Needs Fixing
530. By Joe Simpson

Removed that play(0); code that was requested. However, I have moved the cache folder from /sdcard/subsonic to /sdcard/Android/data/net.sourceforge.subsonic.u1m/music because Android says we should do that. Also, I noticed on my device that the ShuffleBuffer would randomly be empty, so when that happens on get() I tell it to grab more tracks, which works (I end up waiting up to 15 mins for this to refill on my device otherwise)

Revision history for this message
Joe Simpson (headbangerkenny) wrote :

Okay, I've done that with a few changes for you to look at :)

Revision history for this message
Chad Miller (cmiller) wrote :

I want to add some of these ideas, but I don't want to add all of them. Please don't make a proposed merge a package-deal of disparate ideas, all-or-nothing. Branches are free! :) Put one idea in per branch.

I would probably have created branches

lp:~headbangerkenny/ubuntuone-android-music/shuffle-progress-information

lp:~headbangerkenny/ubuntuone-android-music/cache-in-official-suggested-folder

lp:~headbangerkenny/ubuntuone-android-music/shuffle-grab-more-tracks-on-empty

lp:~headbangerkenny/ubuntuone-android-music/automatically-resume-on-subsequent-shuffleplay

Revision history for this message
Joe Simpson (headbangerkenny) wrote :

Okay dokie, I'm new to launchpad and I'll remember this!
On Aug 22, 2011 2:08 PM, "Chad Miller" <email address hidden> wrote:
> I want to add some of these ideas, but I don't want to add all of them.
Please don't make a proposed merge a package-deal of disparate ideas,
all-or-nothing. Branches are free! :) Put one idea in per branch.
>
> I would probably have created branches
>
> lp:~headbangerkenny/ubuntuone-android-music/shuffle-progress-information
>
>
lp:~headbangerkenny/ubuntuone-android-music/cache-in-official-suggested-folder
>
>
lp:~headbangerkenny/ubuntuone-android-music/shuffle-grab-more-tracks-on-empty
>
>
lp:~headbangerkenny/ubuntuone-android-music/automatically-resume-on-subsequent-shuffleplay
> --
>
https://code.launchpad.net/~headbangerkenny/ubuntuone-android-music/branch/+merge/71364
> You are the owner of lp:~headbangerkenny/ubuntuone-android-music/branch.

Revision history for this message
Chad Miller (cmiller) wrote :

First, cheers on using File()s to create deep path, rather than succumbing to temptation to manually construct a string with slashes in it. I don't mind moving it to a standard location, but I wonder if it should be the standard "Music" directory instead. If you're going to move the location, you should handle old data and clean it up or move it, rather than abandoning it.

I don't think you mean a recurring Task in ShufflePlayBuffer.get() .

review: Needs Fixing
Revision history for this message
Chad Miller (cmiller) wrote :

First, cheers on using File()s to create deep path, rather than succumbing to temptation to manually construct a string with slashes in it. I don't mind moving it to a standard location, but I wonder if it should be the standard "Music" directory instead. If you're going to move the location, you should handle old data and clean it up or move it, rather than abandoning it.

I don't think you mean a recurring Task in ShufflePlayBuffer.get() .

review: Needs Fixing

Unmerged revisions

530. By Joe Simpson

Removed that play(0); code that was requested. However, I have moved the cache folder from /sdcard/subsonic to /sdcard/Android/data/net.sourceforge.subsonic.u1m/music because Android says we should do that. Also, I noticed on my device that the ShuffleBuffer would randomly be empty, so when that happens on get() I tell it to grab more tracks, which works (I end up waiting up to 15 mins for this to refill on my device otherwise)

529. By Joe Simpson

I have modified the Ubuntu One Music App so that when you tell it to Shuffle, it'll say it's buffering while the playlist is sorted out. Also it'll automatically play when pressing "Shuffle Songs" from the main screen.

528. By Chad Miller

Don't make non-ui events into Page Views in analytics.

527. By Chad Miller

When overriding Activities, be sure to call the superclass's same method. Android requires it.

526. By MichaƂ Karnicki

Lower priority of media button receiver, unregister receiver when done.

525. By Chad Miller

Merge bad-index crash and trunk with analytics.

524. By Chad Miller

Don't dispatch automatically. We are going to use the network here, so dispatch only when we have the network active.

523. By Chad Miller

Revert bogus mistaken change to auth-return scheme.

522. By Chad Miller

Make analytics use more flag values, instead of magnitude values where appropriate.

Add analytics for cache miss and cache total-use. (Subtraction equals cache hit.)

Add analytics for free-space magnitude.

521. By Chad Miller

Make manual trackers dispatch events.

Make downloader start tracker correctly.

Add tracking to other Activities.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'res/values/styles.xml'
--- res/values/styles.xml 2011-04-21 21:13:18 +0000
+++ res/values/styles.xml 2011-08-20 12:08:25 +0000
@@ -12,14 +12,14 @@
12 <item name="android:textColor">@color/white</item>12 <item name="android:textColor">@color/white</item>
13 </style>13 </style>
1414
15 <style name="Ubuntu.Title" parent="@android:style/WindowTitle">15 <style name="Ubuntu.Title" parent="@android:style/TextAppearance.WindowTitle">
16 <item name="android:paddingLeft">14sp</item>16 <item name="android:paddingLeft">14sp</item>
17 <item name="android:textColor">@color/white</item>17 <item name="android:textColor">@color/white</item>
18 <item name="android:textStyle">normal</item>18 <item name="android:textStyle">normal</item>
19 <item name="android:textAppearance">@style/TextAppearance.WindowTitle</item>19 <item name="android:textAppearance">@style/TextAppearance.WindowTitle</item>
20 </style>20 </style>
2121
22 <style name="Ubuntu.TitleBackground" parent="@android:style/WindowTitleBackground">22 <style name="Ubuntu.TitleBackground">
23 <item name="android:background">@color/project</item>23 <item name="android:background">@color/project</item>
24 </style>24 </style>
2525
2626
=== modified file 'res/xml/appwidget_info.xml'
--- res/xml/appwidget_info.xml 2010-10-23 17:24:51 +0000
+++ res/xml/appwidget_info.xml 2011-08-20 12:08:25 +0000
@@ -3,4 +3,4 @@
3 a:minWidth="294dip"3 a:minWidth="294dip"
4 a:minHeight="72dip"4 a:minHeight="72dip"
5 a:updatePeriodMillis="0"5 a:updatePeriodMillis="0"
6 a:initialLayout="@layout/appwidget"/>
7\ No newline at end of file6\ No newline at end of file
7 a:initialLayout="@layout/appwidget"/>
88
=== modified file 'src/net/sourceforge/subsonic/androidapp/activity/DownloadActivity.java'
--- src/net/sourceforge/subsonic/androidapp/activity/DownloadActivity.java 2011-07-30 11:51:09 +0000
+++ src/net/sourceforge/subsonic/androidapp/activity/DownloadActivity.java 2011-08-20 12:08:25 +0000
@@ -199,8 +199,14 @@
199 registerForContextMenu(playlistView);199 registerForContextMenu(playlistView);
200200
201 if (getIntent().getBooleanExtra(Constants.INTENT_EXTRA_NAME_SHUFFLE, false) && getDownloadService() != null) {201 if (getIntent().getBooleanExtra(Constants.INTENT_EXTRA_NAME_SHUFFLE, false) && getDownloadService() != null) {
202 warnIfNetworkOrStorageUnavailable();202 Thread t = new Thread("Refresh Random-Songs Cache") {
203 getDownloadService().setShufflePlayEnabled(true);203 @Override
204 public void run() {
205 warnIfNetworkOrStorageUnavailable();
206 getDownloadService().setShufflePlayEnabled(true);
207 }
208 };
209 t.start();
204 }210 }
205 }211 }
206212
207213
=== modified file 'src/net/sourceforge/subsonic/androidapp/activity/MainActivity.java'
--- src/net/sourceforge/subsonic/androidapp/activity/MainActivity.java 2011-08-02 13:52:24 +0000
+++ src/net/sourceforge/subsonic/androidapp/activity/MainActivity.java 2011-08-20 12:08:25 +0000
@@ -108,7 +108,7 @@
108108
109 MergeAdapter adapter = new MergeAdapter();109 MergeAdapter adapter = new MergeAdapter();
110 adapter.addViews(Arrays.asList(serverButton, shuffleButton, settingsButton), true);110 adapter.addViews(Arrays.asList(serverButton, shuffleButton, settingsButton), true);
111 File subsonicDir = new File(Environment.getExternalStorageDirectory(), "subsonic");111 File subsonicDir = new File(new File(new File(Environment.getExternalStorageDirectory(), "Android"), "data"), "net.sourceforge.subsonic.u1m");
112 File debugFlagFile = new File(subsonicDir, "showdebugmenu");112 File debugFlagFile = new File(subsonicDir, "showdebugmenu");
113 if (debugFlagFile.exists()) {113 if (debugFlagFile.exists()) {
114 Log.d(TAG, "Revealing debug settings button.");114 Log.d(TAG, "Revealing debug settings button.");
115115
=== modified file 'src/net/sourceforge/subsonic/androidapp/service/CachedMusicService.java'
--- src/net/sourceforge/subsonic/androidapp/service/CachedMusicService.java 2011-07-21 21:55:28 +0000
+++ src/net/sourceforge/subsonic/androidapp/service/CachedMusicService.java 2011-08-20 12:08:25 +0000
@@ -24,6 +24,7 @@
24import net.sourceforge.subsonic.androidapp.domain.Indexes;24import net.sourceforge.subsonic.androidapp.domain.Indexes;
25import net.sourceforge.subsonic.androidapp.domain.MusicDirectory;25import net.sourceforge.subsonic.androidapp.domain.MusicDirectory;
26import net.sourceforge.subsonic.androidapp.domain.Playlist;26import net.sourceforge.subsonic.androidapp.domain.Playlist;
27import net.sourceforge.subsonic.androidapp.domain.PlayerState;
27import net.sourceforge.subsonic.androidapp.domain.Version;28import net.sourceforge.subsonic.androidapp.domain.Version;
28import net.sourceforge.subsonic.androidapp.domain.SearchResult;29import net.sourceforge.subsonic.androidapp.domain.SearchResult;
29import net.sourceforge.subsonic.androidapp.domain.SearchCritera;30import net.sourceforge.subsonic.androidapp.domain.SearchCritera;
@@ -151,6 +152,7 @@
151 Thread t = new Thread("Refresh Random-Songs Cache") {152 Thread t = new Thread("Refresh Random-Songs Cache") {
152 @Override153 @Override
153 public void run() {154 public void run() {
155 DownloadServiceImpl.getInstance().setPlayerState(PlayerState.PREPARING);
154 Log.i(TAG, "Getting new list of random songs in " + (sleepDuration/1000) + " seconds.");156 Log.i(TAG, "Getting new list of random songs in " + (sleepDuration/1000) + " seconds.");
155 if (sleepDuration != 0L) {157 if (sleepDuration != 0L) {
156 Log.i(TAG, "Not requesting new list of songs right away.");158 Log.i(TAG, "Not requesting new list of songs right away.");
157159
=== modified file 'src/net/sourceforge/subsonic/androidapp/service/DownloadService.java'
--- src/net/sourceforge/subsonic/androidapp/service/DownloadService.java 2010-12-01 22:37:01 +0000
+++ src/net/sourceforge/subsonic/androidapp/service/DownloadService.java 2011-08-20 12:08:25 +0000
@@ -67,6 +67,8 @@
6767
68 void reset();68 void reset();
6969
70 public void setPlayerState(PlayerState playerState);
71
70 PlayerState getPlayerState();72 PlayerState getPlayerState();
7173
72 int getPlayerPosition();74 int getPlayerPosition();
7375
=== modified file 'src/net/sourceforge/subsonic/androidapp/service/DownloadServiceImpl.java'
--- src/net/sourceforge/subsonic/androidapp/service/DownloadServiceImpl.java 2011-08-02 13:52:24 +0000
+++ src/net/sourceforge/subsonic/androidapp/service/DownloadServiceImpl.java 2011-08-20 12:08:25 +0000
@@ -450,7 +450,8 @@
450 return playerState;450 return playerState;
451 }451 }
452452
453 private synchronized void setPlayerState(PlayerState playerState) {453 @Override
454 public synchronized void setPlayerState(PlayerState playerState) {
454 Log.i(TAG, this.playerState.name() + " -> " + playerState.name() + " (" + currentPlaying + ")");455 Log.i(TAG, this.playerState.name() + " -> " + playerState.name() + " (" + currentPlaying + ")");
455456
456 boolean show = this.playerState == PAUSED && playerState == PlayerState.STARTED;457 boolean show = this.playerState == PAUSED && playerState == PlayerState.STARTED;
457458
=== modified file 'src/net/sourceforge/subsonic/androidapp/util/FileUtil.java'
--- src/net/sourceforge/subsonic/androidapp/util/FileUtil.java 2011-07-05 23:16:25 +0000
+++ src/net/sourceforge/subsonic/androidapp/util/FileUtil.java 2011-08-20 12:08:25 +0000
@@ -153,7 +153,8 @@
153 }153 }
154154
155 private static File createDirectory(String name) {155 private static File createDirectory(String name) {
156 File subsonicDir = new File(Environment.getExternalStorageDirectory(), "subsonic");156 // http://developer.android.com/guide/topics/data/data-storage.html#filesExternal states we should use this directory instead
157 File subsonicDir = new File(new File(new File(Environment.getExternalStorageDirectory(), "Android"), "data"), "net.sourceforge.subsonic.u1m");
157 File dir = new File(subsonicDir, name);158 File dir = new File(subsonicDir, name);
158 if (!dir.exists() && !dir.mkdirs()) {159 if (!dir.exists() && !dir.mkdirs()) {
159 Log.e(TAG, "Failed to create " + name);160 Log.e(TAG, "Failed to create " + name);
160161
=== modified file 'src/net/sourceforge/subsonic/androidapp/util/ShufflePlayBuffer.java'
--- src/net/sourceforge/subsonic/androidapp/util/ShufflePlayBuffer.java 2011-07-22 19:01:22 +0000
+++ src/net/sourceforge/subsonic/androidapp/util/ShufflePlayBuffer.java 2011-08-20 12:08:25 +0000
@@ -29,6 +29,8 @@
29import net.sourceforge.subsonic.androidapp.domain.MusicDirectory;29import net.sourceforge.subsonic.androidapp.domain.MusicDirectory;
30import net.sourceforge.subsonic.androidapp.service.MusicService;30import net.sourceforge.subsonic.androidapp.service.MusicService;
31import net.sourceforge.subsonic.androidapp.service.MusicServiceFactory;31import net.sourceforge.subsonic.androidapp.service.MusicServiceFactory;
32import net.sourceforge.subsonic.androidapp.service.DownloadServiceImpl;
33import net.sourceforge.subsonic.androidapp.domain.PlayerState;
3234
33/**35/**
34 * @author Sindre Mehus36 * @author Sindre Mehus
@@ -69,6 +71,7 @@
69 }71 }
7072
71 public List<MusicDirectory.Entry> get(int size) {73 public List<MusicDirectory.Entry> get(int size) {
74 DownloadServiceImpl.getInstance().setPlayerState(PlayerState.PREPARING);
72 clearBufferIfnecessary();75 clearBufferIfnecessary();
7376
74 List<MusicDirectory.Entry> result = new ArrayList<MusicDirectory.Entry>(size);77 List<MusicDirectory.Entry> result = new ArrayList<MusicDirectory.Entry>(size);
@@ -77,6 +80,15 @@
77 result.add(buffer.remove(buffer.size() - 1));80 result.add(buffer.remove(buffer.size() - 1));
78 }81 }
79 }82 }
83 if(buffer.size() == 0 && result.size() == 0) { // my phone likes to be empty.... let's fill it up
84 Runnable runnable = new Runnable() {
85 @Override
86 public void run() {
87 refill();
88 }
89 };
90 executorService.scheduleWithFixedDelay(runnable, 1, 3, TimeUnit.SECONDS);
91 }
80 Log.i(TAG, "Taking " + result.size() + " songs from shuffle-play buffer. " + buffer.size() + " remaining.");92 Log.i(TAG, "Taking " + result.size() + " songs from shuffle-play buffer. " + buffer.size() + " remaining.");
81 return result;93 return result;
82 }94 }

Subscribers

People subscribed via source and target branches

to status/vote changes: