Eliminate some regressions introduced by the new lookup code #85484
Conversation
flutter-dashboard
bot
added
a: accessibility
| @@ -3307,8 +3308,9 @@ class SemanticsConfiguration { | |||
| /// This is a request to decrease the value represented by the widget. For | |||
| /// example, this action might be recognized by a slider control. | |||
| /// | |||
| /// If a [value] is set, [decreasedValue] must also be provided and | |||
| /// [onDecrease] must ensure that [value] will be set to [decreasedValue]. | |||
| /// If [this.value] is set, [decreasedValue] must also be provided and | |||
There was a problem hiding this comment.
What will be printed in the HTML docs if we use [this.value]? Will it literally say this.value, or will it just say value? Because just considering the output prose, it's pretty clear that "value" refers to "this.value" to the reader, so it's kind of redundant to say it that way.
There was a problem hiding this comment.
This is a good idea, filed as dart-lang/dartdoc#2713. It will currently display the this.
| /// A Flutter Driver finder that finds widgets by [text] inside a [Text] or | ||
| /// [EditableText] widget. | ||
| /// A Flutter Driver finder that finds widgets by [text] inside a | ||
| /// [widgets.Text] or [widgets.EditableText] widget. |
There was a problem hiding this comment.
Will [widgets.Text] print widgets.Text, or just Text?
There was a problem hiding this comment.
It will display the library, but I think dart-lang/dartdoc#2713 is a good idea to stop doing that.
More broadly, we could consider having some sort of library or package specific specifier indicating how to resolve ambiguous links. I'm less sure what the right solution there is, though.
jcollins-g
changed the title
This includes some major internal changes that should improve performance (the AOT template compiler) and the new lookup code. The big changes noticeable for Flutter will be resolution of field formal parameters, extension method support, and more consistent disambiguation in comment reference lookups. While a vast net improvement, this PR will change a few links to point to the wrong place. #85484 will address that after this lands, as there was no good way to specify what the user wanted unambiguously before dartdoc 1.0.0 in a few cases. That PR includes more details on the introduced regressions and link changes.
|
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. 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. |
|
test-exempt: only affects comments |
This could not land until after #85549 as
thisreferences are not consistently allowed in the legacy lookup code.While in general, the new lookup code is an improvement to resolution where it is different from the legacy code, in some cases this improvement leads to a change in how ambiguous references were resolved. This PR corrects those cases, reducing the change in resolved variables to 37 from ~50 and leaving only 1 (the
Navigatorreference, below) that is definitely not an improvement, caused by dart-lang/dartdoc#2693. The changes aroundWidgetBuilderare necessary to work around a different bug, dart-lang/dartdoc#2711.Since (after this PR) there are 36 changes with the new lookup code that are either an improvement or ambiguous, one regression, and 1335 newly resolved references not previously recognized before (including field formal parameters!) I still think it is a win.
The list of differences in resolved items can be seen here (and generated yourself with
--warnings reference-lookup-missing-with-new,reference-lookup-not-found-with-new --show-statsafter dartdoc 1.0.0). You can add "reference-lookup-found-with-new" to get a dump of all newly recognized references, but I didn't do that here for space reasons.