nextcloud / android

📱 Nextcloud Android app
https://play.google.com/store/apps/details?id=com.nextcloud.client
GNU General Public License v2.0
4.32k stars 1.78k forks source link

Tapping music (media) player notification should open the player #6782

Open pilsnerbeer opened 4 years ago

pilsnerbeer commented 4 years ago

Note: Items 1 & 2 are addressed; leaving open to track enhancement idea in item 3 and updated issue title to reflect that idea

PROBLEM 1 - music controls not working ### ~~Steps to reproduce~~ 1. ~~Open music file~~ 2. ~~try to pause it~~ ~~Screenshot: music controls not responsive.~~ ![Screenshot_3](https://user-images.githubusercontent.com/36133540/90974808-ce436100-e52e-11ea-8dee-bcf5bd7b9d33.png)
PROBLEM 2 - exiting & re-opening music player renders weird layout ### ~~Steps to reproduce~~ ~~1. Open music file~~ ~~2. hit HOME to minimize it~~ ~~3. open it again from multitasking view~~ ~~A non-responsive layout is rendered with no music controls, FAB and some elements from main view:~~ ![Screenshot_4](https://user-images.githubusercontent.com/36133540/90974827-0e0a4880-e52f-11ea-84f7-1182f67955eb.png)

PROBLEM 3 - tapping music player notification produces no response

Steps to reproduce

  1. play music
  2. hit HOME to return to homescreen
  3. tap nextcloud music player notification

Nothing happens. I believe the player should open.


Android version: 10

Device model: OP5T

Nextcloud app version: 3.13.0

Nextcloud server version: 19

ezaquarii commented 4 years ago

@tobiasKaminsky PR #6799 provides only partial fix for first reported problem.

Second problem - controls messed up after screen switch - is much more sinister.

Preview works by adding PreviewMediaFragment to the stack, on top of OCFileListFragment. The problem is that both fragments are attached to the activity and both depend on "global" activity state with different expectations.

When screen is updated (resumed from background or rotated), FileDisplayActivity.onResume() retrieves OCFileListFragment and calls OCFileListFragment.listDirectory() that reconfigures fab_main button and sort_list_button_group via OCFileListFragment.updateLayout().

This happens when PreviewMediaFragment is on top of the fragment backstack.

Issue could be caused by some subtle changes of shared state modification in commit 38c28332e266fa3f4324868b36963fc0d3176700. I'm not sure if this is where the problem was introduced, as commit history suggests that the FAB manipulation was there before as well.

I see 2 possible fixes for this problem: 1) remove the FAB modification code from OCFileListFragment - it does not own it and should not touch a shared resource; move the code to the owning activity that will coordinate updates depending on displayed content

2) launch media preview as separate activity that does not attach OCFileListFragment

Since I'm having very limited time recently, I cannot immediately take care of it atm. :(

I hope the bug analysis will be helpful in resolving it.

tobiasKaminsky commented 4 years ago

Thanks for the detailed analysis! As with #6769 we can remove the split fragment stuff in FileDisplayActivity. Then FAB can be moved back to OCFileListFragment, hopefully, as it is only used there. Also PreviewMediaFragment will then replace OCFileListFragment, when starting playback, eliminating above problem. We just need to make sure that after going back from PreviewMediaFragment the correct folder is displayed in OCFileListFragment, but this should be easy.

What do you think?

ezaquarii commented 4 years ago

@tobiasKaminsky I think it is a good idea. FAB should be owned by the list fragment.

We can rely on backstack history for back navigation. If there is no other convoluted, shared state between activity and fragment, it should just work.

aqaqsubin commented 4 years ago

I still have this issue with a Samsung Galaxy s10e running Android 10 with Nextcloud app 3.14.0 RC2. I followed the reproduce steps, and FAB appeared in the preview player layout.

https://drive.google.com/file/d/1asJioZ1cBs4jj24PTZzYpF0bMx-n8zzV/view?usp=drivesdk

AndyScherzinger commented 4 years ago

@aqaqsubin can you please open an issue for this? (I could see it too now)

aqaqsubin commented 4 years ago

I reported this in #7356.

joshtrichards commented 1 year ago

The Media player was replaced in #8132 and released in v3.17.0. Item 1 & 2 do not appear to exist any longer. Item 3 is a good enhancement idea.