Remove unused context parameter by justinmc · Pull Request #124254 · flutter/flutter

justinmc · 2023-04-05T18:54:39Z

I noticed a context parameter that was unused in these methods, so I've removed them. This could be a breaking change, but because it was just introduced, it's probably unlikely.

…t for localizations

camsim99

This seems legitimate to me. Thanks!

camsim99 · 2023-04-17T23:41:24Z

packages/flutter/test/material/spell_check_suggestions_toolbar_test.dart

    expect(toolbarY, equals(expectedToolbarY));
  });
+
+  test('buildButtonItems builds only a delete button when no suggestions', () {


Great addition!

camsim99 · 2023-04-17T23:41:53Z

packages/flutter/test/cupertino/spell_check_suggestions_toolbar_test.dart

+void main() {
+  TestWidgetsFlutterBinding.ensureInitialized();
+
+  testWidgets('buildButtonItems builds a "No Replacements Found" button when no suggestions', (WidgetTester tester) async {


Great addition!!

flutter/flutter@15cb1f8...42fb0b2 2023-04-18 katelovett@google.com Fix text theme dart fix cases (flutter/flutter#125052) 2023-04-18 gspencergoog@users.noreply.github.com Update the copy icon in snippets and samples to use the standard one (flutter/flutter#123651) 2023-04-18 jmccandless@google.com Remove unused context parameter (flutter/flutter#124254) 2023-04-18 jmccandless@google.com iOS spell check cursor placement (flutter/flutter#124875) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from d2973619074e to 55bb065c607b (1 revision) (flutter/flutter#125047) 2023-04-18 zanderso@users.noreply.github.com Stop running "_impeller_" benchmark variants (flutter/flutter#125044) 2023-04-18 engine-flutter-autoroll@skia.org Roll Packages from 0277f2a to faf53fb (7 revisions) (flutter/flutter#125040) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from c4396f9f602f to d2973619074e (6 revisions) (flutter/flutter#125039) 2023-04-18 fluttergithubbot@gmail.com Roll pub packages (flutter/flutter#125005) 2023-04-18 pateltirth454@gmail.com [InputDatePickerFormField] adds acceptEmptyDate to InputDatePickerFormField Widget (flutter/flutter#124143) 2023-04-18 44755140+werainkhatri@users.noreply.github.com relayout active ListWheelScrollView children every performLayout (flutter/flutter#124476) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4a603aaff32e to c4396f9f602f (2 revisions) (flutter/flutter#125007) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 20034a8d62c4 to 4a603aaff32e (2 revisions) (flutter/flutter#125004) 2023-04-18 katelovett@google.com Add optional axis specifier to static scrollable methods (flutter/flutter#124894) 2023-04-17 barpac02@gmail.com Update usage of standalone`pub` executable in flutter_tools testing docs (flutter/flutter#124898) 2023-04-17 warriorharish95668@gmail.com Add Harish Anbalagan to AUTHORS (flutter/flutter#124684) 2023-04-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from b2d07388ceb6 to 20034a8d62c4 (7 revisions) (flutter/flutter#125001) 2023-04-17 36861262+QuncCccccc@users.noreply.github.com Add an example for SearchBar (flutter/flutter#124992) 2023-04-17 47866232+chunhtai@users.noreply.github.com SelectionContainer's listeners can remove itself during listener callâ�¦ (flutter/flutter#124624) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com,ychris@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

werainkhatri · 2023-04-19T14:47:41Z

shouldn't there be a lint to prevent this from happening? unused parameter lint?

justinmc · 2023-04-20T22:53:57Z

I agree, but in this case it's a public API, and we could potentially be providing the parameter to Flutter users even though it's not used internally in the framework itself.

I'm not sure if we have a lint that would catch it for a private API. We should.

Hixie · 2023-04-20T22:55:23Z

This is a surprising change to me. Generally all build methods in public APIs should have the context, otherwise they can't do things like MediaQuery.of or CupertinoTheme.of or whatnot. Is this not that kind of build method?

justinmc · 2023-04-21T00:12:15Z

Hmm I'll look at this more closely tomorrow, maybe this is just a brain fart on my part as I tried to clean this up. I think I didn't think of it as a typical builder because it doesn't return a Widget. Also there is editableTextState.context, but maybe that's not what should be used here.

Hixie · 2023-04-21T00:16:15Z

(to be clear i'm not familiar with this code and am quite ready to believe these aren't normal builders and that it's fine to make this change)

justinmc · 2023-04-21T17:11:31Z

Ok after taking a closer look, I'm going to advocate for keeping it as-is (no context parameter). These buildButtonItems methods are "building" information about the buttons, which are later turned into widgets elsewhere. They shouldn't need to look up things like MediaQuery or Theme. The one reason to use context would be for localizations, but I think it makes sense to get that from editableTextState.

Maybe the build name was a misnomer here. I think these methods might be more analogous to getEditableButtonItems than a builder.

I'm definitely open to be proven wrong here though. I'll keep an eye out if anyone opens an issue about this or shows confusion for lack of a BuildContext parameter. Also, @camsim99 you're the original author here, let me know if you think I'm not on track with what you intended for these.

Tidying up the spell check API.

@itsjustkevin

This is a cherry pick of 8 of my recent spell check bug fixes into the beta branch. 1. #124259 2. #124875 3. #124254 4. #124899 5. #124895 6. #125162 7. #124897 8. #125432 This is the behavior of spell check with these changes: | Screenshot | Video | | --- | --- | | <img src="https://user-images.githubusercontent.com/389558/234087650-bcd62c89-03e7-427d-afc5-0fe8f96a5f80.png" /> | <video src="https://user-images.githubusercontent.com/389558/234087667-651b0fde-348c-467e-ba00-27b6b3966a27.mov" /> | CC @itsjustkevin @leighajarett

flutter/flutter@15cb1f8...42fb0b2 2023-04-18 katelovett@google.com Fix text theme dart fix cases (flutter/flutter#125052) 2023-04-18 gspencergoog@users.noreply.github.com Update the copy icon in snippets and samples to use the standard one (flutter/flutter#123651) 2023-04-18 jmccandless@google.com Remove unused context parameter (flutter/flutter#124254) 2023-04-18 jmccandless@google.com iOS spell check cursor placement (flutter/flutter#124875) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from d2973619074e to 55bb065c607b (1 revision) (flutter/flutter#125047) 2023-04-18 zanderso@users.noreply.github.com Stop running "_impeller_" benchmark variants (flutter/flutter#125044) 2023-04-18 engine-flutter-autoroll@skia.org Roll Packages from 0277f2a to faf53fb (7 revisions) (flutter/flutter#125040) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from c4396f9f602f to d2973619074e (6 revisions) (flutter/flutter#125039) 2023-04-18 fluttergithubbot@gmail.com Roll pub packages (flutter/flutter#125005) 2023-04-18 pateltirth454@gmail.com [InputDatePickerFormField] adds acceptEmptyDate to InputDatePickerFormField Widget (flutter/flutter#124143) 2023-04-18 44755140+werainkhatri@users.noreply.github.com relayout active ListWheelScrollView children every performLayout (flutter/flutter#124476) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4a603aaff32e to c4396f9f602f (2 revisions) (flutter/flutter#125007) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 20034a8d62c4 to 4a603aaff32e (2 revisions) (flutter/flutter#125004) 2023-04-18 katelovett@google.com Add optional axis specifier to static scrollable methods (flutter/flutter#124894) 2023-04-17 barpac02@gmail.com Update usage of standalone`pub` executable in flutter_tools testing docs (flutter/flutter#124898) 2023-04-17 warriorharish95668@gmail.com Add Harish Anbalagan to AUTHORS (flutter/flutter#124684) 2023-04-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from b2d07388ceb6 to 20034a8d62c4 (7 revisions) (flutter/flutter#125001) 2023-04-17 36861262+QuncCccccc@users.noreply.github.com Add an example for SearchBar (flutter/flutter#124992) 2023-04-17 47866232+chunhtai@users.noreply.github.com SelectionContainer's listeners can remove itself during listener callâ�¦ (flutter/flutter#124624) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com,ychris@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Remove unused context parameter

7214a9b

justinmc self-assigned this Apr 5, 2023

justinmc requested a review from camsim99 April 5, 2023 19:04

Test

2ecb521

justinmc marked this pull request as ready for review April 12, 2023 22:21

justinmc added 5 commits April 12, 2023 16:50

Fix test

11a69bc

Merge branch 'master' into build-button-items-context

632997a

Merge branch 'master' into build-button-items-context

211c31f

Fix missing context for localization after merge

f6243af

Update test to consider that buildButtonItems now needs a real contex…

74af9ec

…t for localizations

justinmc mentioned this pull request Apr 14, 2023

Show iOS menu on iOS for TextField by default #124895

Merged

camsim99 approved these changes Apr 17, 2023

View reviewed changes

Merge branch 'master' into build-button-items-context

355263e

justinmc merged commit 9d68831 into flutter:master Apr 18, 2023

justinmc deleted the build-button-items-context branch April 18, 2023 17:31

engine-flutter-autoroll mentioned this pull request Apr 18, 2023

Roll Flutter from 15cb1f84d726 to 42fb0b23139c (19 revisions) flutter/packages#3756

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 18, 2023

9d68831 Remove unused context parameter (flutter/flutter#124254)

d992c15

reidbaker mentioned this pull request Apr 20, 2023

i123636 android embedding v2 agp gradle kotlin update #125256

Closed

8 tasks

reidbaker mentioned this pull request Apr 21, 2023

[integration_test] add support for agp8.0 #125310

Closed

8 tasks

reidbaker mentioned this pull request Apr 21, 2023

Test AGP 8.0 using java 17 #125323

Merged

8 tasks

justinmc added a commit to justinmc/flutter that referenced this pull request Apr 25, 2023

Remove unused context parameter (flutter#124254)

674f242

Tidying up the spell check API.

justinmc mentioned this pull request Apr 25, 2023

Cherry Pick Spell Check Fixes #125517

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unused context parameter#124254

Remove unused context parameter#124254
justinmc merged 8 commits intoflutter:masterfrom
justinmc:build-button-items-context

justinmc commented Apr 5, 2023

Uh oh!

camsim99 left a comment

Uh oh!

camsim99 Apr 17, 2023

Uh oh!

camsim99 Apr 17, 2023

Uh oh!

werainkhatri commented Apr 19, 2023

Uh oh!

justinmc commented Apr 20, 2023

Uh oh!

Hixie commented Apr 20, 2023

Uh oh!

justinmc commented Apr 21, 2023

Uh oh!

Hixie commented Apr 21, 2023

Uh oh!

justinmc commented Apr 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

justinmc commented Apr 5, 2023

Uh oh!

camsim99 left a comment

Choose a reason for hiding this comment

Uh oh!

camsim99 Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

camsim99 Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

werainkhatri commented Apr 19, 2023

Uh oh!

justinmc commented Apr 20, 2023

Uh oh!

Hixie commented Apr 20, 2023

Uh oh!

justinmc commented Apr 21, 2023

Uh oh!

Hixie commented Apr 21, 2023

Uh oh!

justinmc commented Apr 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants