migrate rendering to nullsafety

[a14n]

Description

NNBD migration for rendering

Related Issues

• Flutter framework null safety migration dart-lang/sdk#40146

Tests

I added the following tests:

None

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I signed the CLA.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I updated/added relevant documentation (doc comments with ///).
• All existing and new tests are passing.
• The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

• No, no existing tests failed, so this is not a breaking change.
• Yes, this is a breaking change. If not, delete the remainder of this section.
  • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
  • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
  • I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

[flutter-dashboard]

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.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[skia-gold]

Gold has detected about 3 untriaged digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/64456

[skia-gold]

Gold has detected about 3 untriaged digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/64456

[Hixie]

position should be non-nullable

[Hixie]

here i also added:

/// Method signature for hit testing a [RenderBox] with a manually
/// managed position (one that is passed out-of-band).
///
/// Used by [RenderSliverSingleBoxAdapter.hitTestBoxChild] to hit test
/// [RenderBox] children of a [RenderSliver].
///
/// See also:
///
///  * [RenderBox.hitTest], which documents more details around hit testing
///    [RenderBox]es.
typedef BoxHitTestWithOutOfBandPosition = bool Function(BoxHitTestResult result);

[Hixie]

position should be non nullable

[Hixie]

assert position isn't null

[Hixie]

here i think position should be non-nullable

[Hixie]

Suggested change

	final Offset? transformedPosition = position == null || offset == null
	? position
	: position - offset;
	final Offset transformedPosition = offset == null ? position : position - offset;

[Hixie]

non-nullable position here too

[a14n]

But the doc explicitally mentions null:

  /// The `position` argument may be null, which will be forwarded to the
  /// `hitTest` callback as-is. Using null as the position can be useful if
  /// the child speaks a different hit test protocol then the parent and the
  /// position is not required to do the actual hit testing in that protocol.

[Hixie]

Yeah, I'm proposing removing that and replacing it with dedicated methods to handle that case.

[Hixie]

Suggested change

	final Offset? transformedPosition = position == null || transform == null
	? position
	: MatrixUtils.transformPoint(transform, position);
	assert(position != null);
	final Offset transformedPosition = transform == null ?
	position : MatrixUtils.transformPoint(transform, position);

[Hixie]

at the end of this class i added:

  /// Pass-through method for adding a hit test while manually managing
  /// the position transformation logic.
  ///
  /// The actual hit testing of the child needs to be implemented in the
  /// provided `hitTest` callback. The position needs to be handled by
  /// the caller.
  ///
  /// The function returns the return value of the `hitTest` callback.
  ///
  /// A `paintOffset`, `paintTransform`, or `rawTransform` should be
  /// passed to the method to update the hit test stack.
  ///
  ///  * `paintOffset` has the semantics of the `offset` passed to
  ///    [addWithPaintOffset].
  ///
  ///  * `paintTransform` has the semantics of the `transform` passed to
  ///    [addWithPaintTransform], except thit it must be invertible; it
  ///    is the responsibility of the caller to ensure this.
  ///
  ///  * `rawTransform` has the semantics of the `transform` passed to
  ///    [addWithRawTransform].
  ///
  /// Exactly one of these must be non-null.
  ///
  /// See also:
  ///
  ///  * [addWithPaintTransform], which takes a generic paint transform matrix and
  ///    documents the intended usage of this API in more detail.
  bool addWithOutOfBandPosition({
    Offset? paintOffset,
    Matrix4? paintTransform,
    Matrix4? rawTransform,
    required BoxHitTestWithOutOfBandPosition hitTest,
  }) {
    assert(hitTest != null);
    assert((paintOffset == null && paintTransform == null && rawTransform != null) ||
           (paintOffset == null && paintTransform != null && rawTransform == null) ||
           (paintOffset != null && paintTransform == null && rawTransform == null));
    if (paintOffset != null) {
      pushOffset(-paintOffset);
    } else if (rawTransform != null) {
      pushTransform(rawTransform);
    } else {
      assert(paintTransform != null);
      paintTransform = Matrix4.tryInvert(PointerEvent.removePerspectiveTransform(paintTransform!));
      assert(paintTransform != null);
      pushTransform(paintTransform!);
    }
    final bool isHit = hitTest(this);
    popTransform();
    return isHit;
  }

[Hixie]

Suggested change

	// TODO(ianh): cacheExtent/cacheExtentStyle should be a single
	// object that specifies both the scalar value and the unit, not a
	// pair of independent setters. Changing that would allow a more
	// rational API and would let us make the getter non-nullable.

[Hixie]

replace from next line until just before the getter:

  /// If set to [CacheExtentStyle.pixel], the [cacheExtent] will be
  /// treated as a logical pixels, and the default [cacheExtent] is
  /// [RenderAbstractViewport.defaultCacheExtent].
  ///
  /// If set to [CacheExtentStyle.viewport], the [cacheExtent] will be
  /// treated as a multiplier for the main axis extent of the
  /// viewport. In this case there is no default [cacheExtent]; it
  /// must be explicitly specified.
  /// {@endtemplate}
  ///
  /// Changing the [cacheExtentStyle] without also changing the [cacheExtent]
  /// is rarely the correct choice.

[Hixie]

replace from the getter until the assert:

  ///
  /// The getter can never return null, but the field is nullable
  /// because the setter can be set to null to reset the value to
  /// [RenderAbstractViewport.defaultCacheExtent] (in which case
  /// [cacheExtentStyle] must be [CacheExtentStyle.pixel]).
  ///
  /// See also:
  ///
  ///  * [cacheExtentStyle], which controls the units of the [cacheExtent].
  double? get cacheExtent => _cacheExtent;
  double _cacheExtent;
  set cacheExtent(double? value) {
    value ??= RenderAbstractViewport.defaultCacheExtent;

[Hixie]

Suggested change

	applyPaintTransform(child, transform);
	final bool isHit = result.addWithPaintTransform(
	transform: transform,
	position: null, // Manually adapting from box to sliver position below.
	hitTest: (BoxHitTestResult result, Offset _) {
	hitTest: (BoxHitTestResult result, Offset? _) {
	applyPaintTransform(child, transform); // must be invertible
	final bool isHit = result.addWithOutOfBandPosition(
	paintTransform: transform,
	hitTest: (BoxHitTestResult result) {

[Hixie]

+= should work fine here since _maxScrollExtent is late non-nullable

[Hixie]

similar here

[Hixie]

please add this inside the applyPaintTransform method:

    // Hit test logic relies on this always providing an invertible matrix.

[Hixie]

add to applyPaintTransform:

    // Hit test logic relies on this always providing an invertible matrix.

[Hixie]

i think this should be non-nullable. it would simplify a ton of other code.

[Hixie]

In addition to my comments above, the following changes would also be good to do at the same time (some are required by some of the changes I suggested):

https://github.com/flutter/flutter/pull/64476/files#diff-acd2b473dd8ae3b5cab347ba47c55c94R902 until the end of the PR

[Hixie]

I applied the rest of my review comments and uploaded #64621, please feel free to continue from there if there's more work to be done.

[a14n]

Continued in #64621