8000 Some widgets' nullOk parameters are inconsistently applied. · Issue #68637 · flutter/flutter · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Some widgets' nullOk parameters are inconsistently applied. #68637

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.

Sign up for GitHub

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

Jump to bottom
Closed
shyndman opened this issue Oct 20, 2020 · 18 comments · Fixed by #74657 or #74680
Closed

Some widgets' nullOk parameters are inconsistently applied. #68637

shyndman opened this issue Oct 20, 2020 · 18 comments · Fixed by #74657 or #74680
Assignees
@gspencergoog
Labels
a: error message Error messages from the Flutter framework d: api docs Issues with https://api.flutter.dev/ framework flutter/packages/flutter repository. See also f: labels. P1 High-priority issues at the top of the work list
Milestone
February Beta Rel...

Comments

@shyndman
Copy link
Contributor

The Actions widget has a number of static methods with a nullOk parameter, which can be used to enable or disable the throwing of errors when the caller's requested object was not found.

Turns out, this parameter is only considered when asserts are enabled. This feels a little iffy to me, as the documentation makes no mention of how the contract can change depending on environment.

Request: Improve docs (and maybe the name of the param, since it's sort of a debug only thing), or alternatively normalize behavior between asserts-enabled and disabled builds.

Affected methods:

  • Actions.find
  • Actions.of
  • Actions.handler
  • Actions.invoke

Thanks!

@gspencergoog
@gspencergoog
Copy link
Contributor

The name of the parameter is consistent across many different similar static methods, not just Actions, and has the same behavior there (e.g. MediaQuery.of, Focus.of, Localizations.localeOf, etc.). And yes, it only applies to builds with assertions on, but that was always its intent.

I agree that the behavior should be mentioned more explicitly in the docs.

@gspencergoog gspencergoog added d: api docs Issues with https://api.flutter.dev/ framework flutter/packages/flutter repository. See also f: labels. labels Oct 20, 2020
@shyndman
Copy link
Contributor Author
shyndman commented Oct 20, 2020 •
edited
Loading

I took a look at all of those, and it appears they are designed to throw regardless of whether asserts are enabled (although Localizations.localeOf might throw an NPE as opposed to a well-formed FlutterError).

@gspencergoog
Copy link
Contributor

Ahh, I see what you mean. OK, I can see making those throw a FlutterError on a non-assert build.

We seem to actually be quite inconsistent with our methods that take nullOk as a parameter, perhaps we should try and normalize that across the framework.

@shyndman
Copy link
Contributor Author

Couldn't hurt. :)

Thanks!

@gspencergoog gspencergoog added the a: error message Error messages from the Flutter framework label Oct 20, 2020
8000 @gspencergoog gspencergoog changed the title Actions widget's nullOk parameters only considered with asserts enabled Some widgets' nullOk parameters are inconsistently applied. Oct 20, 2020
@gspencergoog
Copy link
Contributor

I'm converting this issue over to cover all the static methods that take a nullOk parameter.

We have a bunch of static member functions that have a nullOk argument to them, but we're really inconsistent with how we apply it. Sometimes it will return null in some circumstances even if nullOk is false (e.g. CupertinoDynamicColor.resolve()), sometimes it only works when asserts are enabled (e.g. Actions.find), and other times it throws a FlutterError in release mode (e.g. MediaQuery.of()). Sometimes I think the checks are assert-only because of performance, though, so we can't just make them all throw FlutterError all the time. At the least, we should probably update the API docs for each one that describes what behavior is expected.

@shyndman
Copy link
Contributor Author

Thanks Greg.

When the time comes, I'd love to be CC'd on that PR if you get a chance.

@gspencergoog
Copy link
Contributor

Sure, will do.

@gspencergoog
Copy link
Contributor

Related: #67163

@gspencergoog
Copy link
Contributor

Here's a design doc for doing this: https://flutter.dev/go/eliminating-nullok-paramters (I know, I'm fixing the name typo in the go link).

@gspencergoog gspencergoog mentioned this issue Oct 23, 2020
Merged
2 tasks
@gspencergoog gspencergoog added the P1 High-priority issues at the top of the work list label Oct 23, 2020
@gspencergoog gspencergoog self-assigned this Oct 23, 2020
@gspencergoog gspencergoog added this to the 1.27 - January 2021 milestone Oct 23, 2020
@gspencergoog gspencergoog mentioned this issue Oct 23, 2020
Merged
4 tasks
@gspencergoog
Copy link
Contributor

FYI, in discussions about this, @Hixie was of the opinion (and I agree) that eventually the FlutteError exceptions here should be thrown only from inside of asserts, since we eventually want to remove debug messages from release builds for size reasons (and because they're not really actionable there anyhow).

This was referenced Oct 23, 2020
Merged
Merged
Merged
Merged
Merged
Merged
@shyndman
Copy link
Contributor Author

Makes sense. Would exceptions without messages be thrown in the non-assert case?

@gspencergoog
Copy link
Contributor

@shyndman Yes, and I added wording to that effect in the docs.

@gspencergoog gspencergoog mentioned this issue Nov 17, 2020
Merged
2 tasks
@gspencergoog
Copy link
Contributor

This is all done, except for Localizations.localeOf, since a client (flutter_svg) is still using it. Once dnfield/flutter_svg#443 lands, and then we remove the nullOk from Localizations.localeOf, this issue can be closed.

@shyndman
Copy link
Contributor Author
shyndman commented Dec 4, 2020

Thanks!

It's one of those things where a few people will never realize the thanks they owe — the bug they didn't have to track down. :)

@gspencergoog gspencergoog mentioned this issue Jan 25, 2021
Merged
4 tasks
@goderbauer goderbauer mentioned this issue Jan 25, 2021
Merged
@goderbauer
Copy link
Member

There's one more nullOK param remaining after all changes linked from this bug are submitted: https://master-api.flutter.dev/flutter/widgets/Actions/invoke.html

@gspencergoog gspencergoog reopened this Jan 25, 2021
@gspencergoog
Copy link
Contributor

I'll fix the Actions.invoke nullOk before closing.

@gspencergoog gspencergoog mentioned this issue Jan 25, 2021
Merged
4 tasks
@shyndman
Copy link
Contributor Author

Thanks Greg

@github-actions
Copy link
github-actions bot commented Aug 6, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@gspencergoog gspencergoog

Labels
a: error message Error messages from the Flutter framework d: api docs Issues with https://api.flutter.dev/ framework flutter/packages/flutter repository. See also f: labels. P1 High-priority issues at the top of the work list
Projects
None yet
Milestone
February Beta Release (1.27)
4 participants
@shyndman @goderbauer @gspencergoog @pedromassangocode
0