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/notification accent color dark mode #1370

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Jun 29, 2021

Issue

Notification accent color set in strings.xml in values-night folder were not being used when a device is in dark mode. The color was set in the manifest metadata com.onesignal.NotificationAccentColor.DEFAULT and did not seem to grab the night values.

Solution

Get the resource string directly in GenerateNotification.getAccentColor(). Users will need to put <string name="onesignal_notification_accent_color"> in their resources instead of in the manifest. Documentation will need to be updated as well.

Tests

The following tests were written in GenerateNotificationRunner.java:

  • shouldUseDarkIconAccentColorInDarkMode_hasMetaData
  • shouldUseDayIconAccentColorInDayMode
  • shouldUseManifestIconAccentColor
  • shouldUseBgacAccentColor_hasMetaData

In order to test, I removed the private keyword from getAccentColor().


This change is Reviewable

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @emawby and @Jeasmine)

Copy link
Contributor

@Jeasmine Jeasmine left a comment

Choose a reason for hiding this comment

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

Nice one! As a recommendation, you might want to use the squash and merge option since we might not want some commits in the git history.

Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nan-li)


OneSignalSDK/onesignal/src/main/java/com/onesignal/GenerateNotification.java, line 942 at r1 (raw file):

            return new BigInteger(defaultColor, 16);
         }
      } catch (Throwable t) {} // Can throw a parse error parse error.

Nit typo in the comment


OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java, line 2336 at r1 (raw file):

   @Test
   @Config(shadows = { ShadowResources.class })
   public void shouldUseManifestIconAccentColor() throws Exception {

Great job on these unit tests!

Modified files:
- GenerateNotification.java
- OneSignalPackagePrivateHelper.java

Created file:
- ShadowResources.java

Added 4 unit tests for accent colors in GenerateNotificationRunner.java:
- shouldUseDarkIconAccentColorInDarkMode_hasMetaData
- shouldUseDayIconAccentColorInDayMode
- shouldUseManifestIconAccentColor
- shouldUseBgacAccentColor_hasMetaData

Added resources to unittest app
@nan-li nan-li force-pushed the feat/notification-accent-color-dark-mode branch from c6a5c0b to 3371d8d Compare June 29, 2021 18:15
@nan-li
Copy link
Contributor Author

nan-li commented Jun 29, 2021

I recall the build didn't fail when I first made the pull request. I then did squash my misc. commits into 1 commit, which I'm wondering if it is a problem.

@jkasten2
Copy link
Member

This test failed.

com.test.onesignal.GenerateNotificationRunner > shouldContainPayloadWhenOldSummaryNotificationIsOpened FAILED
    java.lang.Exception: Main looper has queued unexecuted runnables. This might be the cause of the test failure. You might need a shadowOf(getMainLooper()).idle() call.
        at org.robolectric.android.internal.AndroidTestEnvironment.checkStateAfterTestFailure(AndroidTestEnvironment.java:502)
        at org.robolectric.RobolectricTestRunner$HelperTestRunner$1.evaluate(RobolectricTestRunner.java:581)
        at org.robolectric.internal.SandboxTestRunner$2.lambda$evaluate$0(SandboxTestRunner.java:263)
        at org.robolectric.internal.bytecode.Sandbox.lambda$runOnMainThread$0(Sandbox.java:89)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:748)
        Caused by:
        junit.framework.ComparisonFailure: expected:<UUID[2]> but was:<UUID[1]>
            at junit.framework.Assert.assertEquals(Assert.java:100)
            at junit.framework.Assert.assertEquals(Assert.java:107)
            at com.test.onesignal.GenerateNotificationRunner.shouldContainPayloadWhenOldSummaryNotificationIsOpened(GenerateNotificationRunner.java:344)

Re-running tests as it could be a flaky test.

Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @Jeasmine and @jkasten2)

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Jeasmine)

@nan-li nan-li merged commit 0319b90 into main Jul 1, 2021
@nan-li nan-li deleted the feat/notification-accent-color-dark-mode branch July 1, 2021 19:00
@Jeasmine Jeasmine mentioned this pull request Jul 12, 2021
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.

4 participants