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

[SuperEditor][Android] Show expanded handles when expanding the selection (Resolves #1937) #2014

Merged
merged 3 commits into from
May 22, 2024

Conversation

angelosilvestre
Copy link
Collaborator

[SuperEditor][Android] Show expanded handles when expanding the selection. Resolves #1937

When the selection changes from collapsed to expanded, the collapsed handle is visible at the beginning of the selection, and the expanded handles aren't visible:

collapsed.webm

This PR fixes the issue by changing the visibility of the handles when needed.

@matthew-carroll
Copy link
Contributor

@angelosilvestre please post a video of the solution

if (widget.selection.value != null &&
widget.selection.value?.isCollapsed == false &&
_controlsController!.shouldShowCollapsedHandle.value == true) {
// The selection is expanded, but the collapsed handle is visible. This can happen when the
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads like a bandaid - it says "here's a state that shouldn't happen, but does happen, so we'll correct it here".

Why is the wrong thing happening to begin with? Why is this the right place to correct it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, usually we show/hide the handles in the touch handlers of the interactor. A double tap shows the expanded handle, a single tap shows the collapsed handle, etc...

In this case the selection is changed elsewhere. So I think we should be changing the handle visibility when the selection changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case the selection is changed elsewhere

Where is it being changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@angelosilvestre did you see this question?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@matthew-carroll I haven't noticed this, sorry.

The "Select All" button issues a ChangeSelectionRequest, this isn't initiated by one of our widgets.

@angelosilvestre
Copy link
Collaborator Author

expanded.webm

@matthew-carroll
Copy link
Contributor

@angelosilvestre this PR now has some conflicts after another PR was merged.

Also, can you take a look at the failing website deployment actions? They seem to be failing in a number of PRs but I'm not sure why.

@angelosilvestre angelosilvestre force-pushed the 1937_android-show-expanded-handles branch from 35b8412 to 92e60bc Compare May 20, 2024 23:26
@angelosilvestre
Copy link
Collaborator Author

@matthew-carroll I resolved the conflicts.

The website build failure seems to be due to the LinkAttribution constructor change, where now we expect a String. I already solved this in my website PR, so for new PR's this should fail again.

return;
}

if (selection.isCollapsed == true &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that you're doing == true and == false for a boolean value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously we were handling a nullable value and I forgot to update it. I updated it now.

Copy link
Contributor

@matthew-carroll matthew-carroll left a comment

Choose a reason for hiding this comment

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

LGTM

@matthew-carroll matthew-carroll merged commit ffe2f95 into main May 22, 2024
11 checks passed
@matthew-carroll matthew-carroll deleted the 1937_android-show-expanded-handles branch May 22, 2024 03:48
github-actions bot pushed a commit that referenced this pull request May 22, 2024
quaaantumdev pushed a commit to quaaantumdev/super_editor that referenced this pull request May 23, 2024
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.

Under certain conditions, text selection handlers appear in the wrong place on Android
2 participants