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

Rotary input support for InlineSlider #1748

Closed
cvb941 opened this issue Oct 8, 2023 · 7 comments
Closed

Rotary input support for InlineSlider #1748

cvb941 opened this issue Oct 8, 2023 · 7 comments

Comments

@cvb941
Copy link

cvb941 commented Oct 8, 2023

I was attempting to use rotary input to control the InlineSlider value.

From the documentation url, it seems like it should only be necessary to simply give focus to the Slider component and it would automatically receive rotary events.

However, this did not work and I had to manually setup onRotaryScrollEvent() and adjust the Slider value from there.

@yschimke
Copy link
Collaborator

yschimke commented Oct 8, 2023

Yep - not really a Horologist issue, but I can see what you drew that conclusion from the docs.

I'll fix docs next week.

You probably want to use Horologist's onRotaryInputAccumulatedWithFocus, which handles the focus for you.

We could build a wrapper for InlineSlider that does all this for you. Would that work for you?

@cvb941
Copy link
Author

cvb941 commented Oct 8, 2023

Hello, thank you for your response and proactivity.

You're right, InlineSlider is a material component and does not have anything to do with Horologist. I think I was just reading about some other rotary input-related issues of Horologist and mixed up the two.

The onRotaryInputAccumulatedWithFocus modifier accumulates a Float which would somehow need to be tied with the slider value state. For that reason, I'm using the raw onRotaryScrollEvent.

I increment the slider value when triggered and ignore the scroll value. This feels fine for Galaxy Watch 4 and the Emulator, but may be unusable for other devices.

If there is any built in code support for this, I think it should be part of the Wear material library itself. It is up to you though to judge that.

I think in an ideal scenario, the current InlineSlider should handle rotary input automatically when it gets focused.

@yschimke
Copy link
Collaborator

yschimke commented Oct 9, 2023

PR above is what it would look like for Stepper. Do you need InlineSlider particularly? How are you using it?

@cvb941
Copy link
Author

cvb941 commented Oct 9, 2023

image

I have an InlineSlider acting as a difficulty selector.

I alone wouldn't bother adding the rotary input to it, but my app kept getting rejected at Play Store reviews due to missing rotary input, even though it was present on every scrollable screen. I then tried adding rotary to this screen and the review issue has disappeared, who knows if that was the problem though.
It would be great to have some quick feedback from the reviewers with a screenshot of the problem that they have encountered.

@yschimke
Copy link
Collaborator

yschimke commented Oct 9, 2023

This is tough, I'm not sure we can do this if it's not full screen. If you have two items on this screen that are scrollable, what should it do?

I could make the default scrollable modifiers for InlineSlider, but you'd have to know to use it.

Closing for now, in favour of PR for screen level Stepper, and no obvious fix for InlineSlider.

@yschimke yschimke closed this as completed Oct 9, 2023
@cvb941
Copy link
Author

cvb941 commented Oct 9, 2023

I thought it could use the focus hierarchy for determining the consumer of the input.

If the InlineSlider is focused -> Scroll the slider
No component focused -> Scroll the list

I don't know how feasible this is though.

@yschimke
Copy link
Collaborator

yschimke commented Oct 9, 2023

Yep, but you would still need to wrap things with HierarchicalFocusCoordinator, and move between them. I'm not sure we have a pattern for that.

cc @BowerSteve is there a defined pattern for shifting focus between multiple row sized components in Wear Compose?

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

No branches or pull requests

2 participants