Internal music player does not stop playing when file is deleted #1292

rishabh7m · 2015-11-18T21:01:45Z

Bug fix for issue #906

edited by @jabarros

Create 'internal_music_player' branch from master in android repository @jabarros
Merge 'internal_music_player' branch into master
Download rishabh7m:master branch changes
CR changes @jabarros

przybylski · 2015-11-18T23:02:38Z

Hmm there are two scenarios which might be problematic here.

having two audio files A and B, play file A and delete file B remotely and locally, won't that cause to stop playing of file A ?
If external app plays sounds and you delete a file remotely and locally won't that cause to stop playing current sound?

przybylski · 2015-11-18T23:05:20Z

Ok, second scenario is invalid because external apps cannot use owncloud MediaService but the first one stays valid.

rishabh7m · 2015-11-19T10:21:19Z

@przybylski Hi, I have fixed the 1st issue with this commit.

przybylski · 2015-11-19T10:23:41Z

src/com/owncloud/android/media/MediaService.java

+            }
+        }
+        catch (NullPointerException e) {
+            e.printStackTrace();


I think instead of printing stack you should write to log @davivel @tobiasKaminsky any thoughts?

Also http://www.javapractices.com/topic/TopicAction.do?Id=129

I second that, should definitely be a log.Call 👍

rishabh7m · 2015-11-21T08:40:59Z

@przybylski @AndyScherzinger I have added log statement in latest commit.

przybylski · 2015-11-21T22:50:09Z

src/com/owncloud/android/media/MediaService.java

+            }
+        }
+        catch (NullPointerException e) {
+            Log_OC.e(TAG, "NullPointerException playing " + remotePath, e);


Isn't it more of as "NullPointerException while stopping" ?

Yup, you are right. I will change that 👍

przybylski · 2015-11-21T22:52:30Z

Why do you need Remote path anyway? It can be get from OCFile. Beside if someone wouldn't provide OCFile as a parcelable it is highly likely that remote path also won't be included. IMO this additional argument is redundant.

rishabh7m · 2015-11-22T06:37:44Z

I think if OCFile will be null then OCFile.getRemotePath() will again throw NullPointerException. That's why I included it.

przybylski · 2015-11-22T08:53:21Z

Yeah, null OCFile won't even be included because in onConfirmation you are doing the check for null, It is a good practice that you are catching null ptr exception, but if REMOTE_PATH is just there for logging purposes then I don't find it very useful.

rishabh7m · 2015-11-22T18:06:25Z

Will log statement - Log_OC.e(TAG, "NullPointerException while stopping file "); will be fine ?

przybylski · 2015-11-22T20:36:09Z

NullPointerException while stopping on OCFile is more descriptive :)

rishabh7m · 2015-11-22T20:37:16Z

Okay, I will fix that :)

przybylski · 2015-11-22T20:45:41Z

Great! :) but now REMOTE_PATH is completely redundant :)

rishabh7m · 2015-11-22T20:47:40Z

Sorry, forgot about that. Removing it.

przybylski · 2015-11-22T20:55:31Z

Code looks good to me.
Please squash your commits and you have green light from me :)

bug_906_fix_1 Revert "bug_906_fix_1" This reverts commit 4e23ce3. Revert "bug_906_fix_1" This reverts commit 4e23ce3. revert_906_fix_1 bug_906_fix_log bug_906_log_fix bug_906_log_1

przybylski · 2015-11-22T21:50:39Z

@tobiasKaminsky IMO this is ready for merge, I belive it should first go to beta right ?

przybylski · 2015-11-22T21:53:15Z

@rishabh7m Thank you for your contribution and the will to cooperate with annoying reviewers :)

przybylski · 2015-11-22T21:55:31Z

@rishabh7m One more thing, could you maybe provide some meaningful commit message :)

AndyScherzinger · 2015-11-23T08:14:51Z

Hi @rishabh7m thanks for contributing and great to have you here. One question since you don't have a "collaborator" tag on your postings. Did you already sign the contributor agreement https://owncloud.org/contribute/agreement/ and send it to @karlitschek and @masensio ? Is this a necessary in order to get your pull requests merged to the code base by the core dev team.
And again, thanks for your contribution! Always great to have new people around pushing the app forward!

rishabh7m · 2015-11-23T09:55:51Z

Hi @AndyScherzinger , I have not signed the agreement earlier. There is an employer field in the agreement but I am a student, so will it be fine to leave that empty?

rishabh7m · 2015-11-23T09:56:45Z

@przybylski lol, no one was annoying :)

AndyScherzinger · 2015-11-23T10:17:58Z

@rishabh7m yeah, just leave it empty then, should be fine 👍

malkomich · 2016-06-03T10:35:19Z

src/com/owncloud/android/ui/dialog/RemoveFileDialogFragment.java

+            Intent i = new Intent(getActivity(), MediaService.class);
+            i.putExtra(MediaService.EXTRA_FILE, ocFile);
+            i.setAction(MediaService.ACTION_STOP_FILE);
+            getActivity().startService(i);


It would be great if you set this behaviour also for a local remove, since it makes sense that if you want to remove the file locally, you don't want to keep it in the buffer of the media service.

P.D: The local remove listener is currently set in the negative button (which listener mehotd is onCancel). Extract to a new method to reuse the MediaService call.

davivel · 2016-07-14T07:45:06Z

Hi, @rishabh7m. Sorry for joining so late, and thanks for your patience.

This PR seems really fine, thank you very much.

I think @malkomich is right, makes sense stopping the reproduction also when the deletion is only local (method onCancel in RemoveFileDialogFragment. Could you create a private method with the code stoping the media service and call when any of the deletions is confirmed?

I will help you then with the rebase on master.

jabarros · 2016-09-22T08:57:00Z

Hi everybody,
We have just opened a new PR, #1814 in order to be able to do the rebase and apply the CR changes proposed, so let me close this one.
If you want to do any comment, please, add it into the new PR thread.

przybylski reviewed Nov 19, 2015
View reviewed changes

przybylski reviewed Nov 21, 2015
View reviewed changes

tobiasKaminsky added the 2 - Developing label Nov 22, 2015

bug_906_fix

033e297

bug_906_fix_1 Revert "bug_906_fix_1" This reverts commit 4e23ce3. Revert "bug_906_fix_1" This reverts commit 4e23ce3. revert_906_fix_1 bug_906_fix_log bug_906_log_fix bug_906_log_1

rishabh7m force-pushed the master branch from f5956de to f67f320 Compare November 22, 2015 21:40

rishabh7m changed the title ~~bug_906_fix~~ Internal music player does not stop playing when file is deleted Nov 23, 2015

tobiasKaminsky added 3 - To review and removed 2 - Developing labels Nov 23, 2015

rishabh7m force-pushed the master branch 2 times, most recently from 5562783 to 033e297 Compare November 28, 2015 15:53

LukeOwlclaw mentioned this pull request Mar 18, 2016

Top Open PRs #1454

Closed

malkomich reviewed Jun 3, 2016
View reviewed changes

davivel added this to the backlog milestone Jul 14, 2016

davivel modified the milestones: 2.2.0, backlog Jul 14, 2016

davivel removed the 3 - To review label Jul 14, 2016

jabarros mentioned this pull request Sep 22, 2016

Internal music player does not stop playing when file is deleted #1814

Merged

8 tasks

jabarros closed this Sep 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internal music player does not stop playing when file is deleted #1292

Internal music player does not stop playing when file is deleted #1292

rishabh7m commented Nov 18, 2015 •

edited by jabarros

Loading

przybylski commented Nov 18, 2015

przybylski commented Nov 18, 2015

rishabh7m commented Nov 19, 2015

przybylski Nov 19, 2015

przybylski Nov 19, 2015

AndyScherzinger Nov 19, 2015

rishabh7m commented Nov 21, 2015

przybylski Nov 21, 2015

rishabh7m Nov 22, 2015

przybylski commented Nov 21, 2015

rishabh7m commented Nov 22, 2015

przybylski commented Nov 22, 2015

rishabh7m commented Nov 22, 2015

przybylski commented Nov 22, 2015

rishabh7m commented Nov 22, 2015

przybylski commented Nov 22, 2015

rishabh7m commented Nov 22, 2015

przybylski commented Nov 22, 2015

przybylski commented Nov 22, 2015

przybylski commented Nov 22, 2015

przybylski commented Nov 22, 2015

AndyScherzinger commented Nov 23, 2015

rishabh7m commented Nov 23, 2015

rishabh7m commented Nov 23, 2015

AndyScherzinger commented Nov 23, 2015

malkomich Jun 3, 2016

davivel commented Jul 14, 2016

jabarros commented Sep 22, 2016

Internal music player does not stop playing when file is deleted #1292

Internal music player does not stop playing when file is deleted #1292

Conversation

rishabh7m commented Nov 18, 2015 • edited by jabarros Loading

przybylski commented Nov 18, 2015

przybylski commented Nov 18, 2015

rishabh7m commented Nov 19, 2015

przybylski Nov 19, 2015

Choose a reason for hiding this comment

przybylski Nov 19, 2015

Choose a reason for hiding this comment

AndyScherzinger Nov 19, 2015

Choose a reason for hiding this comment

rishabh7m commented Nov 21, 2015

przybylski Nov 21, 2015

Choose a reason for hiding this comment

rishabh7m Nov 22, 2015

Choose a reason for hiding this comment

przybylski commented Nov 21, 2015

rishabh7m commented Nov 22, 2015

przybylski commented Nov 22, 2015

rishabh7m commented Nov 22, 2015

przybylski commented Nov 22, 2015

rishabh7m commented Nov 22, 2015

przybylski commented Nov 22, 2015

rishabh7m commented Nov 22, 2015

przybylski commented Nov 22, 2015

przybylski commented Nov 22, 2015

przybylski commented Nov 22, 2015

przybylski commented Nov 22, 2015

AndyScherzinger commented Nov 23, 2015

rishabh7m commented Nov 23, 2015

rishabh7m commented Nov 23, 2015

AndyScherzinger commented Nov 23, 2015

malkomich Jun 3, 2016

Choose a reason for hiding this comment

davivel commented Jul 14, 2016

jabarros commented Sep 22, 2016

rishabh7m commented Nov 18, 2015 •

edited by jabarros

Loading