CustomPainter.hitTest missing size argument

CustomPainter.hitTest is missing the size of the widget, making it impossible to specify custom hit test behaviour when paint uses its size argument.

Simplistic example:

class CirclePainter extends CustomPainter {
  @override
  void paint(Canvas canvas, Size size) {
    final center = size.center(Offset.zero);
    final borderPaint = Paint()
      ..color = const Color(0xffffffff)
      ..style = PaintingStyle.stroke
      ..strokeWidth = 2;
    canvas.drawCircle(center, size.width/2, borderPaint);
  }

  @override
  bool hitTest(Offset position) {
    final size = ???;
    final center = size.center(Offset.zero);
    return (position - center).distance < 0.5;
  }

  @override
  bool shouldRepaint(CustomPainter oldDelegate) => false;
}

A workaround would be to have paint save the size in a field, but that only works if the painter is used for a single widget only.

Is it acceptable to add a size argument to hitTest?

[✓] Flutter (Channel unknown, v1.2.2-pre.22, on Mac OS X 10.14 18A391, locale en-NL)
    • Flutter version 1.2.2-pre.22 at /Users/sander/Development/flutter
    • Framework revision dd23be3936 (9 hours ago), 2019-02-19 21:35:31 -0800
    • Engine revision f45572e95f
    • Dart version 2.1.2 (build 2.1.2-dev.0.0 c92d5ca288)

Please add the output of flutter doctor -v.

@dnfield

CustomPainter.hitTest should indeed take a Size argument. That was an oversight to not provide one. I added this to the list in #24722.

Since this is a breaking change we need to carefully think about how we fix this as we do not want to annoy our users with constant breaking changes that they have to apply to their code base manually. I believe we have three options here:

Make the breaking change and add the additional argument. This will require all people subclassing CustomPainter to update their subclass. Luckily, this change can be automated and in order to assist our users with breaking changes we would like to provide them with some kind of a tool that automatically upgrades their code base. We need to figure out what infrastructure we need for that and build that for this option to work. This would leave us with a nice and concise API.
Convince the Dart language team that methods in subclasses do not have to take all the arguments specified in the superclass. This option would make your proposed change a non-breaking change because we can add Size to the CustomPainter without requiring any changes to subclasses. This option will also make future changes of this type easier (I'd anticipate we will have more like this...). I've filed Allow adding method arguments to superclass without breaking subclass dart-lang/sdk#36027 to discuss this with the language team. Feel free to add your thoughts there.
We make this change non-breaking by adding a new method. This method would take an Offset and Size. By default, it would just call the existing hitTest with just the Offset ensuring that existing implementations continue to work. Subclasses, who want access to Size can override this new method instead. This option would require that we can come up with a sensible name for this new method, that blends in with the general API of CustomPainter and doesn't look like it was just an afterthought. So far, I haven't found a great name...

/cc @dnfield @Hixie

My own preference would be for option 3. I think we could then mark hitTest as @deprecated to encourage people to override the new method.

The problem is what would you call it. hitTest is really the only reasonable name for this method.

sizedHitTest?

If we can get the language change that would be great. My concern with a breaking change on this is that it seems likely to break packages that will be challenging to fix for consumers - not to mention creating a need to fork the package for any further fixes.

Another option is to make CustomPaint generic and test that it takes the current interface or the new one...

@Hixie

@Hixie @dnfield Have you considered either a separate interface or an arity test?

abstract class CustomPainter {
  void hitTest(Offset offset);
}

class Offset {}

abstract class CustomPainterWithSize implements CustomPainter {
  @override
  void hitTest(Offset offset, {Size size});
}

class Size {}

void example(CustomPainter p) {
  // Idea 1. Check for "CustomPainterWithSize".
  if (p is CustomPainterWithSize) {
    // ...
  }
  
  // Idea 2. Check for arity.
  final hitTest = p.hitTest;
  if (hitTest is void Function(Offset, {Size size})) {
    // ... 
  }
}

/cc @jonahwilliams

There are only two places where we actually need to call the CustomPainter.hitTest - both in RenderCustomPaint

It feels like adding another interface would add unnecessary complexity. Now as a user you'd have to decide do I just want to subclass a regular CustomPainter or do I care about Size and what does size mean here? You'd have to make that decision even if you don't need to implement hitTest, so this would affect even more users.

I'd probably prefer adding a new method to the existing interface over that because that keeps the decision "do I care about size" local to when you need to override hitTest and doesn't require you to think about it when you subclass the custom painter in the first place...

Wasn't the idea of the new interface to also deprecate the old one? In that case there is no choice for the user.

Doing the deprecation on the interface level would affect every user who is subclassing CustomPainter. Doing it on the method level would only effect people overriding hitTest, which I suspect would be fewer.

I suspect something like this will be our cleanest, least breaking option (that doesn't require a language change):

abstract class CustomPainter {
  ...

  @Deprecated('use hitTestForBox')
  bool hitTest(Offset offset) => null;
  bool hitTestForBox(Offset offset, Size boxSize) => hitTest(offset); // ignore: deprecated_member
}

And update all callsites in the framework to use hitTestForBox. I'm choosing that name to preserve discoverability (when you start typing hitTest... in an IDE, and because we document the size parameter on paint as: "The area of the box is the size of the size argument").

Make the breaking change and add the additional argument

Alternatively a new type extending Offset could be added. (I called it OffsetInBox. The name is obviously open for debate.)
From this two new options rise:

CustomPainters hitTest method takes a OffsetInBox. (I prefer this option)
- Developers overwriting hitTest don't have to change their implementation. ✅
- Developers calling hitTest on an CustomBuilder (this includes super.hitTest) have to supply a OffsetInBox 💥 (But I'm not sure if anyone does that.)
- Clean API ✅
CustomPainters hitTest still takes a Offset, but RenderCustomPaint always calls it with an OffsetInBox.
- Developers overwriting hitTest don't have to change their implementation. ✅
- Developers calling hitTest on an CustomBuilder don't have to change their implementation. ✅
- Developers overwriting hitTest and need the size have to check if the offset is an OffsetInBox. 😐
- Less clean API 😐

A minimal example for an OffsetInBox implementation.

class OffsetInBox extends Offset {
  const OffsetInBox({
    required double dx,
    required double dy,
    required this.height,
    required this.width,
  }) : super(dx, dy);

  final double height;
  final double width;
}

In case there is positive feedback, I'll provide a PR.

@Piinks

I think the right solution here is just for us to add a Size size parameter to hitTest, combined with same automated migration. If dart fix can do it then that's the obvious path, now that we have that tool (it didn't exist when this bug was originally filed). I'm not sure if dart fix can add a new parameter to an overridden method like this? (cc @Piinks)

I'm not sure if dart fix can add a new parameter to an overridden method like this?

I think it can! If it is a change that needs to be made to the base and all subclasses, they each need to have an individual fix written.

I'm not sure I understand if that means it's possible or not. :-) The situation here is we would need every subclass of CustomPainter (in user code) to have the hitTest method gain a new argument.

I've looked at the dart fix documentation, and while I did not try it yet, I think it's possible.

I'm not sure I understand if that means it's possible or not.

Apologies for not being clear. I don't know.

I think it should work. You can alter a method on a given class using the method: key, and the change you would want to make is addParameter. While there may not be an existing fix rule that matches this exact use case, we have many similar cases if you search for those keywords in flutter/packages/flutter/lib/fix_data. As the only current customer for dart fix, we are still finding cases that do not work or do not have full support. If that be the case, we can file an issue with the dart team. Happy to assist on a PR.

I just did some testing (here). And it seems like this is (still) locked by dart-lang/sdk#36027. (Didn't knew about that issue.)

dart fix --compare-to-golden

Building flutter tool...
Computing fixes in rendering... 4.6s
Failed when applying fixes to flutter-dev/packages/flutter/test_fixes/rendering/rendering.dart
Expected:

// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/rendering.dart';

void main() {
  // TODO: Add the pull request id
  // Changes made in https://github.com/flutter/flutter/pull/
  CustomPainter painter;
  painter.hitTest(position, size);
  _CustomPainter painter2;
  painter2.hitTest(position, size);

  // Changes made in https://github.com/flutter/flutter/pull/66305
  RenderStack renderStack = RenderStack();
  renderStack = RenderStack(clipBehavior: Clip.none);
  renderStack = RenderStack(clipBehavior: Clip.hardEdge);
  renderStack = RenderStack(error: '');
  renderStack.clipBehavior;

  // Changes made in https://flutter.dev/docs/release/breaking-changes/clip-behavior
  RenderListWheelViewport renderListWheelViewport = RenderListWheelViewport();
  renderListWheelViewport = RenderListWheelViewport(clipBehavior: Clip.hardEdge);
  renderListWheelViewport = RenderListWheelViewport(clipBehavior: Clip.none);
  renderListWheelViewport = RenderListWheelViewport(error: '');
  renderListWheelViewport.clipBehavior;
}

// TODO: Add the pull request id
// Changes made in https://github.com/flutter/flutter/pull/
class _CustomPainter extends CustomPainter {
  @override
  bool? hitTest(Offset position, Size size) => super.hitTest(position, size);
}

Actual:

// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/rendering.dart';

void main() {
  // TODO: Add the pull request id
  // Changes made in https://github.com/flutter/flutter/pull/
  CustomPainter painter;
  painter.hitTest(position, size);
  _CustomPainter painter2;
  painter2.hitTest(position);

  // Changes made in https://github.com/flutter/flutter/pull/66305
  RenderStack renderStack = RenderStack();
  renderStack = RenderStack(clipBehavior: Clip.none);
  renderStack = RenderStack(clipBehavior: Clip.hardEdge);
  renderStack = RenderStack(error: '');
  renderStack.clipBehavior;

  // Changes made in https://flutter.dev/docs/release/breaking-changes/clip-behavior
  RenderListWheelViewport renderListWheelViewport = RenderListWheelViewport();
  renderListWheelViewport = RenderListWheelViewport(clipBehavior: Clip.hardEdge);
  renderListWheelViewport = RenderListWheelViewport(clipBehavior: Clip.none);
  renderListWheelViewport = RenderListWheelViewport(error: '');
  renderListWheelViewport.clipBehavior;
}

// TODO: Add the pull request id
// Changes made in https://github.com/flutter/flutter/pull/
class _CustomPainter extends CustomPainter {
  @override
  bool? hitTest(Offset position) => super.hitTest(position);
}
Passed: 0, Failed: 1

I might have done something wrong, but since size was added when calling CustomPainter.hitTest, I just think adding arguments to superclasses is indeed not handled at the moment.

@goderbauer

CustomPainter.hitTest should indeed take a Size argument. That was an oversight to not provide one. I added this to the list in #24722.

@goderbauer i feel like its massive oversight for hitTest to not have gesture information. maybe im missing something but when im using an interactiveviewer with custompainter, panning will trigger hit test method, whereas i only need it to trigger on TAP.

think a google maps like experience. im painting items on top of a map. how do i interact with them? how do i paint 200 items performantly?

Navigation Menu

Repository navigation

CustomPainter.hitTest missing size argument #28206

26 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

CustomPainter.hitTest missing size argument #28206

Description

Activity

zoechi commented on Feb 20, 2019

goderbauer commented on Feb 26, 2019

dnfield commented on Feb 26, 2019

Hixie commented on Feb 26, 2019

dnfield commented on Feb 26, 2019

matanlurey commented on Feb 26, 2019

jonahwilliams commented on Feb 26, 2019

goderbauer commented on Feb 26, 2019

spkersten commented on Feb 26, 2019

goderbauer commented on Feb 26, 2019

dnfield commented on Feb 26, 2019

26 remaining items

fischerscode commented on Nov 18, 2022

Hixie commented on Nov 18, 2022

Piinks commented on Nov 18, 2022

Hixie commented on Nov 20, 2022

fischerscode commented on Nov 21, 2022

Piinks commented on Nov 21, 2022

fischerscode commented on Nov 22, 2022

definev commented on Sep 20, 2023

benthillerkus commented on Sep 25, 2024

Piinks commented on Sep 25, 2024

colbymaloy commented on Nov 27, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions