Notification signal settings. #305 #403

lemonboston · 2017-08-10T17:09:40Z

This pull requests adds 3 extra entries to the Settings screen for customizing notifications by enabling/disabling the sound, vibration, and lights.

A new NotificationSignal type is introduced to handle this aspect better.
The intent extra indicating that a notification should be silent has been renamed to "no signal" along with boolean variables representing it, in order to make it less ambiguous - "silent" and especially "withSound" can refer to only sound, while as I understand in these cases we need the notification to not signal in any way.

The changes also fix #399 by using no signal for pinned notification.

Knows issue:
Task started notification for already pinned task doesn't blink the LED, only plays sound and vibrate (when all is enabled). I don't know why is this, maybe it's a system behavior that updating existing notification doesn't use the lights.
But I suppose it's not that important, let me know if I should look into it, create a separate task maybe.

dmfs · 2017-09-04T10:56:53Z

@lemonboston please rebase on settings/master and while you're at it, rename all the strings starting with schedjoules… to opentasks… ;-)

lemonboston · 2017-09-04T14:53:14Z

Rebased and fixed the strings (oops..)

dmfs

First pass done.

dmfs · 2017-09-05T11:27:06Z

opentasks/src/main/res/xml/app_preferences.xml

-    <CheckBoxPreference
-            android:key="@string/schedjoules_pref_notification_enabled"
+    <SwitchPreference
+            android:key="@string/opentasks_pref_notification_enabled"
            android:title="@string/enable_alarms"


Please also rename this string resource to opentasks_settings_notification_enable.

dmfs · 2017-09-05T11:37:15Z

opentasks/src/main/java/org/dmfs/tasks/notification/signals/CustomizableSignal.java

+
+
+    @Override
+    public int defaultsValue()


SRP! This should be 3 decorator classes: Sound, Vibrator, Light or maybe even better: Acoustic, Haptic and Visual as adjectives are better names for decorators. Each of these would add its flag to the result of defaultsValue.
like this

public final class Haptic implements NotificationSignal { private final NotificationSignal mDelegate; public Haptic() { this(new NoSignal()); } public Haptic(NotificationSignal delegate) { mDelegate = delegate; } @Override public int defaultsValue() { return BitFlagUtils.addFlag(mDelegate.defaultsValue(), Notification.DEFAULT_VIBRATE); } }

You probably can refactor that into a delegation pattern with a common delegate which takes the integer flag value to add.

In order to create a NotifcationsSignal from three booleans you could use a Composite in conjunction with the Switchable decorator:

new Composite(new Switchable(new Visual(), lights), new Switchable(new Acoustic(), sound), new Switchable(new Haptic(), vibrate));

This is certainly more verbose, so may we can find an even better solution.

True that they can be separated more, I remember that I thought about that for a second as well.

So I've refactored it now. I didn't use the names Acoustic, Haptic Visual because I think it would just add a parallel terminology even if in some terms it could be more appropriate. I think it's simpler to keep in line with the constant names in Notification which also correspond to settings UI labels as welll. So I just used Sound, Lights, Vibration.

The result is this:

return new Lights(lights, new Vibration(vibrate, new Sound(sound, new NoSignal()))).value();

dmfs · 2017-09-05T11:42:42Z

opentasks/src/main/java/org/dmfs/tasks/notification/signals/NotificationSignal.java

+     * <p>
+     * (<code>0</code> means no signal.)
+     */
+    int defaultsValue();


the name defaultsValue() isn't very good. I know that it's meant to refer to the name of the method which takes the value, but initially I thought there is a typo and it should be called defaultValue which still doesn't make any sense either.
How about just value? NotificationSignal should make it clear enough what sort of value you can expect.

You're right, I agree that that name was not good. Changed to the suggested value().

dmfs · 2017-09-05T11:45:57Z

opentasks/src/main/java/org/dmfs/tasks/notification/signals/SwitchableSignal.java

+    private final NotificationSignal mDelegate;
+
+
+    public SwitchableSignal(Context context, boolean withoutSignal)


This should be a decorator which evaluates lazily.

dmfs · 2017-09-05T12:29:25Z

opentasks/src/main/res/values/strings.xml

+    <string name="opentasks_settings_notification_vibrate">Vibrate</string>
+    <!-- Notification "Pulse light" enabling/disabling entry label in settings -->
+    <string name="opentasks_settings_notification_lights">Pulse light</string>
+


German translations:

Notification signals: Benachrichtungseinstellungen
Sound: Ton
Vibrate: Vibration
lights: LED

Added them.

…l instead of AllSignal in Toggled (doesn't matter).

lemonboston · 2017-09-07T14:36:18Z

Ready for next round of review.

dmfs · 2017-09-08T06:22:12Z

opentasks/src/main/java/org/dmfs/tasks/notification/signals/SwitchableSignal.java

+    private final boolean mWithoutSignal;
+
+
+    public SwitchableSignal(Context context, boolean withoutSignal)


I'm still not happy with this class. As mentioned before this should be a decorator which takes the "enabled" signal in the constructor. There could be a secondary constructor which then passes a SettingsSignal. So this becomes more like a "conditional signal" like so:

public final class Conditional implements NotificationSignal { private final NotificationSignal mDelegate; private final boolean mCondition; public Conditional(Context context, boolean condition) { this(new SettingsSignal(mContext), condition); } public Conditional(NotificationSignal delegate, boolean condition) { mDelegate = delegate; mCondition = contition; } @Override public int value() { return (mWithoutSignal ? new NoSignal() : mDelegate).value(); } }

You're right, it's better that way, I missed the decoration the first time. Updated now.

dmfs · 2017-09-08T06:58:55Z

opentasks/src/main/java/org/dmfs/tasks/utils/BitFlagUtils.java

+ *
+ * @author Gabor Keszthelyi
+ */
+public final class BitFlagUtils


This class should be removed. Either inline the code (Only Toggled and ToggledTest use any of these methods) or refactor it into a proper model. Maybe something like Flags with decorators like Set, Cleared, Flipped ("contains" could be tested with flags.equals(new Set(flags, flag))).
I would be ok if you move the respective methods as private methods into Toggled and ToggledTest for now.

Just noticed that not even ToggledTest should use any of these methods.

Removed the class and just used private methods as suggested.

dmfs · 2017-09-08T07:02:49Z

opentasks/src/test/java/org/dmfs/tasks/notification/signals/ToggledTest.java

+    @Test
+    public void testValidFlags()
+    {
+        new Toggled(Notification.DEFAULT_SOUND, true, new NoSignal());


This test doesn't test anything. It just creates an object but doesn't use it. Why would I do that in my code?

dmfs · 2017-09-08T07:08:05Z

opentasks/src/test/java/org/dmfs/tasks/notification/signals/ToggledTest.java

+    @Test
+    public void testAddingFlag()
+    {
+        assertTrue(containsFlag(


don't use containsFlag here, instead test the expected outcome like so

assertThat(new Toggled(Notification.DEFAULT_SOUND, true, new NoSignal()).value(), is(new NoSignal().value() | Notification.DEFAULT_SOUND)); assertThat(new Toggled(Notification.DEFAULT_SOUND, false, new NoSignal()).value(), is(new NoSignal().value()));

dmfs · 2017-09-08T07:57:07Z

opentasks/src/test/java/org/dmfs/tasks/notification/signals/ToggledTest.java

+    @Test(expected = IllegalArgumentException.class)
+    public void testInValidFlag()
+    {
+        new Toggled(15, true, new NoSignal());


Same here. If I never call value() why shouldn't I create a Toggled object with invalid params? Who cares if I don't use the value it returns?
A test case should test actual usage. But this test doesn't use the object it just creates one.

It's like building a car with just three wheels. That's totally fine if you don't intend to drive it, right?

This probably boils down to the question if a constructor should be code free (including validation) or not and whether eager validation is better than lazy validation. I think the latter one is more "declarative". I mean I can declare a trunk to be a car and that's fine if I don't intend to start the engine and actually drive with it (that's why it works pretty well for kids). If I try to start the engine of a trunk I'll fail badly, but not before.
In our code we don't care if the car is actually a trunk as long as we can start the engine. That's why we have test cases.
Or to go with the three wheel car example, we don't care how many wheels it has if it passes the "drive" test. Or do we?

Moved this validation from ctor to method. Not sure we even need it, but it doesn't hurt. Updated the tests accordingly to call on value().
(Personally, I don't have a clear opinion on the eager or lazy validation, in general I think fail-fast is good, but this declarative oop approach doesn't follow that, so not sure what fits best.)

lemonboston · 2017-09-08T12:24:28Z

Read for review again.

lemonboston assigned dmfs Aug 10, 2017

lemonboston requested a review from dmfs August 10, 2017 17:09

lemonboston mentioned this pull request Aug 10, 2017

Pinned task triggers the notification LED #399

Closed

dmfs changed the base branch from settings/279-settings-screen to settings/master September 4, 2017 10:53

lemonboston force-pushed the settings/305-vibration branch from d9304f0 to 7d8e8fa Compare September 4, 2017 14:46

Notification signal settings. #305

1e7cf56

lemonboston force-pushed the settings/305-vibration branch from 7d8e8fa to 1e7cf56 Compare September 4, 2017 14:51

dmfs requested changes Sep 5, 2017

View reviewed changes

Gabor Keszthelyi added 2 commits September 7, 2017 14:15

Update notification settings label string resource name.

011b091

Improve design of NotificationSignals.

4b37347

lemonboston force-pushed the settings/305-vibration branch from 427dc03 to 4b37347 Compare September 7, 2017 13:16

Gabor Keszthelyi added 2 commits September 7, 2017 16:14

Add German translations.

5635239

Fix flag disable in Toggled. Rename Vibrate to Vibration. Use NoSigna…

562d08b

…l instead of AllSignal in Toggled (doesn't matter).

dmfs requested changes Sep 8, 2017

View reviewed changes

Gabor Keszthelyi added 2 commits September 8, 2017 14:07

Improve former SwitchableSignal, renamed to Conditional.

c4281ce

Remove BitFlagUtils. Move validation in Toggled from ctor to method.

0abbd2a

dmfs approved these changes Sep 14, 2017

View reviewed changes

dmfs merged commit fa344d7 into settings/master Sep 14, 2017

dmfs deleted the settings/305-vibration branch September 14, 2017 20:28

lemonboston pushed a commit that referenced this pull request Sep 18, 2017

Notification signal settings. #305 (#403)

74307fe

dmfs pushed a commit that referenced this pull request Sep 19, 2017

Notification signal settings. #305 (#403)

6d5db7b

dmfs mentioned this pull request Oct 9, 2017

option for disabling vibrations on notification #305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notification signal settings. #305 #403

Notification signal settings. #305 #403

lemonboston commented Aug 10, 2017

dmfs commented Sep 4, 2017

lemonboston commented Sep 4, 2017

dmfs left a comment

dmfs Sep 5, 2017

lemonboston Sep 7, 2017

dmfs Sep 5, 2017

lemonboston Sep 7, 2017 •

edited

Loading

dmfs Sep 5, 2017

lemonboston Sep 7, 2017

dmfs Sep 5, 2017

lemonboston Sep 7, 2017

dmfs Sep 5, 2017

lemonboston Sep 7, 2017

lemonboston commented Sep 7, 2017

dmfs Sep 8, 2017

lemonboston Sep 8, 2017

dmfs Sep 8, 2017

dmfs Sep 8, 2017

lemonboston Sep 8, 2017

dmfs Sep 8, 2017

lemonboston Sep 8, 2017

dmfs Sep 8, 2017

lemonboston Sep 8, 2017

dmfs Sep 8, 2017

lemonboston Sep 8, 2017

lemonboston commented Sep 8, 2017

		private final NotificationSignal mDelegate;


		public SwitchableSignal(Context context, boolean withoutSignal)

		private final boolean mWithoutSignal;


		public SwitchableSignal(Context context, boolean withoutSignal)

Notification signal settings. #305 #403

Notification signal settings. #305 #403

Conversation

lemonboston commented Aug 10, 2017

dmfs commented Sep 4, 2017

lemonboston commented Sep 4, 2017

dmfs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemonboston Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemonboston commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemonboston commented Sep 8, 2017

lemonboston Sep 7, 2017 •

edited

Loading