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.

Sign up for GitHub

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

Jump to bottom

Magnifier for iOS 17+ #1000

Merged
merged 20 commits into from
Apr 5, 2024
Merged

Magnifier for iOS 17+ #1000

merged 20 commits into from
Apr 5, 2024

Conversation

alexzhirkevich
Copy link

Proposed Changes

  • Text field and selection container magnifier for iOS 17+

Testing

Test: try magnifier in all types of text fields

Issues Fixed

Partly: JetBrains/compose-multiplatform#3692

@alexzhirkevich alexzhirkevich mentioned this pull request Jan 18, 2024
Closed
@alexzhirkevich
Copy link
Author

What is the correct way to get local UIView from foundation module? Seems like there no more LocalLayerContainer and interop container is something different

@mazunin-v-jb
Copy link

@alexzhirkevich afaik for now we don't have such API
@dima-avdeev-jb correct me please if I'm wrong

@mazunin-v-jb
Copy link

PR looks good to me. Although, it shouldn't go to the next release, so I'll leave this PR without an approval for now.
And we should wait for the second review.

@alexzhirkevich
Copy link
Author

There is 1 issue.
Text selection in debug build causes significant FPS drops on my old 12 mini (especially in low power mode, but in default mode too). There are no drops when displaying magnifier outside of text bounds (for example in empty text field). I measured time of magnifier position calculation and it's fine: most of the time it's <1ms and sometimes it's 1ms (in LPM at the high cpu load moments).

Then I checked jb-main branch and it has such FPS drops too. Magnifier just makes it much more noticeable because of freezing spring animation.

Can it potentially be some expensive calls in selection logic?

Here is what i'm talking about (jb-main and magnifier):

jbmain.MP4
magnifier.MP4

@MatkovIvan
Copy link
Member

@alexzhirkevich Sorry for the delay, the team is busy with the upcoming 1.6.0 release. We'll come back to this after that

@ASalavei ASalavei requested review from ASalavei and removed request for dima-avdeev-jb March 15, 2024 13:15
@mazunin-v-jb
Copy link

@alexzhirkevich
We're ready to review. Could you please resolve merge conflicts?

@alexzhirkevich
Copy link
Author

Fixed conflicts, but found a bug.
When you select some text and delete it or start typing while holding the loupe - it getting stuck. Appreciate any help

@alexzhirkevich
Copy link
Author

alexzhirkevich commented Mar 22, 2024 •
edited
Loading

When you select some text and delete it or start typing while holding the loupe - it getting stuck

Fixed it. The problem was when SelectionHandle with detectDragGestures modifier is disposed before drag finish neither onDragEnd nor onDragCancel is called. I guess it wasn't reproducible on android only because you can't use software keyboard while selecting text (at least on android device i have).

@ASalavei
Copy link
Collaborator

ASalavei commented Apr 4, 2024

Just to warn: because of the platform layers refactoring done here, it might be a problem with magnifier positioning. Depending on which MR is merged first, the bug may need to be fixed in another one.

ASalavei
ASalavei approved these changes Apr 4, 2024
View reviewed changes
Copy link
Collaborator

@ASalavei ASalavei left a comment

Choose a reason for hiding this comment

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

LGTM from my side. Thank you for the feature!

mazunin-v-jb
mazunin-v-jb approved these changes Apr 5, 2024
View reviewed changes
Copy link

@mazunin-v-jb mazunin-v-jb left a comment

Choose a reason for hiding this comment

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

looks good to me too
thank you for PR!

@mazunin-v-jb mazunin-v-jb merged commit 5a698ba into JetBrains:jb-main Apr 5, 2024
1 of 2 checks passed
@elijah-semyonov
Copy link

elijah-semyonov commented Apr 17, 2024 •
edited
Loading

The PR is related to a crash on pre iOS 17 devices
JetBrains/compose-multiplatform#4644

The symbol UITextLoupeSession is uniquely referenced here.

alexzhirkevich
alexzhirkevich commented Apr 17, 2024
View reviewed changes
...se/foundation/foundation/src/uikitMain/kotlin/androidx/compose/foundation/Magnifier.uikit.kt
}

internal fun isPlatformMagnifierSupported() =
available(OS.Ios to OSVersion(major = 17))
Copy link
Author

Choose a reason for hiding this comment

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

There is a check for iOS version. It is implemented incorrectly?

Copy link

@elijah-semyonov elijah-semyonov Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's implemented correctly, but the problem is due to linking. K/N can see the symbol and tried to link it during the launch.

Copy link

Choose a reason for hiding this comment

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

I'll promote this problem to K/N team, the only known workaround as of now is to use strings and C-apis directly to avoid mentioning the symbol altogether.

Copy link
Author

Choose a reason for hiding this comment

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

use strings and C-apis directly

Do you mean using cinterop like here or something different?

Copy link

@elijah-semyonov elijah-semyonov Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, I actually meant barebone ObjC runtime, but @ASalavei came with a much better solution to simply hide the symbols from cinterop

Copy link
Author

@alexzhirkevich alexzhirkevich Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

simply hide the symbols from cinterop

I thought about it too. Nice solution.
I seen some places in skiko where iOS 13 is checked the same way and class/method is loaded. Should it be fixed as well?

Copy link

@elijah-semyonov elijah-semyonov Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Methods are not a big deal, ObjC will simply throw unrecognised selector exception in case we don't do a version check. The classes are a problem (currently), can you leave a link?

Copy link
Collaborator

@ASalavei ASalavei Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I found one place in Skiko, and it seems like No, there are no iOS 13 specific classes, only a method call, which is wrapped into proper check.

@alexzhirkevich alexzhirkevich deleted the uikit-magnifier branch July 15, 2024 09:10
@alexzhirkevich alexzhirkevich mentioned this pull request Sep 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ASalavei ASalavei ASalavei approved these changes

@elijah-semyonov elijah-semyonov elijah-semyonov left review comments

@MatkovIvan MatkovIvan MatkovIvan left review comments

@mazunin-v-jb mazunin-v-jb mazunin-v-jb approved these changes

Assignees
No one assigned
Labels
changes in common
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alexzhirkevich @mazunin-v-jb @MatkovIvan @ASalavei @elijah-semyonov