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

rotaryWithScroll library doesn't respect use crown haptic settings #1750

Closed
blakadder opened this issue Oct 11, 2023 · 10 comments
Closed

rotaryWithScroll library doesn't respect use crown haptic settings #1750

blakadder opened this issue Oct 11, 2023 · 10 comments
Assignees

Comments

@blakadder
Copy link

I am directed by the creators of the Home Assistant Companion app to file a bug report;

On TicWatch Pro 5 with OS 3.5 the app doesn't respect the "Use crown" vibration system settings and keeps vibration on crown use. Furthermore the type of vibrations are not the usual ones used when Use crown in enabled bit instead the strong vibration that's used for touch events is used

@yschimke
Copy link
Collaborator

Thanks for the report. We can add specific haptics but we would need to know which ones are preferred. I'll see if it's doing something stupid and using the ones for a different model.

Is there documentation for this setting? Or it's a standard wear one?

@jpelgrom
Copy link

jpelgrom commented Oct 11, 2023

I can confirm the same 'issue' exists on a Samsung Galaxy Watch 4 with the touch bezel (= rotary input on this device). In the system settings app > General > Touch bezel, there is an option "Vibration feedback". Sounds like it is kind of standard Wear but depends on the device what it's called and where it is located.

Turning it off doesn't change the behavior of apps using the rotaryWithScroll modifier, while other apps that (presumably) use a classic ScrollView do change and stop vibrating when rotating. Easy to spot as thin vs thick scroll bar.

@blakadder
Copy link
Author

blakadder commented Oct 12, 2023

Thanks for the report. We can add specific haptics but we would need to know which ones are preferred. I'll see if it's doing something stupid and using the ones for a different model.

Is there documentation for this setting? Or it's a standard wear one?

Specific or not the existing one is overly aggressive and should be toned down. Best course of action would be to use the same one used in all the standard apps

The setting also exists on the Pixel Watch as "Using crown" - https://www.verizon.com/support/knowledge-base-303381/

@yschimke
Copy link
Collaborator

yschimke commented Oct 13, 2023

So I think we need to do one of two things

  1. Find the relevant haptics code for this device, or hopefully class of devices.
  2. Fallback to reading the setting, possibly Settings.Global.getInt(context.contentResolver, "wear_vibrate_for_rotary_input")

It's possible neither are public APIs.

A quick test of 1, are you able to add this in and test on that device?

public class LikePixelWatchRotaryHapticFeedback(private val view: View) : RotaryHapticFeedback {

    @ExperimentalHorologistApi
    override fun performHapticFeedback(
        type: RotaryHapticsType,
    ) {
        when (type) {
            RotaryHapticsType.ScrollItemFocus -> {
                view.performHapticFeedback(
                    if (Build.VERSION.SDK_INT >= 33) {
                        ROTARY_SCROLL_ITEM_FOCUS
                    } else {
                        WEAR_SCROLL_ITEM_FOCUS
                    },
                )
            }

            RotaryHapticsType.ScrollTick -> {
                view.performHapticFeedback(
                    if (Build.VERSION.SDK_INT >= 33) ROTARY_SCROLL_TICK else WEAR_SCROLL_TICK,
                )
            }

            RotaryHapticsType.ScrollLimit -> {
                view.performHapticFeedback(
                    if (Build.VERSION.SDK_INT >= 33) ROTARY_SCROLL_LIMIT else WEAR_SCROLL_LIMIT,
                )
            }
        }
    }

    private companion object {
        // Hidden constants from HapticFeedbackConstants.java specific for Pixel Watch
        // API 33
        public const val ROTARY_SCROLL_TICK: Int = 18
        public const val ROTARY_SCROLL_ITEM_FOCUS: Int = 19
        public const val ROTARY_SCROLL_LIMIT: Int = 20

        // API 30
        public const val WEAR_SCROLL_TICK: Int = 10002
        public const val WEAR_SCROLL_ITEM_FOCUS: Int = 10003
        public const val WEAR_SCROLL_LIMIT: Int = 10003
    }
}

you would pass it into

public fun rememberRotaryHapticHandler(
    scrollableState: ScrollableState,
    throttleThresholdMs: Long = 30,
    hapticsThresholdPx: Long = 50,
    hapticsChannel: Channel<RotaryHapticsType> = rememberHapticChannel(),
    rotaryHaptics: RotaryHapticFeedback = rememberDefaultRotaryHapticFeedback(),
): RotaryHapticHandler {

And then that into the scroll modifiers

public fun Modifier.rotaryWithScroll(
    scrollableState: ScrollableState,
    focusRequester: FocusRequester = rememberActiveFocusRequester(),
    flingBehavior: FlingBehavior? = ScrollableDefaults.flingBehavior(),
    rotaryHaptics: RotaryHapticHandler = rememberRotaryHapticHandler(scrollableState),
    reverseDirection: Boolean = false,
): Modifier = rotaryHandler(

@jpelgrom
Copy link

A quick test of 1, are you able to add this in and test on that device?

After adding this in, I no longer get any haptic feedback at all even though it is enabled on the Galaxy Watch 4 with Wear OS 4 I'm using.

@yschimke
Copy link
Collaborator

I think we would stick with the current Samsung code. But mainly interested in devices other than pixel or Samsung.

Is the current published code not working for Samsung for you.

@Kpeved
Copy link
Collaborator

Kpeved commented Oct 16, 2023

I created a prototype branch which should use device specific haptics based on the Wear version rather than specific device.
Change: 40a7940
Branch: https://github.com/google/horologist/tree/version_specific_rotary_haptics_16.10

Please test this solution and see whether it solves your problem @blakadder @jpelgrom .
Unfortunately I don't have a lot of devices at hand for testing so would be great to test that before submitting.

@Kpeved Kpeved self-assigned this Oct 17, 2023
@jpelgrom
Copy link

Sorry for the delay, I see the change was already merged. I only have a Galaxy Watch 4 to test which is using the same haptics settings as with <= 0.5.8 so nothing to test for me.

@Kpeved
Copy link
Collaborator

Kpeved commented Oct 20, 2023

We returned Samsung haptics to its original state, so it should be working as before - haptics setting shouldn't influence compose haptics on GW devices

@yschimke
Copy link
Collaborator

Released with 0.4.16. If still happening, raise an issue with specific device details, but should be fixed.

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 a pull request may close this issue.

4 participants