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

AOSP commonization: removing seemingly redundant drag cancellation for iOS magnifier #1713

Closed

Conversation

mazunin-v-jb
Copy link

Potentially fixes CMP-5763 and CMP-5764

I wasn't able to reproduce the bug, which should be fixed by these lines (it was introduced here), so I suggest retesting magnifier behavior without these lines and removing them afterwards if my suspicion is correct.

…FieldSelectionManager (for commonization with AOSP)
@mazunin-v-jb mazunin-v-jb marked this pull request as draft December 9, 2024 12:48
@mazunin-v-jb
Copy link
Author

on draft until tested

@mazunin-v-jb
Copy link
Author

Retested
Issue wasn't reproduced on this branch -> we should remove these lines

@mazunin-v-jb mazunin-v-jb marked this pull request as ready for review December 10, 2024 18:31
@@ -461,10 +461,7 @@ internal class TextFieldSelectionManager(val undoManager: UndoManager? = null) {
updateFloatingToolbar(show = true)
}

override fun onCancel() {
draggingHandle = null
Copy link
Collaborator

@ASalavei ASalavei Dec 10, 2024

Choose a reason for hiding this comment

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

The code looks reasonable (The same happened in onStop()).
Could you please elaborate, what fixes this removal?

Copy link
Author

@mazunin-v-jb mazunin-v-jb Dec 11, 2024

Choose a reason for hiding this comment

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

It does look reasonable indeed.
My point here is that we should upstream this to AOSP, and the AOSP doesn't have these lines and doesn't have the stuck magnifier bug. It was assumed in the magnifier PR that this issue couldn't be reproduced on Android because the reproduction steps couldn't be replicated there, which turned out to be incorrect.
So I have tested that on physical iOS and Android devices, and I couldn't reproduce the issue without these lines.
Although I remember that bug too, I think we can remove these lines if we can't reproduce it now.

Copy link
Member

Choose a reason for hiding this comment

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

Can we upstream these lines to be on the safe side?

Copy link
Member

Choose a reason for hiding this comment

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

Or instead we can write some test in AOSP to make sure that we won't have regression here

@alexzhirkevich
Copy link

alexzhirkevich commented Dec 11, 2024

I could retest and record reproduction steps if the bug is still present, but i'm unable to build iOS part on macos 15 / xcode 16 - xcodebuild crashes on uikit utils module. Still can do it with IPA if you want or if there is a build workaround

@ASalavei
Copy link
Collaborator

ASalavei commented Dec 11, 2024

@alexzhirkevich , we don't have proper solution as well. A workaround is to use XCode 15.4 to build and test the app on a device. Build on simulator should work fine, just clean all cached (compose-mulatiplatform-core/out) and restart the gradle deamon.

@mazunin-v-jb
Copy link
Author

mazunin-v-jb commented Dec 11, 2024

@alexzhirkevich, in addition to @ASalavei answer, building on a device may require resetting the checkbox "Automatically manage signing" and changing bundle identifier for each build

upd. If you manage to reproduce it, could you please attach a video and the reproduction steps? Thanks in advance, you will help us greatly

@alexzhirkevich
Copy link

alexzhirkevich commented Dec 12, 2024

I confirm the bug is no longer reproducible

@mazunin-v-jb
Copy link
Author

@alexzhirkevich, many thanks!

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.

4 participants