Skip to content

Move MagnifierBuilder, MagnifierOverlayInfoBearer from text_selection.dart#108956

Merged
auto-submit[bot] merged 14 commits intoflutter:masterfrom
tgucio:move-magnifier-classes
Aug 10, 2022
Merged

Move MagnifierBuilder, MagnifierOverlayInfoBearer from text_selection.dart#108956
auto-submit[bot] merged 14 commits intoflutter:masterfrom
tgucio:move-magnifier-classes

Conversation

@tgucio
Copy link
Contributor

@tgucio tgucio commented Aug 4, 2022

This PR moves the MagnifierBuilder, MagnifierOverlayInfoBearer and TextMagnifierConfiguration classes from text_selection.dart to magnifier.dart where they seem to belong better. Additional changes:

  • MagnifierOverlayInfoBearer. _fromRenderEditable constructor is replaced by a simple method TextSelectionOverlay. _buildMagnifier()
  • MagnifierOverlayInfoBearer.empty() constructor is replaced with static const MagnifierOverlayInfoBearer.empty

test-exempt: refactor only, no functional change.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Aug 4, 2022
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@tgucio
Copy link
Contributor Author

tgucio commented Aug 4, 2022

/cc @antholeole

@antholeole
Copy link
Contributor

I can get behind this. I considered making this change multiple times. The reason why I didn't was because I wanted everything in widgets/magnifier.dart to be text-editing agnostic, since it can lead users to think the magnifier can only be used for text editing. That being said, These methods and data classes are tightly coupled to both the magnifier and the selectionOverlay, and could easily go in both places.

cc @Renzo-Olivares what are you thoughts? I'm leaning towards approval because this is a change I was going to make myself but opted not to. If it was suggested so quickly, I probably made the wrong choice :)

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a comment

Choose a reason for hiding this comment

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

I'm onboard with this change. Cleans up text_selection.dart a bit. Thanks for the change! Just had some small nits.

@Renzo-Olivares
Copy link
Contributor

Renzo-Olivares commented Aug 4, 2022

I can get behind this. I considered making this change multiple times. The reason why I didn't was because I wanted everything in widgets/magnifier.dart to be text-editing agnostic, since it can lead users to think the magnifier can only be used for text editing. That being said, These methods and data classes are tightly coupled to both the magnifier and the selectionOverlay, and could easily go in both places.

cc @Renzo-Olivares what are you thoughts? I'm leaning towards approval because this is a change I was going to make myself but opted not to. If it was suggested so quickly, I probably made the wrong choice :)

I think I like these better in magnifier.dart, but I agree it could go in either. This cleans up text_selection.dart a bit. I like the use of the _buildMagnifier function vs MagnifierOverlayInfoBearer._fromRenderEditable.

@antholeole
Copy link
Contributor

On board! @tgucio do you mind cleaning up those nits? Definitely my mess leftover from my PR, sorry about that.

@antholeole antholeole self-requested a review August 4, 2022 20:58
@tgucio
Copy link
Contributor Author

tgucio commented Aug 4, 2022

@antholeole sure thing - will update in a bit. Coud you also maybe add the "automerge" label?

Edit: I've thrown in a change in the MagnifierOverlayInfoBearer equality operator and spelling corrections.

@Renzo-Olivares
Copy link
Contributor

@tgucio Could you ask @Hixie for a test-exempt on discord? Thanks

@Hixie
Copy link
Contributor

Hixie commented Aug 8, 2022

test-exempt: code refactor with no semantic change

@Renzo-Olivares Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 8, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 8, 2022

  • Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 8, 2022

Validations Fail.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 8, 2022
@tgucio
Copy link
Contributor Author

tgucio commented Aug 8, 2022

Bad bot.
/cc @LongCatIsLooong @justinmc @chunhtai to see if they could take a look and add the autosubmit label?

@flutter-dashboard flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Aug 10, 2022
@tgucio
Copy link
Contributor Author

tgucio commented Aug 10, 2022

I somehow missed TextMagnifierConfiguration in the original change so moved this one to magnifier.dart too. @antholeole @Renzo-Olivares could you PTAL?
Also converted MagnifierOverlayInfoBearer.empty to a static const so it's a single compile time instance.

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM, just some nits

@tgucio
Copy link
Contributor Author

tgucio commented Aug 10, 2022

If someone could label this with "autosubmit" again that'd be great.

@Renzo-Olivares Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 10, 2022
@auto-submit auto-submit bot merged commit 5be898a into flutter:master Aug 10, 2022
@tgucio tgucio deleted the move-magnifier-classes branch August 10, 2022 20:46
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants