Gallery multi select test and fix #16267
Conversation
PhilLab
changed the title
PhilLab
force-pushed
the
ph/gallery_multi_select_test_and_fix
branch
from
bba226a to
a97a3ed
Compare
The method actually returned the number of rows, but each row contains multiple files (currently: 2 or 5, depending on the display rotation). In the unit test, the expected file count was changed from the original and correct value 4 to 2 in the commit 66d8756, but the correct change is in the implementation. Hence, reinstating the old expected value and the test does pass. Signed-off-by: Philipp Hasper <[email protected]>
The crash came from a collision of the improperly coded hash function GalleryRow#calculateHashCode(). Simply summing the row's file hashes can easily cause a collision as the same sum can also be achieved by two other file hashes. Take two rows with two files each. All having the same parentId=0. Row 1: 263512 and 148830 Row 2: 279897 and 132445 Summing the hashes given by calculateHashCode() will return in the same value for Row1 and Row2. PR #15918 fixed this by migrating away from this faulty hash function. This commit tests the bugfix first with a known collision and then some random fileIds for some additional explorative testing. This commit also removes the problematic calculateHashCode(), as it is now unused. Signed-off-by: Philipp Hasper <[email protected]>
This was originally meant to find the cause of crashes caused by non-unique IDs in the ViewHolder (#15918 and #16194). But the UI test still succeeded because it didn't carefully craft the fileIds to cause the issue. Once the actual reason, a collision of an improper hash function, was found, a unit test for that scenario was added instead - see prior commit. Committing this UI test anyways, as it might be able to catch other, future regressions. Signed-off-by: Philipp Hasper <[email protected]>
ThumbnailsCacheManager#clearCache() needs to mutex the access, otherwise there is a race condition with initDiskCacheAsync(), as observed in a small fraction of GalleryFragmentIT tests Signed-off-by: Philipp Hasper <[email protected]>
The CI will fail the build in case of long lines, so at least warn the user before. As the CI sees this as error, we might also set this as error in the IDE, but for now let's be conservative and only show a warning Signed-off-by: Philipp Hasper <[email protected]>
alperozturk96
force-pushed
the
ph/gallery_multi_select_test_and_fix
branch
from
a97a3ed to
c1f393d
Compare
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/16267.apk |
|
blue-Light-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed. |
|
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.) |
Uh oh!
There was an error while loading. Please reload this page.