Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[iOS text input] move FlutterTextInputView implementation to a different file #38044

Closed

Conversation

LongCatIsLooong
Copy link
Contributor

WIP

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 and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@LongCatIsLooong LongCatIsLooong force-pushed the ios-textinputplugin-arc branch 2 times, most recently from d207439 to 09ca27e Compare December 2, 2022 23:45
@skia-gold
Copy link

Gold has detected about 16 new digest(s) on patchset 5.
View them at https://flutter-engine-gold.skia.org/cl/github/38044

@LongCatIsLooong LongCatIsLooong force-pushed the ios-textinputplugin-arc branch 2 times, most recently from b8f748c to c7859f9 Compare December 3, 2022 01:07
@LongCatIsLooong LongCatIsLooong force-pushed the ios-textinputplugin-arc branch from c7859f9 to 6491f7a Compare December 5, 2022 18:49
@cyanglaz
Copy link
Contributor

cyanglaz commented Dec 7, 2022

@stuartmorgan You mentioned concerns about having MRC in ARC code base, which I agree.
I thought the opposite is OK. I can't think of any example of mistakenly writing MRC code in ARC files that will cause any bad things.

Are you concerned about having ARC code in MRC?

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Dec 8, 2022

I can't think of any example of mistakenly writing MRC code in ARC files that will cause any bad things.

Trying to write MRC code in an ARC file just won't compile, so there's no danger there.

Are you concerned about having ARC code in MRC?

In the context of an active transition, I don't think "in" is really a meaningful concept; there will be context switches back and forth, which means increased chances of messing up in an MRC file on accident, and that's not ideal. But it's unavoidable temporarily when migrating a codebase of any non-trivial size. The important thing IMO is to move toward a homogenous codebase, ideally minimizing the transition time as much as is reasonable to do. (By "reasonable" I mean to exclude things like "do the migration in a branch and then land it on main all at once", because that's a nightmare in practice and creates a different class of problems.) And the good news is that since the mistakes can only be in the MRC files, they should be temporary because those files are on the conversion list.

My comments about MRC-in-ARC were about concerns about a final state where we permanently leave some MRC files among mostly-ARC code, because those files become permanent landmines.

@cyanglaz
Copy link
Contributor

cyanglaz commented Dec 8, 2022

Trying to write MRC code in an ARC file just won't compile, so there's no danger there.

The only thing I can think of is having an instance variable in MRC is unsafely unretained (which compiles), and the same code in ARC would create a strong pointer. I think this is a good "fix" anyway so it is still ok.

there will be context switches back and forth, which means increased chances of messing up in an MRC file on accident, and that's not ideal.

I think for now it is "fine" since we only have a few files in ARC so we are still treating the codebase as MRC by default. I agree there will be a mentality change at some point where majority of the code in the code base is ARC and the codebase is treated as an ARC code base. We will need to have a plan before that happens and have the migration done in as less time as possible.

@LongCatIsLooong
Copy link
Contributor Author

also /cc @hellohuanlin

Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

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

Looks like you are doing 2 things in the same PR: 1. covert to ARC; 2. some refactoring works like moving code around.

Github didn't pick up those moving codes, which makes it pretty hard to review. Is it possible to split up those so that Github shows those changes side by side?

@@ -917,7 +917,7 @@ - (void)flutterTextInputView:(FlutterTextInputView*)textInputView
arguments:@[ @(client), stateString, position ]];
}

- (void)flutterTextInputView:(FlutterTextInputView*)textInputView
- (void)flutterTextInputView:(UIView<FlutterTextInputClient>*)textInputView
Copy link
Contributor

Choose a reason for hiding this comment

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

are these change required as part of the ARC migration?

@LongCatIsLooong
Copy link
Contributor Author

Extracted the ARC migration part to #38179

@LongCatIsLooong LongCatIsLooong changed the title [iOS text input] enable ARC & move FlutterTextInputView implementation to a different file [iOS text input] move FlutterTextInputView implementation to a different file Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants