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

[Gutenberg] Adjust the fetching of custom colors #1620

Merged
merged 3 commits into from
Jun 29, 2020

Conversation

chipsnyder
Copy link
Contributor

@chipsnyder chipsnyder commented Jun 25, 2020

Fixes: wordpress-mobile/gutenberg-mobile#2432
WPAndroid PR: wordpress-mobile/WordPress-Android#12289

To Test:

  1. Log in to the demo app for a self-hosted site
  2. Set a theme with custom colors and/or gradients on your site
  3. In the demo app select Fetch theme from first site
    Expect to see a log of colors and/or gradients
  4. Set a theme with out custom colors and/or gradients on your site
  5. In the demo app select Fetch theme from first site
    Expect to see a log of [null] for colors and gradients

@chipsnyder chipsnyder changed the title [Gutenberg] Adjust the fetching of [Gutenberg] Adjust the fetching of custom colors Jun 25, 2020
Comment on lines 95 to 98
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

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.

Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

Nice work Chip! I tested via the steps described, works as expected. LGTM!

@chipsnyder chipsnyder merged commit 2040bd0 into develop Jun 29, 2020
@chipsnyder chipsnyder deleted the rnmobile/2432-defaultColors branch June 29, 2020 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default Gradients are lost on theme changes for self hosted sites.
2 participants