nit: orphan (

So, the plan is that for multi view we will extend the signature of the MouseTrackerRenderViewHitTest here and have it take viewId as an argument as well?

Yes. And hitTest probably needs a new viewId parameter as well, as in your prototype framework PR (or in other less breaking ways.)

for my own understanding: In the old code we were checking event.down to determine whether to use a cached hitTestResult, in the new code we are using event is PointerMoveEvent - why this change? Are they equivalent?

I agree. But the current state is:

• At the outside (_handlePointerEventImmediately, here) the code is checking "is down or is PanZoomUpdateEvent, after eliminating all the other classes".
• At the inside, the code is checking "event is MoveEvent" (we definitely want to include PanZoomUpdateEvent as well).
• We definitely would like them to agree with each other.

So first, to answer your question directly (which is a valid question), the event when down is true after eliminating PointerDownEvent can only mean PointerMoveEvent. Second, even if it's not, we'd like the two conditions to agree with each other.

Second, even if it's not, we'd like the two conditions to agree with each other.

Makes sense, but this was now reverted?

Yeah, I think the fix might be better off in a separate PR.

This is kinda funny: when useCachedHitTestResult returns true, we DON'T use the cached hit test result... wondering if we maybe need a better name for useCachedHitTestResult.

Also wondering: Why is it not just up to the mouseTracker to decide whether it wants to re-use the cached hitTestResult or create a new one based on the event it receives? It basically already does this for PointerRemovedEvent where the provided hitTestResult is silently replaced. Why not do this for all events and basically document on updateWithEvent that if available it takes the hitTestResult performed when the pointer associated with the provided event first went down. This is an optimization; the MouseTracker may ignore that result if necessary and perform its own hit testing.

I don't think it's up to the MouseTracker to decide, because the MouseTracker has no idea how the hit test result it received was generated.

I have a better idea: let the GestureBinding provide a method that always return the uncached hit test. Let me do this right now.

I don't think it's up to the MouseTracker to decide, because the MouseTracker has no idea how the hit test result it received was generated.

I was thinking, we could just define that the MouseTracker always wants the hitTestResult that was obtained when the pointer associated with the event first went down (this is the only result we cache in _hitTests anyways, right?), if such a result is available, or null, if it is not. If you pass in any other hitTestResult, you're in violation of the API contract. Then, the mouse tracker can decide if it wants to use that cached hitTestResult or not, depending on the current event type.

The MouseTracker just doesn't want any cached result. It always needs the hit test result performed at this frame. The current plan (before the revert) is that the MouseTracker always receive the fresh result if available or null if not.

It took me a good amount of time to understand the purpose and semantics of useCachedHitTestResult. Among other things, it's kinda confusing that - as the comment below admits - it's not actually telling you about all caching instances. It seems to be very much tailored to the needs of the MouseTracker instead of being a general purpose API. Another reason for maybe considering to have the MouseTracker internally decide whether to reuse the hitTestResult from the pointer down (see my other comment on this).

I don't think it's up to the MouseTracker to decide, because the MouseTracker has no idea how the hit test result it received was generated.

I was thinking, we could just define that the MouseTracker always wants the hitTestResult that was obtained when the pointer associated with the event first went down (this is the only result we cache in _hitTests anyways, right?), if such a result is available, or null, if it is not. If you pass in any other hitTestResult, you're in violation of the API contract. Then, the mouse tracker can decide if it wants to use that cached hitTestResult or not, depending on the current event type.

Second, even if it's not, we'd like the two conditions to agree with each other.

Makes sense, but this was now reverted?

nit: for consistency lets switch the parameter order around since hitTestInView puts position first followed by viewId.

Done.

nit: let's remove the "RenderView" from this name. Technically, the MouseTracker doesn't care what the hit test is performed on. It only wants to know what is hit at a particular offset in a particular view. Whether that view is backed by a RenderView is not relevant.

Maybe just call this MouseTrackerHitTest ?

Similarly, "render view" should be removed from the doc and and the property below.

Done.

Can you revert this back to just checking that the provided viewId matches the FlutterView associated with the RenderView? It's not necessary in this change to make an assumption about how the RenderView will be retrieved based on the viewId (the actual implementation will actually use a map here instead of a function) - and until that's implemented it just makes this code look like it implements a functionality that's just not there yet.

You could move the TODO from above to here, though, since this is going to be the assert that fires when we add multi view support and forget to update that. Having a TODO with some specific instructions here would be helpful then.

It's either extracting it to a method _viewIdToRenderView or inlining the function here, because now that hitTestInView has a viewId, I have to check whether this ID is the implicit view ID, and skip the hit testing if the view is not the implicit view. I'm leaning to keeping the current structure, and when you implement it with a map you can change the line to final RenderView targetView = _viewIdToRenderView[viewId!];. What do you think?

There is currently no way this can give you anything but that ID, right? So I would argue that just firing an assert here is the right thing to do if the viewID is not the expected (and only valid) one. In the current implementation it would actually give you a null-dereferencing error on line 542 if we were to receive an invalid ID, which I think is worse then firing an assert clearly stating what the current expected behavior is.

You're right! I'll do it.