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

Rework audio and haptic feedback of FlorisBoard #1142

Merged
merged 2 commits into from
Aug 12, 2021

Conversation

patrickgold
Copy link
Member

@patrickgold patrickgold commented Aug 8, 2021

This PR adds a completely revamped settings panel for audio and haptic feedback when pressing keys, doing gestures, etc. and fixes a lot of bugs / unexpected behavior etc.

Audio/haptic feedback screen

The new audio/haptic screen is accessible where the old settings for sounds & vibration where located.

Audio Haptic
input-feedback-screen-audio input-feedback-screen-haptic

The feature toggles may change and do not work fully yet, still working on it.

Fixes

Known issues currently

  • When the phone is set to muted, even ignore system prefs can't help here and audio won't play at all. Fixed by adding a note that this is the cause.
  • Some features don't respect feature toggles or are assigned to the incorrect ones. Fixed by implementing the feature toggles

Old settings for reference
input-feedback-screen-old

@Glitchy-Tozier
Copy link
Collaborator

Very nice!
This may be too off-topic for this PR, but what do you think about removing those long (e.g…-additions after the settings-titles? They can't be read in normal upright-orientation, so imo they aren't that helpful.

@klaurence
Copy link
Contributor

klaurence commented Aug 8, 2021

I'm not a contributor, but I think the examples (the e.g.s) can probably be placed in the respective option's description. Unlike the options' titles, the descriptions stay readable even if it's lengthy.

@patrickgold
Copy link
Member Author

I agree, the e.g. stuff gets cut off, making it hard to read, moving it to the description would indeed be a solution.

@patrickgold patrickgold marked this pull request as ready for review August 12, 2021 10:14
@patrickgold
Copy link
Member Author

I've now moved the e.g.'s in the description, looks a lot better. Additionally I've fixed some bugs, adjusted default values and added input feedback to the Smartbar and the one-handed buttons (they were silent before). I've tested out the behavior and will now merge it into master, so I can have it in the next beta release for additional feedback and eventual bug reports on the new implementation.

@patrickgold patrickgold merged commit 07ad682 into master Aug 12, 2021
@klaurence
Copy link
Contributor

Were the types of vibration intended to be different in perceived strength? I find that, in my device, the other vibration types are weaker than the key press vibration.

Also, should further bug reports be reported in this pull request? If not, would there be a discussion thread?

@patrickgold
Copy link
Member Author

Were the types of vibration intended to be different in perceived strength? I find that, in my device, the other vibration types are weaker than the key press vibration.

Yes, this is fully intended. I did this because in my testing it was very annoying that things such as the repeated action or moving swipe vibrate in full strength rapidly (it just feels so weird), so these vibrations are reduced in strength.

Also, should further bug reports be reported in this pull request? If not, would there be a discussion thread?

That's a good question. A discussion thread will be made when I do the beta release, for now if you have a bug report just post it here.

@klaurence
Copy link
Contributor

A discussion thread will be made when I do the beta release, for now if you have a bug report just post it here.

I see. I'll keep an eye out for that thread!

Other than that, I tinkered more with the debug artifact and found some potential bugs:

  1. Holding down the Delete button on the keyboard doesn't trigger the key repeated action vibration, even at full duration and strength (100ms, 100%).
  2. The perceived strength doesn't vary in accordance with the "Vibration strength" slider value, no matter if its value is 1% or 100%. The only difference in behavior that I have found is that the keyboard doesn't vibrate at 0%.

@patrickgold
Copy link
Member Author

Holding down the Delete button on the keyboard doesn't trigger the key repeated action vibration, even at full duration and strength (100ms, 100%).

Strange, it does for me. Does the preference "Use vibrator directly" make any difference?

The perceived strength doesn't vary in accordance with the "Vibration strength" slider value, no matter if its value is 1% or 100%. The only difference in behavior that I have found is that the keyboard doesn't vibrate at 0%.

What Android version are you on? Only Android 8.0 and newer support this value, before that version I can only specify the duration.

@Glitchy-Tozier
Copy link
Collaborator

Use vibrator directly

I'd like to point out that this setting, assuming this is its full name, could probably be phrased more expressively.

Continue on, gentleman!

@patrickgold
Copy link
Member Author

patrickgold commented Aug 12, 2021

I'd like to point out that this setting, assuming this is its full name, could probably be phrased more expressively.

Ok ok I agree the preference name is a bit two-phrased now that you say it haha. If in combination with the description it is less worse:

image

An alternative frontend UI string I propose would be "Control hardware vibration directly", which is a long title though. What do you think of this title? (If you have a better title I am also open for suggestions :) )

@Glitchy-Tozier
Copy link
Collaborator

I like that the current heading is short, but the one you suggested sounds better.
Does it get cut off?

Additionally, I don't think most people know what an API is 🌝. Putting "Android" into the long description might make it easier to understand.

A non-tech-savy description might be sth. like "Trigger vibration directly instead of asking android to do it"

@klaurence
Copy link
Contributor

Does the preference "Use vibrator directly" make any difference?

It being on or off makes no difference, sadly.

What Android version are you on?

My phone is on Android 10.

"Control hardware vibration directly"
What do you think of this title?

I think it's good. The alternative titles I've thought of are "Bypass system's vibration settings" or "Ignore system's vibration settings" but they aren't much shorter than your title.

@Glitchy-Tozier
Copy link
Collaborator

Ignore system's vibration settings

This sounds amazing. Is it correct, @patrickgold ?

@klaurence
Copy link
Contributor

Actually, I don't think that's correct, as I've just realized that that title is already used as another option! That's a facepalm, to be honest 😄

@patrickgold
Copy link
Member Author

It being on or off makes no difference, sadly.

Ok thanks for clarifying, will debug the input feedback manager tomorrow, maybe I can catch the bug.

Ignore system's vibration settings

This sounds amazing. Is it correct, @patrickgold ?

Nope, it is not correct. There's a setting above the vibrator one which is named exactly like this and indeed checks a system setting if system-wide touch vibration is enabled or not. The use vibrator one literally decides if I call <view>.performHapticFeedback() or vibrator.vibrate(), the former one behaving like a b*tch on some devices while the latter one is much more stable, thus I give users the choice here.

Another suggestion for the vibrator preference:

Title: Trigger vibration directly
Summary: Trigger vibration directly instead of using Android's haptic feedback feature set

It has a short title but a non-tec-savy description IMO. What do both of you think of this option?

@Glitchy-Tozier
Copy link
Collaborator

What do both of you think of this option?

I like it a lot, go for it! :)

@klaurence
Copy link
Contributor

klaurence commented Aug 13, 2021

I like it. I also prefer it over "Control hardware vibration directly". The only thing I want to do is add why this option is necessary in the summary, so the option would look like:

Trigger vibration directly
Trigger vibration directly instead of using Android's haptic feedback feature set that might not work properly on some devices

This summary could entice users to enable the option. I don't think there is a problem with enabling it unless it's unnecessary or even harmful, but I'm not sure if it is either of the two.

@patrickgold
Copy link
Member Author

Does the preference "Use vibrator directly" make any difference?

It being on or off makes no difference, sadly.

I just discovered that while >=Android 8.0 devices supports providing an amplitude in the Vibrator service, many devices have no hardware amplitude control support and then the amplitude (which is basically the value of strength but adjusted) is just ignored. I will add this to the summary of the vibration strength preference.

@patrickgold
Copy link
Member Author

I don't think there is a problem with enabling it unless it's unnecessary or even harmful, but I'm not sure if it is either of the two.

Enabling this is never harmful, as I just do a short one-shot vibration effect. Neither unnecessary, more like a personal preference of the user. The only devices where the haptic feedback API is actually good and even better than using the vibrator is on Google Pixel devices. Seems like Google devices have some well-implemented haptics on a hardware level, thus using the vibrator directly is a step back.

@patrickgold
Copy link
Member Author

#1150 is the "part 2" of this rework, which overhauls the mentioned UI strings and also adds some more descriptive summaries for the vibration strength and why it won't work (if your device is affected). @klaurence as you have had this PR's debug artifact, could you also try out #1150's and test the settings panel of the audio/haptic panel? Thanks in advance!

@klaurence
Copy link
Contributor

klaurence commented Aug 13, 2021

I just discovered that while >=Android 8.0 devices supports providing an amplitude in the Vibrator service, many devices have no hardware amplitude control support and then the amplitude (which is basically the value of strength but adjusted) is just ignored. I will add this to the summary of the vibration strength preference.

Enabling this is never harmful. Neither unnecessary, more like a personal preference of the user.

These make sense.

As you have had this PR's debug artifact, could you also try out #1150's and test the settings panel of the audio/haptic panel?

I'm currently having issues with downloading the artifact, but once the issues disappear, I'll update you.

@klaurence
Copy link
Contributor

The vibration strength option is grayed out and says that my device doesn't support amplitude control, which explains the bug, frankly. The new "Trigger vibration directly" title and summary strings appear in my phone just fine too.

Other than that, nothing in the artifact's settings seems out of the blue for me.

Thank you for the bugfix!

@patrickgold
Copy link
Member Author

The vibration strength option is grayed out and says that my device doesn't support amplitude control, which explains the bug, frankly. The new "Trigger vibration directly" title and summary strings appear in my phone just fine too.

I added this because else I suspect I will continue to get issues for the vibration strength and why it won't work.

Thanks for testing it out. I will merge the improvements in and they will be released in beta09 either tomorrow or on Sunday.

Waelwindows added a commit to Waelwindows/florisboard that referenced this pull request Aug 15, 2021
commit bcbf561
Author: Patrick Goldinger <patrick@patrickgold.dev>
Date:   Sat Aug 14 18:39:16 2021 +0200

    Fix popup merge bug for group assigned keys (florisboard#1028)

commit 813f300
Author: Waelwindows <Waelwindows@users.noreply.github.com>
Date:   Sat Aug 14 12:49:56 2021 +0300

    Adjust Arabic popups for main forms and remove nums (florisboard#1087)

    This commit makes the most common popups the main ones which should
    allow Arabic sub-layout users to use FlorisBoard's smart popup feature.

commit a356585
Author: Patrick Goldinger <patrick@patrickgold.dev>
Date:   Fri Aug 13 19:59:16 2021 +0200

    Fix 5+1 keyboard layout bug (florisboard#1100)

commit 689881f
Author: dvrnynr <80413364+dvrnynr@users.noreply.github.com>
Date:   Fri Aug 13 18:12:43 2021 +0200

    Remove popups not related to Turkish

commit d473369
Author: Patrick Goldinger <patrick@patrickgold.dev>
Date:   Fri Aug 13 14:37:03 2021 +0200

    Improve haptic feedback UI and internal logic

commit 5fcd605
Author: Patrick Goldinger <patrick@patrickgold.dev>
Date:   Thu Aug 12 15:33:18 2021 +0200

    Possibly fix repeating delete key

commit 2ea9dfe
Author: Patrick Goldinger <patrick@patrickgold.dev>
Date:   Fri Aug 13 18:51:12 2021 +0200

    Fix theme editor preview looking distorted (florisboard#1136)

commit 07ad682
Merge: efc03a9 1c8523c
Author: Patrick Goldinger <patrick.goldinger@pm.me>
Date:   Thu Aug 12 12:31:32 2021 +0200

    Merge pull request florisboard#1142 from florisboard/input-feedback-manager

    Rework audio and haptic feedback of FlorisBoard

commit 1c8523c
Author: Patrick Goldinger <patrick@patrickgold.dev>
Date:   Thu Aug 12 12:14:07 2021 +0200

    Adjust input feedback feature toggle internals

commit 84f682a
Author: Patrick Goldinger <patrick@patrickgold.dev>
Date:   Sun Aug 8 11:57:05 2021 +0200

    Add new InputFeedbackManager
@patrickgold patrickgold mentioned this pull request Aug 15, 2021
@patrickgold patrickgold deleted the input-feedback-manager branch March 22, 2022 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants