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

Scrollwheel #6228

Closed
wants to merge 10 commits into from
Closed

Scrollwheel #6228

wants to merge 10 commits into from

Conversation

jslavitz
Copy link
Contributor

Engine fix for #16235

@@ -428,6 +432,8 @@ private void addPointerForIndex(MotionEvent event, int pointerIndex, ByteBuffer

int pointerKind = getPointerDeviceTypeForToolType(event.getToolType(pointerIndex));

// This is ignored for non-gesture deviced kinds.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what you mean by this

final double scrollDeltaY;

/// A final value that is passed so that we are passing more than 21 values in the packet
final double sentinal;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way you could keep this field private?

tilt: packet.getFloat64(kStride * offset++, _kFakeHostEndian),
scrollDeltaX: packet.getFloat64(kStride * offset++, _kFakeHostEndian),
scrollDeltaY: packet.getFloat64(kStride * offset++, _kFakeHostEndian),
sentinal: packet.getFloat64(kStride * offset++, _kFakeHostEndian),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error happens when reading from the packet right? do you need to explicitly pass it or can you just ignore the last float?

@@ -34,6 +34,9 @@ enum PointerChange {

/// The pointer has stopped making contact with the device.
up,

/// The pointer has scrolled
scroll,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this back? This is incompatible with the second part of the change I'm working on, to support trackpad gesture scrolling.

My intent is to finish that patch and then put both of them up for review so that the reason for the design choices I made was clear. I think it would make a lot more sense to wait and do it that way than to try to land this one and make changes to it without being able to see how that affects the whole design.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They weren't needed to get the mouse wheel scrolling working, and given that your GestureType only had one type of gesture I felt it wasn't necessary to implement it that way. I also thought you were no longer working on this? Didn't realize you intended to bring it through to the finish line. Anyway, I've stopped working on chrome book stuff for now as we are waiting on various design docs to come through.

@stuartmorgan-g
Copy link
Contributor

I've posted flutter/flutter#21953 to provide context for the event type design choices. I think we should probably wait until there's some agreement on what that layer of the design should look like before continuing to review this, since the way pointer gestures (like trackpad scroll) are expressed informs the choices here.

@chinmaygarde
Copy link
Member

It appears this is stalled on pending discussions. Can we close this for now and re-open when ready? Trying to clear the PR backlog.

@Hixie
Copy link
Contributor

Hixie commented Sep 27, 2018

Ok, gonna close this for now.

@Hixie Hixie closed this Sep 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants