Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix drag vs align #585

Merged
merged 11 commits into from
Sep 9, 2023
Merged

Fix drag vs align #585

merged 11 commits into from
Sep 9, 2023

Conversation

zsoltk
Copy link
Contributor

@zsoltk zsoltk commented Sep 8, 2023

Description

Related to #539

Issue that arose in that PR's context

  • boundsInParent() gives an offset relative to 0,0 in parent – this offset is what we use to setup the gesture detection area
  • .align(), if present in the elementUiModel modifiers will also affect the gesture detection area's alignment though, and we can't turn it off, because:
  • .align() can only be set once in compose (or more precisely, the first one takes priority, any subsequent invocations are ignored), which means we couldn't do the same trick as with offset (+offset, gesture, -offset) to temporarily disable alignment
  • but without turning it off temporarily the effect is that boundsInParent() offset is always relative to 0,0 whereas it should be relative to the alignment point, resulting in a wrong target area being calculated

Approach that was implemented instead:

  • A Box was extracted from Child to apply the gesture detection on – this way it can remain unaffected by alignment change

The problem with the approach

  • If Child has any gesture-related code (e.g. click), pointer input handling can't properly reach the Box by bubbling up, since it's not a parent to Child but a sibling in the hierarchy
  • Dragging the AppyxComponent would break in this case and no longer do anything

New solution

  • Revert back to using a single composable
  • Detecting if PositionInside/PositionOutside MotionProperty are present
  • Fetch their the alignment-related offset (relative to 0,0) and account for it in the offset calculation for the gesture

Result

  • Click + Drag works properly
  • Drag detection area is calculated properly

...

Check list

  • I have updated CHANGELOG.md if required.
  • I have updated documentation if required.

@zsoltk zsoltk added bug Something isn't working gestures appyx-interactions labels Sep 8, 2023
@zsoltk zsoltk added this to the 2.0 milestone Sep 8, 2023
Comment on lines +188 to +189
val positionInside = motionPropertyRenderValue<Value, PositionInside>()
val positionOutside = motionPropertyRenderValue<PositionOutside.Value, PositionOutside>()
Copy link
Contributor

Choose a reason for hiding this comment

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

What about importing PositionInside.Value and PositionOutside.Value using aliases to make this code more orthogonal? Something like this:

import com.bumble.appyx.interactions.core.ui.property.impl.position.PositionInside.Value as PositionInsideValue
import com.bumble.appyx.interactions.core.ui.property.impl.position.PositionOutside.Value as PositionOutsideValue
// ...
val positionInside = motionPropertyRenderValue<PositionInsideValue, PositionInside>()
val positionOutside = motionPropertyRenderValue<PositionOutsideValue, PositionOutside>()

Comment on lines +216 to +217
val positionInside = motionPropertyRenderValue<Value, PositionInside>()
val positionOutside = motionPropertyRenderValue<PositionOutside.Value, PositionOutside>()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same could be applied here.

}
}
}
}

@Composable
private fun elementOffset(
Copy link
Contributor

Choose a reason for hiding this comment

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

What about moving this elementOffset composable somewhere common but internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As part of #534 we can keep only one rather than moving out.

@zsoltk zsoltk merged commit 6cd22d7 into bumble-tech:2.x Sep 9, 2023
7 checks passed
@zsoltk zsoltk deleted the fix-drag-vs-align branch September 9, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants