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

feat: implements customizable alarm vibration #613

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

theMr17
Copy link

@theMr17 theMr17 commented Sep 3, 2023

Fixes #470

This adds a feature to enable or disable vibration of any individual alarm.

Copy link
Owner

@yuriykulikov yuriykulikov left a comment

Choose a reason for hiding this comment

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

UI looks good, but there are a few things missing:

  • What should be the behaviour of the Vibrate preference in settings?
  • What should be the default value for this new checkbox?
  • Are there any additional changes required in VibrationPlugin.kt?

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@theMr17
Copy link
Author

theMr17 commented Sep 15, 2023

  • What should be the behaviour of the Vibrate preference in settings?

According to me Vibrate preference in settings can be removed, as individual alarms will have there own vibrate preference now.

  • What should be the default value for this new checkbox?

As of now, the default value for this new checkbox is set to true

  • Are there any additional changes required in VibrationPlugin.kt?

I don't think any additional changes are required in VibrationPlugin.kt

@theMr17
Copy link
Author

theMr17 commented Sep 16, 2023

Hi @yuriykulikov, I have made the changes which you mentioned. Please have a look.

@yuriykulikov
Copy link
Owner

It seems to me that keeping the preference would be better. Probably in a different form.

What do you think about the following:

Create a new vibration preference with 3 possible values:

  • Vibration is always off (this removes the chexkbox from alarm details)
  • Vibration is on per default
  • Vibration is off per default

Another possibility would be to treat the value in settings as the default for newly created alarms

@yuriykulikov
Copy link
Owner

@the-mr17 also please have a look at https://github.com/yuriykulikov/AlarmClock/blob/develop/app/src/main/java/com/better/alarm/background/VibrationPlugin.kt#L36

@theMr17
Copy link
Author

theMr17 commented Sep 17, 2023

Create a new vibration preference with 3 possible values:

  • Vibration is always off (this removes the chexkbox from alarm details)
  • Vibration is on per default
  • Vibration is off per default

I am planning to implement this one.

@theMr17 theMr17 marked this pull request as draft September 18, 2023 14:12
@theMr17
Copy link
Author

theMr17 commented Sep 19, 2023

@yuriykulikov I have implemented the new vibration preference. Please have a look.

@theMr17 theMr17 marked this pull request as ready for review September 19, 2023 05:55
Copy link
Owner

@yuriykulikov yuriykulikov left a comment

Choose a reason for hiding this comment

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

Looking good!

Here are some remaining points:

  • VibrationPlugin changes required
  • Translations
  • Tests

@@ -35,8 +35,11 @@ class VibrationPlugin(
Observable.combineLatest(
vibratePreference,
targetVolume,
BiFunction<Boolean, TargetVolume, TargetVolume> { isEnabled, volume ->
if (isEnabled) volume else TargetVolume.MUTED
BiFunction<String, TargetVolume, TargetVolume> { vibrate, volume ->
Copy link
Owner

Choose a reason for hiding this comment

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

Try testing this. From the code alone I assume that changing the value for an alarm will not have any effect on the vibration, because here you evaluate only the global setting. I think alarm: PluginAlarmData, should contain the data you need to decide per alarm.

Copy link
Author

@theMr17 theMr17 Sep 25, 2023

Choose a reason for hiding this comment

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

I have tested it on a physical device, it works well.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, let's walk through the code.

Given
The preference value is "on"
And alarm checkbox is not checked

When
Alarm fires

Then
What is expected?
What actually happens?

Copy link
Author

Choose a reason for hiding this comment

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

I am a bit confused over here. Can you please guide me, exactly what needs to be done here.

Copy link
Owner

Choose a reason for hiding this comment

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

what needs to be done here

You have to walk through the code as if you are Android. You can try writing comments on each like. Repeat this process 6 times for this particular function with different input parameters:

  • Setting on & alarm vibrate on
  • Setting on & alarm vibrate off
  • Setting off & alarm vibrate on
  • Setting off & alarm vibrate off
  • Setting alwaysOff & alarm vibrate on
  • Setting alwaysOff & alarm vibrate off

app/src/main/java/com/better/alarm/configuration/Prefs.kt Outdated Show resolved Hide resolved
Comment on lines +91 to +93
<item>Always Off</item>
<item>On (by default)</item>
<item>Off (by default)</item>
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please add translations for German, Spanish, Italian, Norwegian, Russian and Ukranian? You can use ChatGPT for this, like this:

Please translate into German, Spanish, Italian, Norwegian, Russian and Ukranian:
<string name="vibration_title">Vibration</string>
<string-array name="vibration_entries">
        <item>On by default</item>
        <item>Off by default</item>
        <item>Disabled</item>
</string-array>

This also is not very intuitive, so probably we should carefully explain to the user what is the deal here.

Copy link
Author

Choose a reason for hiding this comment

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

Could you please add translations for German, Spanish, Italian, Norwegian, Russian and Ukranian? You can use ChatGPT for this, like this:

Sure, I am adding the translations of those languages.

This also is not very intuitive, so probably we should carefully explain to the user what is the deal here.

Can you please say more on how can I do this?

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.

Vibration only per alarm
2 participants