Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues with Media Tab & performance improvement. #11321

Merged
merged 2 commits into from
Apr 12, 2023

Conversation

digitaltechconsult
Copy link
Contributor

Signed-off-by: Bogdan Coticopol bcoticopol@gmail.com
References:
#10882 Slow Media Tab View
#11281

For more info, please check the discussion at the above references.
CC: @AlvaroBrey @starypatyk

Fix issues with Media Tab & performance improvement.
@digitaltechconsult
Copy link
Contributor Author

Are there any news regarding testing and possible moving this change to the release?

@digitaltechconsult digitaltechconsult mentioned this pull request Feb 2, 2023
4 tasks
@starypatyk
Copy link
Contributor

@eraser2021999
Sorry - I have not been able to look at it yet. Maybe around weekend I will have some more time. No promises though...

@starypatyk
Copy link
Contributor

@eraser2021999

I have run some tests and with these changes the Media tab is indeed much more stable. 👍 I have not noticed negative side-effects.

I have put a few small comments above regarding unused variables - should be trivial to change. Also a trivial change suggested by Codacy Static Code Analysis: https://github.com/nextcloud/android/pull/11321/files#annotation_8372803674

I have managed to crash the app in the emulator - but this seems to be unrelated to this PR. On the actual phone the app was stable - although this was on an old version of Android (7). The previous version was even easier to crash. This will need to be investigated separately.

Generally - looks good to me.

PS. @eraser2021999 - When making updates, please do not sync your master branch with master in nextcloud/android. Hopefully this will let us avoid problems with Git.

@digitaltechconsult
Copy link
Contributor Author

@starypatyk @AlvaroBrey
I refactored the code based on your remarks. I retested the application with both scenarios (empty gallery and with some old media already loaded in the local storage via folder navigation), everything seems to work as expected.

I kindly ask you to retest, to make sure I didn't introduced new bugs and, if everything is ok.

Thanks a lot!

@codecov
Copy link

codecov bot commented Feb 5, 2023

Codecov Report

Merging #11321 (3ce6a16) into master (aeb1632) will increase coverage by 0.15%.
The diff coverage is 20.00%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #11321      +/-   ##
============================================
+ Coverage     31.44%   31.60%   +0.15%     
- Complexity     3413     3424      +11     
============================================
  Files           575      575              
  Lines         42018    42016       -2     
  Branches       5660     5662       +2     
============================================
+ Hits          13214    13279      +65     
+ Misses        26861    26778      -83     
- Partials       1943     1959      +16     
Impacted Files Coverage Δ
...cloud/android/ui/asynctasks/GallerySearchTask.java 34.21% <10.34%> (-2.78%) ⬇️
.../owncloud/android/ui/fragment/GalleryFragment.java 46.29% <27.77%> (-3.41%) ⬇️
...loud/android/ui/fragment/ExtendedListFragment.java 51.23% <0.00%> (-2.13%) ⬇️
...wncloud/android/providers/FileContentProvider.java 71.91% <0.00%> (-1.55%) ⬇️
.../third_parties/daveKoeller/AlphanumComparator.java 86.90% <0.00%> (-1.20%) ⬇️
...xtcloud/client/account/UserAccountManagerImpl.java 69.33% <0.00%> (-0.67%) ⬇️
...cloud/android/ui/activity/SyncedFoldersActivity.kt 20.53% <0.00%> (-0.45%) ⬇️
...loud/android/datamodel/FileDataStorageManager.java 62.52% <0.00%> (+0.16%) ⬆️
.../java/com/owncloud/android/utils/DisplayUtils.java 37.10% <0.00%> (+0.57%) ⬆️
...ain/java/com/owncloud/android/db/UploadResult.java 39.74% <0.00%> (+1.28%) ⬆️
... and 7 more

Copy link
Contributor

@starypatyk starypatyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested briefly, looks fine. 👍

Thanks @eraser2021999!

@szaimen szaimen requested review from tobiasKaminsky, AlvaroBrey and Unpublished and removed request for AlvaroBrey February 6, 2023 14:28
@digitaltechconsult
Copy link
Contributor Author

Any update on the release progress?

@AlvaroBrey
Copy link
Member

@tobiasKaminsky can you review?

@szaimen
Copy link
Contributor

szaimen commented Mar 19, 2023

@tobiasKaminsky what is missing here?

@digitaltechconsult
Copy link
Contributor Author

Wow, really 2 months for a code review...do you plan to implement it? @szaimen @AlvaroBrey

@tobiasKaminsky
Copy link
Member

Sorry for the delay, we are currently a bit understaffed… :S

@tobiasKaminsky tobiasKaminsky merged commit 4266698 into nextcloud:master Apr 12, 2023
@szaimen
Copy link
Contributor

szaimen commented May 18, 2023

Thanks a lot for this PR! Just tested it and works much faster! 🚀🎉

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

Successfully merging this pull request may close these issues.

5 participants