flutter

flutter copied to clipboard

This PR moves the MagnifierBuilder and MagnifierOverlayInfoBearer classes from text_selection.dart to magnifier.dart where they seem to belong better. MagnifierOverlayInfoBearer. _fromRenderEditable constructor is replaced by a simple method TextSelectionOverlay. _buildMagnifier().

test-exempt: refactor only, no functional change.

Pre-launch Checklist

• [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
• [x] I signed the [CLA].
• [ ] I listed at least one issue that this PR fixes in the description above.
• [x] 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].
• [x] All existing and new tests are passing.

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.

/cc @antholeole

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 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.

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

@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.

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

test-exempt: code refactor with no semantic change

• 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.

Validations Fail.

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

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.

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