Skip to content

Conversation

Rene-Damm
Copy link
Contributor

@Rene-Damm Rene-Damm commented Jan 13, 2021

Fixes 1190150 (internal).

Principal change is that TouchSimulation now disables Pointer devices (that aren't already Touchscreens) and reroutes their input into a simulated Touchscreen.

Turns out this also fixes 1271942.

@Rene-Damm Rene-Damm requested a review from jimon January 13, 2021 15:19
@Rene-Damm Rene-Damm changed the title FIX: Touch simulations leading to exceptions and assertions in UI module (case 1190150). FIX: Touch simulation leading to exceptions and assertions in UI module (case 1190150). Jan 13, 2021
Copy link
Contributor

@jimon jimon left a comment

Choose a reason for hiding this comment

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

I think we shouldn't create desynchronization between managed and native states unnecessarily, if it can be avoided let's fix it in this PR before merging. I'm ok with ignoring nitpicks for now.

{
get => (m_ControlFlags & ControlFlags.IsButton) == ControlFlags.IsButton;
set
{
Copy link
Contributor

@jimon jimon Jan 15, 2021

Choose a reason for hiding this comment

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

Nitpick: Setter on question-as-property feels weird, control.isButton = true is hard to understand what it is gonna to. Either we need a better prop, like controlType which accepts an enum, and then that is mapped to flags internally. Or remove setter completely, because there is only one place in this PR using the setter

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's ok for now because button is a property of control, not type of control. But it's even more confusing.

@Rene-Damm
Copy link
Contributor Author

I think we shouldn't create desynchronization between managed and native states unnecessarily, if it can be avoided let's fix it in this PR before merging. I'm ok with ignoring nitpicks for now.

I'm not clear on what you mean by fixing, though. Having the backend keep sending events with the frontend no longer processing them IMO is something we want. Communicating this fact to the backend just seems like it would duplicate data for no benefit -- the backend wouldn't act any different. But that's probably not what you mean.

Turning the "flat" disabled flag on InputDevice into a mode that more finely discerns what specifically do with sending and what we do with receiving, that part sounds like a good avenue to explore to me, though. Is that the part you mean by fixing?

What if we have two flags? (something like "Sending events is disabled" and "Processing events is disabled")

@Rene-Damm Rene-Damm merged commit b398919 into develop Jan 26, 2021
@Rene-Damm Rene-Damm deleted the fix-touch-simulation branch January 26, 2021 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants