Skip to content

Added vibration option in App Overrides #32

Open
ahmed-shehata wants to merge 12 commits intolangerhans:mainfrom
ahmed-shehata:newVibration
Open

Added vibration option in App Overrides #32
ahmed-shehata wants to merge 12 commits intolangerhans:mainfrom
ahmed-shehata:newVibration

Conversation

@ahmed-shehata
Copy link

Added vibration option in App Overrides

This allows the user to select the vibration strength per app/game

This PR changes

  • Added option for Vibration Strength (Off/Low/Medium/High)
  • Migrated database to include vibrationStrength field

image

@miguelguzmanr miguelguzmanr self-assigned this Apr 18, 2024
Copy link
Collaborator

@miguelguzmanr miguelguzmanr left a comment

Choose a reason for hiding this comment

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

I have added a review.

}

companion object {
private const val KEY_VIBRATION_STRENGTH = "vibration_strength"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Property "KEY_VIBRATION_STRENGTH" is never used

@miguelguzmanr
Copy link
Collaborator

Thank you for taking the time to submit a pull request!
Some concerns:

  • Current implementation of changing the vibration strength is exclusive to (only works for) the Odin2. Let's consider disabling/hiding this per-app setting for non-Odin2 devices.
  • Changing the vibration strength via the main screen uses a slider to manually assign a preferred value. Meanwhile, this per-app setting changes the vibration strength between preset values (Low, Mid, High). I like this, but let's consider if consistency is required or not e.g. main screen should also work with preset values.
  • This per-app setting changes the vibration strength. However, if vibration isn't enabled, it will not work. Let's consider the "vibrate_on" system setting, which is currently used in the main screen to enable/disable vibration.

@langerhans Please let us know your thoughts as well!

@ahmed-shehata
Copy link
Author

@miguelguzmanr Thanks a lot for the review, I've changed the option to be numeric instead for better customizability:

Screenshot 2024-05-16 at 11 27 53 pm
Screenshot 2024-05-16 at 11 27 57 pm

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

ktlint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

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