Fix deleting from slideshow #15669
Fix deleting from slideshow #15669
Conversation
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
alperozturk96
force-pushed
the
fix_15636_deleting_from_slideshow
branch
from
94917e8 to
3143bb7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
PhilLab
force-pushed
the
fix_15636_deleting_from_slideshow
branch
2 times, most recently
from
a93ea4d to
dd711d0
Compare
The patch added in nextcloud#14782 prevented that the callback onRemoteOperationFinish() was ever called. Secondly, the manual update of adapter and pager is only needed if user is not present. When user is present, the initViewPager() takes care of everything. Signed-off-by: Philipp Hasper <[email protected]>
In that case, deleting the currently viewed file had the following issues: 1. When deleting the first image, the UI just stayed with it. 2. When deleting another image, the UI switched to the previous image, instead of the next one, which is inconsistent to the behavior when user is available 3. When switching to the next image, the title shown at the top did not correctly update Signed-off-by: Philipp Hasper <[email protected]>
PhilLab
force-pushed
the
fix_15636_deleting_from_slideshow
branch
2 times, most recently
from
e04c528 to
806900a
Compare
|
|
||
| if (user.isPresent) { | ||
| // Re-init the view pager, which will advance to the next image | ||
| initViewPager(user.get()) |
There was a problem hiding this comment.
After an image is deleted from the preview, the first condition is matched, and we end up re-initializing the ViewPager, but updateViewPagerAfterDeletionAndAdvanceForward isn’t triggered. I was thinking, since this logic is already inside the if (operation is RemoveFileOperation) block, could we consider calling only updateViewPagerAfterDeletionAndAdvanceForward when the result is successful instead of re-initializing the ViewPager entirely or any other reason to have user.present check and initViewPager ?
e.g.
if (result.isSuccess) {
updateViewPagerAfterDeletionAndAdvanceForward()
}
What do you think?
There was a problem hiding this comment.
@alperozturk96 yes, good idea. Though the if (user.isPresent) { initViewPager has been added deliberately here and been kept ever since.
So I am wondering what this would break.
There was a problem hiding this comment.
@tobiasKaminsky What do you think?
|
You can fix |
3 participants
Fixes #15636.
onRemoteOperationFinish()was ever called.The changes were removed. I was not able to reproduce the end-to-end encryption issue, so this seems fine.
initViewPager()takes care of everything.