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 Image becoming unclickable after using double tap zoom #20

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

K1rakishou
Copy link

@K1rakishou K1rakishou commented May 27, 2023

Fixes #18

@saket saket self-requested a review May 27, 2023 12:53
Copy link
Owner

@saket saket left a comment

Choose a reason for hiding this comment

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

Thank you @K1rakishou! Let's move the test to its correct location and this should be good to go.

private val composableTag = "ZoomableAsyncImage"

@Test
fun must_still_be_clickable_after_double_tap_zooming() = runTest {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove this test and move the assertions ZoomableImageTest which already has a test for click listeners?

Copy link
Owner

Choose a reason for hiding this comment

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

That'll also remove the need for having a base test class. Base classes aren't great 🙂.

state.handleDoubleTapZoomTo(centroid = centroid)
} finally {
isQuickZooming = false
}
Copy link
Owner

Choose a reason for hiding this comment

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

Good catch!

Copy link
Owner

@saket saket left a comment

Choose a reason for hiding this comment

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

Thanks @K1rakishou!

@saket
Copy link
Owner

saket commented May 30, 2023

Can you rebase on trunk so that this can be merged? It should be a fast forward if you squash your commits into one.

@saket saket force-pushed the (#18)-fix-image-becoming-unclickable branch from d64cfe7 to 8acff3d Compare June 1, 2023 15:42
Fix Zoomable not resetting isQuickZooming to false after calling handleDoubleTapZoomTo() because it may throw CancellationException which wasn't handled.

Unzoom back and check that the image is still clickable.

Post review fixes.

- Move click tests to click_listeners_work().
- Revert CoilImageSourceTest.
- Remove BaseTest and ClicksTest.

Revert this too.
@saket saket force-pushed the (#18)-fix-image-becoming-unclickable branch from 8acff3d to 8b7815f Compare June 1, 2023 15:45
@emulator-wtf
Copy link

emulator-wtf bot commented Jun 1, 2023

:zoomable-image:sub-sampling-image - Tests were flaky!

✅ 22 passed / ⚠️ 1 flaky

First failure encountered:

me.saket.telephoto.subsampling.SubSamplingImageTest.draw_base_tile_to_fill_gaps_in_foreground_tiles click for details

Test failed to run to completion. Reason: 'Instrumentation run failed due to 'keyDispatchingTimedOut''. Check device logcat for details

@saket saket merged commit 481a4c5 into saket:trunk Jun 1, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] onClick does not work after a double tap zoom gesture
2 participants