Bugfix and automated tests for #15636: deleting from slideshow

[PhilLab]

Original PR: #16184

• Tests written, or not not needed

This tests the deletion behavior when deleting from slide show, fixed in #15636. It also fixes another bug which was uncovered during the automated tests, where the view pager was not correctly updated.

This PR requires #16296 to be merged first, as the automated tests also stumble upon the same timing issue fixed (and tested!) in said PR.

[PhilLab]

@tobiasKaminsky @alperozturk96 Now I am concerned. I checked this PR's drone run and saw that the PreviewImageActivityIT.kt was checked out, but apparently did not actually run.

I then created a PR to provoke a failure, #16329 (comment), and its drone test ran through: https://drone.nextcloud.com/nextcloud/android/27697

Where is the error?

Update Seems this is a more general problem, as many more provoked failures are not catched in the CI. I created #16350 to track this. So it is more or less independent of the PR here, as it is a more widespread issue.

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

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]

Hello

Thank you for the PR and for the updates.

I tested it with the latest master branch. I noticed that when a user deletes a picture from the preview, the next image appears not file listing as expected so I couldn't able to reproduce the bug.

Also, the onRemoveFileOperationFinish function updates the adapter and should trigger the UI updates, rather than handling it directly in RemoveFilesDialogFragment.kt‎.

As a side note, not directly related to this PR: the line fda?.refreshCurrentDirectory() may be unnecessary, since onRemoveFileOperationFinish already performs the update.

I suggest to remove unnecessary lines (refreshDirectory, initPager) from RemoveFilesDialogFragment.kt‎ , test again and add tests for these scenarios.

Thanks again for your work on this.

[PhilLab]

@alperozturk96

I tested it with the latest master branch. I noticed that when a user deletes a picture from the preview, the next image appears not file listing as expected so I couldn't able to reproduce the bug.

Exactly. This is because this PR is a follow-up of the already merged bug fix, see the PR description:

This tests the deletion behavior when deleting from slide show, fixed in #15636.

I created the new PR to "guard" my fix 😅 against future regressions. If you want to experience the bug, you would need to revert the mentioned PR. Then, you'll also see the automated PreviewImageActivityIT fail.

And you can reproduce

another bug which was uncovered during the automated tests, where the view pager was not correctly updated.

by checking out this branch here, locally reverting 74a9a3d and then run the PreviewImageActivityIT. It will then fail and prove the existence of another bug.

Regarding your code suggestion: Trusting on onRemoveFileOperationFinish() doesn't work, I tried. It is not called for local only deletions, because it is only called from FileDisplayActivity.onRemoteOperationFinish().
So I matched the existing fda?.refreshCurrentDirectory() and added pia?.initViewPager() alongside.

I'd prefer to keep the changes as small as possible - a refactoring to have the file operation finish callback work for every type of deletion feels like a significantly larger scope. Maybe a follow-up of this PR?

[alperozturk96]

@alperozturk96

I tested it with the latest master branch. I noticed that when a user deletes a picture from the preview, the next image appears not file listing as expected so I couldn't able to reproduce the bug.

Exactly. This is because this PR is a follow-up of the already merged bug fix, see the PR description:

This tests the deletion behavior when deleting from slide show, fixed in #15636.

I created the new PR to "guard" my fix 😅 against future regressions. If you want to experience the bug, you would need to revert the mentioned PR. Then, you'll also see the automated PreviewImageActivityIT fail.

And you can reproduce

another bug which was uncovered during the automated tests, where the view pager was not correctly updated.

by checking out this branch here, locally reverting 74a9a3d and then run the PreviewImageActivityIT. It will then fail and prove the existence of another bug.

Regarding your code suggestion: Trusting on onRemoveFileOperationFinish() doesn't work, I tried. It is not called for local only deletions, because it is only called from FileDisplayActivity.onRemoteOperationFinish(). So I matched the existing fda?.refreshCurrentDirectory() and added pia?.initViewPager() alongside.

I'd prefer to keep the changes as small as possible - a refactoring to have the file operation finish callback work for every type of deletion feels like a significantly larger scope. Maybe a follow-up of this PR?

I think adding a guard here isn’t the right direction. The fix should address the underlying bug directly, without needing additional defensive logic.

So I matched the existing fda?.refreshCurrentDirectory() and added pia?.initViewPager() alongside.

That’s actually what concerns me. It introduces more conditional and tightly coupled behavior into a dialog fragment.

RemoveFilesDialogFragment is a dialog fragment, and its responsibility should remain limited to presenting the dialog. Expanding its role increases complexity and makes the flow harder to reason about.

If onRemoteOperationFinish currently doesn’t handle local-only deletions, a solution would be for RemoveFilesDialogFragment to implement an interface to handle that case explicitly. That would solve the issue with fewer changes and clearer ownership.

This approach would keep the architecture simpler and make the fix easier to maintain going forward.

[PhilLab]

I think adding a guard here isn’t the right direction. The fix should address the underlying bug directly, without needing additional defensive logic.

@alperozturk96 okay, take a look I just added two commits:

1. fix(slideshow) Dependency-inject ConnectivityService
2. fix(slideshow): Use EventBus to notify activies of file deletion

I used the EventBus, as I saw it being used elsewhere in the repository, so I would use the same here.

[github-actions]

Codacy

SpotBugs

Category	Base	New
Bad practice	42	42
Correctness	75	75
Dodgy code	256	256
Experimental	1	1
Internationalization	7	7
Malicious code vulnerability	2	2
Multithreaded correctness	34	34
Performance	44	44
Security	18	18
Total	479	479

[github-actions]

blue-Light-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

[github-actions]

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/16297.apk



To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

[alperozturk96]

Hello

Thank you very much for your effort here, we truly appreciate the work you’ve put into this. Improving testability is very valuable for us, and your contribution definitely helps move things in that direction.

That said, we’ve previously aligned on gradually eliminating EventBus and LocalBroadcastManager usage over time. This PR appears to introduce additional usages, though I completely understand that you are not aware of it. @tobiasKaminsky fyi.

Please correct me if I’m misunderstanding, but as I see it, this PR does not fix a new issue and instead focuses on adding tests and making the current logic testable. Is that correct? As far as I can tell, the related bug was already addressed in #15669, and the loading dialog timing issue was handled in #16296.

If that is correct, I would suggest that we first align on how we want to approach improving testability in these areas without introducing new EventBus dependencies. I’d be very happy to hear other opinions and discuss possible alternatives so we can decide on the most appropriate approach.

If this PR does address a specific bug that is not covered by the previous fixes, could you please share the steps to reproduce it? That would help us better understand the context and evaluate the changes more accurately.

Once again, thank you for your contribution. It’s great to see the focus on improving testability, and we genuinely appreciate the effort you’ve put into this.

[PhilLab]

@alperozturk96 the second commit

fix(slideshow): After file deletion, properly refresh view pager

Otherwise, the localOnly file deletion will not update the UI.

does fix a real issue with the local-only file deletion. This can be reproduced by running the test without the fix (checkout the first commit)