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

update nullability of drawAtlas methods and flesh out docs #20176

Merged

Conversation

flar
Copy link
Contributor

@flar flar commented Jul 31, 2020

These changes fix two outstanding issues with the Canvas.drawAtlas and Canvas.drawRawAtlas methods:

The changes bring the null-ness of 2 of their parameters into compliance with the Skia implementations and the needs of common uses of these methods. Since nulls will now be allowed where they were not before, the changes represent a relaxing of restrictions and so should be backwards compatible with existing apps that use these methods.

The implementation is complete for the regular engine and includes new tests, but the web_ui version of these classes will need some updates to match (and I expect some check failures on this initial push due to the temporarily mismatched method signatures).

The changes are ready to have their API changes reviewed in their current form, though.

@flar flar removed the request for review from GaryQian July 31, 2020 08:32
@flar
Copy link
Contributor Author

flar commented Jul 31, 2020

This is odd, I haven't updated the web_ui signatures, but I guess the API tests don't test the nullability parts of the signatures.

I still have to update the implementation of the methods in web_ui to match the changes in the lib/ui files, though.

@flar flar requested review from yjbanov, dnfield and Hixie July 31, 2020 21:16
@flar
Copy link
Contributor Author

flar commented Jul 31, 2020

The web_ui files are all updated as far as I could understand them. I pasted in the new lengthy doc comments even though I understand that the doc comments in the web_ui api file are not actually used to generate any documents...?

@dnfield
Copy link
Contributor

dnfield commented Jul 31, 2020

I haven't double checked but isn't the C++ side expecting these to not be Dart_Null? I really like the doc updates but it's not clear to me why we would start accepting null instead of an empty array. I think in general it makes the code more complex and if you forget the null check somewhere you crash.

@flar
Copy link
Contributor Author

flar commented Jul 31, 2020

I haven't double checked but isn't the C++ side expecting these to not be Dart_Null? I really like the doc updates but it's not clear to me why we would start accepting null instead of an empty array. I think in general it makes the code more complex and if you forget the null check somewhere you crash.

I looked through and it appears that they handle null lists being sent down in the arg converters. I am certainly running a test case where I send down nulls and it behaves as expected.

@flar
Copy link
Contributor Author

flar commented Jul 31, 2020

Tagging @chinmaygarde regarding the question about whether the nulls will be handled correctly by the DartNative argument converters. Since an empty colors array used to translate into a null value being sent down, I think this is mostly a moot question, but it would be good to verify that my null handling here is kosher with respect to the native code.

@jason-simmons
Copy link
Member

The Tonic wrapper for typed lists such as Int32List will construct an empty list if the converted Dart handle is a Dart null.

See https://github.com/flutter/engine/blob/master/third_party/tonic/typed_data/typed_list.cc#L20

@flar
Copy link
Contributor Author

flar commented Jul 31, 2020

The Tonic wrapper for typed lists such as Int32List will construct an empty list if the converted Dart handle is a Dart null.

See https://github.com/flutter/engine/blob/master/third_party/tonic/typed_data/typed_list.cc#L20

It looks like the data_ field, which is what the value of the argument is, will be nullptr in that case.

See https://github.com/flutter/engine/blob/master/third_party/tonic/typed_data/typed_list.cc#L19
and https://github.com/flutter/engine/blob/master/third_party/tonic/typed_data/typed_list.h#L40
and https://github.com/flutter/engine/blob/master/lib/ui/painting/canvas.cc#L462

@flar
Copy link
Contributor Author

flar commented Jul 31, 2020

Does this need to go through any formal API review?

@dnfield
Copy link
Contributor

dnfield commented Jul 31, 2020

Nah. If you're concerned though feel free to ping @Hixie or @zanderso

@flar flar added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 1, 2020
@fluttergithubbot fluttergithubbot merged commit 22fb58b into flutter:master Aug 1, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 3, 2020
zanderso pushed a commit to flutter/flutter that referenced this pull request Aug 3, 2020
* 7f5d044 Wait before switching surfaces (flutter/engine#20100)

* 5513273 Reland: Avoid a copy in EncodeImage (flutter/engine#20003)

* 1efdd95 Roll Dart SDK from bd528bfbd69d to ea6bde577d1c (19 revisions) (flutter/engine#20172)

* 3b0e697 Roll Skia from 8cc118dce813 to c3794dd52778 (27 revisions) (flutter/engine#20173)

* cb1a374 Roll Fuchsia Mac SDK from T2xc0OuiK... to i0zTcQ8Qb... (flutter/engine#20175)

* ed36b1a Roll Skia from c3794dd52778 to 2d01ed94605a (10 revisions) (flutter/engine#20179)

* fcc1eaf Fix iOS Keyboard stuck as UIKeyboardTypeNamePhonePad (flutter/engine#20181)

* 9c6837c Roll Skia from 2d01ed94605a to 7225788b9070 (6 revisions) (flutter/engine#20183)

* 13e993e Fix Typos (flutter/engine#19691)

* 98cfd1d Move platform specific information to `PlatformConfiguration` class (flutter/engine#19652)

* 22fb58b update nullability of drawAtlas methods and flesh out docs (flutter/engine#20176)

* bcc43df Roll Dart SDK from ea6bde577d1c to 033a81d924b9 (23 revisions) (flutter/engine#20186)

* cb4bb93 [web] increase number of shards. sync engine web tests same as flutter repo (flutter/engine#20164)

* d986b8d Enable linting in several files (flutter/engine#20134)

* 7dd092d Enable more linting (flutter/engine#20187)

* 3cc86ac Roll Dart SDK from 033a81d924b9 to ad5bcf16f1c8 (9 revisions) (flutter/engine#20191)

* 5ca8a2a Roll Dart SDK from ad5bcf16f1c8 to d169af6f7d8f (1 revision) (flutter/engine#20192)

* 4de0c04 Roll Dart SDK from d169af6f7d8f to 7e6c55e3aaf5 (1 revision) (flutter/engine#20196)

* 908fe01 Fix navigation message relay. (flutter/engine#20193)

* f1b3b69 Roll Dart SDK from 7e6c55e3aaf5 to 365525432a70 (2 revisions) (flutter/engine#20197)

* 8fbdd3f Fix parameter names

* 083282e Fix Implments typo
Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request Aug 11, 2020
* 7f5d044 Wait before switching surfaces (flutter/engine#20100)

* 5513273 Reland: Avoid a copy in EncodeImage (flutter/engine#20003)

* 1efdd95 Roll Dart SDK from bd528bfbd69d to ea6bde577d1c (19 revisions) (flutter/engine#20172)

* 3b0e697 Roll Skia from 8cc118dce813 to c3794dd52778 (27 revisions) (flutter/engine#20173)

* cb1a374 Roll Fuchsia Mac SDK from T2xc0OuiK... to i0zTcQ8Qb... (flutter/engine#20175)

* ed36b1a Roll Skia from c3794dd52778 to 2d01ed94605a (10 revisions) (flutter/engine#20179)

* fcc1eaf Fix iOS Keyboard stuck as UIKeyboardTypeNamePhonePad (flutter/engine#20181)

* 9c6837c Roll Skia from 2d01ed94605a to 7225788b9070 (6 revisions) (flutter/engine#20183)

* 13e993e Fix Typos (flutter/engine#19691)

* 98cfd1d Move platform specific information to `PlatformConfiguration` class (flutter/engine#19652)

* 22fb58b update nullability of drawAtlas methods and flesh out docs (flutter/engine#20176)

* bcc43df Roll Dart SDK from ea6bde577d1c to 033a81d924b9 (23 revisions) (flutter/engine#20186)

* cb4bb93 [web] increase number of shards. sync engine web tests same as flutter repo (flutter/engine#20164)

* d986b8d Enable linting in several files (flutter/engine#20134)

* 7dd092d Enable more linting (flutter/engine#20187)

* 3cc86ac Roll Dart SDK from 033a81d924b9 to ad5bcf16f1c8 (9 revisions) (flutter/engine#20191)

* 5ca8a2a Roll Dart SDK from ad5bcf16f1c8 to d169af6f7d8f (1 revision) (flutter/engine#20192)

* 4de0c04 Roll Dart SDK from d169af6f7d8f to 7e6c55e3aaf5 (1 revision) (flutter/engine#20196)

* 908fe01 Fix navigation message relay. (flutter/engine#20193)

* f1b3b69 Roll Dart SDK from 7e6c55e3aaf5 to 365525432a70 (2 revisions) (flutter/engine#20197)

* 8fbdd3f Fix parameter names

* 083282e Fix Implments typo
@flar flar deleted the update-drawatlas-nullability-and-docs branch September 4, 2020 19:11
@flar flar restored the update-drawatlas-nullability-and-docs branch September 4, 2020 19:12
@flar flar deleted the update-drawatlas-nullability-and-docs branch September 4, 2020 19:13
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
* 7f5d044 Wait before switching surfaces (flutter/engine#20100)

* 5513273 Reland: Avoid a copy in EncodeImage (flutter/engine#20003)

* 1efdd95 Roll Dart SDK from bd528bfbd69d to ea6bde577d1c (19 revisions) (flutter/engine#20172)

* 3b0e697 Roll Skia from 8cc118dce813 to c3794dd52778 (27 revisions) (flutter/engine#20173)

* cb1a374 Roll Fuchsia Mac SDK from T2xc0OuiK... to i0zTcQ8Qb... (flutter/engine#20175)

* ed36b1a Roll Skia from c3794dd52778 to 2d01ed94605a (10 revisions) (flutter/engine#20179)

* fcc1eaf Fix iOS Keyboard stuck as UIKeyboardTypeNamePhonePad (flutter/engine#20181)

* 9c6837c Roll Skia from 2d01ed94605a to 7225788b9070 (6 revisions) (flutter/engine#20183)

* 13e993e Fix Typos (flutter/engine#19691)

* 98cfd1d Move platform specific information to `PlatformConfiguration` class (flutter/engine#19652)

* 22fb58b update nullability of drawAtlas methods and flesh out docs (flutter/engine#20176)

* bcc43df Roll Dart SDK from ea6bde577d1c to 033a81d924b9 (23 revisions) (flutter/engine#20186)

* cb4bb93 [web] increase number of shards. sync engine web tests same as flutter repo (flutter/engine#20164)

* d986b8d Enable linting in several files (flutter/engine#20134)

* 7dd092d Enable more linting (flutter/engine#20187)

* 3cc86ac Roll Dart SDK from 033a81d924b9 to ad5bcf16f1c8 (9 revisions) (flutter/engine#20191)

* 5ca8a2a Roll Dart SDK from ad5bcf16f1c8 to d169af6f7d8f (1 revision) (flutter/engine#20192)

* 4de0c04 Roll Dart SDK from d169af6f7d8f to 7e6c55e3aaf5 (1 revision) (flutter/engine#20196)

* 908fe01 Fix navigation message relay. (flutter/engine#20193)

* f1b3b69 Roll Dart SDK from 7e6c55e3aaf5 to 365525432a70 (2 revisions) (flutter/engine#20197)

* 8fbdd3f Fix parameter names

* 083282e Fix Implments typo
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects: docs cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants