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.

Sign up for GitHub

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

[Gutenberg] Adjust the fetching of custom colors #1620

Merged
merged 3 commits into from
Jun 29, 2020
Merged

[Gutenberg] Adjust the fetching of custom colors #1620

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
119f2ae
Adjust Editor theme store to return null when fetching an empty set o…
chipsnyder Jun 25, 2020
20782cf
Fix lint issue
chipsnyder Jun 25, 2020
3d30383
Allow null on stored elements and fix typo when checking gradients
chipsnyder Jun 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions ...c/androidTest/java/org/wordpress/android/fluxc/mocked/MockedStack_EditorThemeStoreTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.wordpress.android.fluxc.mocked

import android.os.Bundle
import org.greenrobot.eventbus.Subscribe
import org.greenrobot.eventbus.ThreadMode
import org.junit.Assert
Expand Down Expand Up @@ -46,9 +47,17 @@ class MockedStack_EditorThemeStoreTest : MockedStack_Base() {

// See onEditorThemeChanged for the latch's countdown to fire.
Assert.assertTrue(countDownLatch.await(TestUtils.DEFAULT_TIMEOUT_MS.toLong(), MILLISECONDS))

// Validate Callback
assertNotEmpty(editorTheme)

// Validate Cache
val cachedTheme = editorThemeStore.getEditorThemeForSite(site)
assertNotEmpty(cachedTheme)

// Validate Bundle
val themeBundle = editorTheme!!.themeSupport.toBundle()
assertNotEmpty(themeBundle)
}

@Test
Expand All @@ -58,9 +67,17 @@ class MockedStack_EditorThemeStoreTest : MockedStack_Base() {

// See onEditorThemeChanged for the latch's countdown to fire.
Assert.assertTrue(countDownLatch.await(TestUtils.DEFAULT_TIMEOUT_MS.toLong(), MILLISECONDS))

// Validate Callback
assertEmpty(editorTheme)

// Validate Cache
val cachedTheme = editorThemeStore.getEditorThemeForSite(site)
assertEmpty(cachedTheme)

// Validate Bundle
val themeBundle = editorTheme!!.themeSupport.toBundle()
assertEmpty(themeBundle)
}

private fun assertNotEmpty(theme: EditorTheme?) {
Expand All @@ -69,8 +86,22 @@ class MockedStack_EditorThemeStoreTest : MockedStack_Base() {
}

private fun assertEmpty(theme: EditorTheme?) {
Assert.assertTrue(theme?.themeSupport?.colors.isNullOrEmpty())
Assert.assertTrue(theme?.themeSupport?.gradients.isNullOrEmpty())
Assert.assertTrue(theme?.themeSupport?.colors == null)
Assert.assertTrue(theme?.themeSupport?.gradients == null)
}

private fun assertEmpty(theme: Bundle) {
val colors = theme.getSerializable("colors")
val gradients = theme.getSerializable("gradients")
Assert.assertTrue(colors == null)
Assert.assertTrue(gradients == null)
}

private fun assertNotEmpty(theme: Bundle) {
val colors = theme.getSerializable("colors") as ArrayList<*>
val gradients = theme.getSerializable("gradients") as ArrayList<*>
Assert.assertFalse(colors.isNullOrEmpty())
Assert.assertFalse(gradients.isNullOrEmpty())
}

@Suppress("unused")
Expand Down
14 changes: 10 additions & 4 deletions fluxc/src/main/java/org/wordpress/android/fluxc/persistence/EditorThemeSqlUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,17 @@ class EditorThemeSqlUtils {
override fun getId() = mId

fun toEditorTheme(
storedColors: List<EditorThemeElementBuilder>?,
storedGradients: List<EditorThemeElementBuilder>?
storedColors: List<EditorThemeElementBuilder>,
storedGradients: List<EditorThemeElementBuilder>
): EditorTheme {
val colors = storedColors?.mapNotNull { it.toEditorThemeElement() }
val gradients = storedGradients?.mapNotNull { it.toEditorThemeElement() }
val colors = if (storedColors.count() > 0) storedColors.mapNotNull { it.toEditorThemeElement() } else null
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should keep this optional and also keep the guard, as well as checking the count, since we don't yet know why WellSql is occasionally sending null or empty. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that would probably be the safer path. I’ll make that tweak

Copy link
Contributor

@mkevins mkevins Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I tested, and everything is working as expected now. The only thing I'm wondering about is, do we have a risk of NPE dereferencing the values returned from WellSql? (i.e. I think storedColors or storedGradients can be null, since the inferred platform type is (MutableList<EditorThemeSqlUtils.EditorThemeElementBuilder!>..List<EditorThemeSqlUtils.EditorThemeElementBuilder!>?)).

https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/1620/files/20782cf123eda3e81d4812cd4dbcc360a4b6551a#diff-7279efeeae7d8b75a9f9ead365830ba1R50

I looked a bit deeper into the WellSql source, but couldn't be sure. I didn't see any nullability annotations though. So what I mean is, maybe we need to keep storedColors and storedGradients optional?

Edit: Nevermind, I was looking at an older changeset 🤦‍♂️ .. not sure why it wasn't showing me the latest code.

val gradients: List<EditorThemeElement>?
if (storedColors.count() > 0) {
gradients = storedGradients.mapNotNull { it.toEditorThemeElement() }
} else {
gradients = null
}

val editorThemeSupport = EditorThemeSupport(colors, gradients)

return EditorTheme(editorThemeSupport, stylesheet, version)
Expand Down