MM-23853 Support dark mode and light mode syncing with OS theme setting #18140

komik966 · 2021-08-14T06:09:48Z

Summary

This PR adds:

validation for dark theme preference
db migration to turn off theme synchronization with os for users who have changed their theme before the introduction of this feature

Ticket Link

Release Note

Added validation for theme_dark preference on API endpoint PUT /users/:user_id/preferences

mattermod · 2021-08-14T06:09:52Z

Hello @komik966,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

komik966 · 2021-08-14T06:31:37Z

https://github.com/mattermost/mattermost-server/blob/308a88efb17e5756812a992ea17dd2cb346bcb8a/app/export.go#L35
https://github.com/mattermost/mattermost-server/blob/308a88efb17e5756812a992ea17dd2cb346bcb8a/app/import_functions.go#L583
At first sight, import/export will be not complete. From now, full theme preferences consist of theme, theme_dark, disable_theme_sync categories. Should it be fixed in this PR?

…os-theme-sync

streamer45

Thanks for the contribution @komik966 🎉! I've left some questions.

streamer45 · 2021-09-02T12:41:47Z

model/preference.go

@@ -116,7 +118,7 @@ func (o *Preference) PreUpdate() {

 		// blank out any invalid theme values
 		for name, value := range props {
-			if name == "image" || name == "type" || name == "codeTheme" {
+			if name == "image" || name == "type" || name == "codeTheme" || name == "colorScheme" {


Does this colorScheme name refer to the light/dark mode?

@streamer45 possible values of colorScheme are light / dark. It is used by client apps to categorize themes in theme selection form:

Thanks 👍 I am wondering if the name is appropriate. I identify a color scheme as a set of colors and you can have many of those. Maybe lightMode would be more fitting?

@streamer45 React native uses colorScheme https://reactnative.dev/docs/appearance#getcolorscheme. I named it same way to keep "ubiquitous language".

For me, it is no problem to change it 😉

@streamer45 I think isLight would be better (this is theme's property so in the webapp we have theme.isLight)

React native uses colorScheme https://reactnative.dev/docs/appearance#getcolorscheme. I named it same way to keep "ubiquitous language".

TIL :)

I think isLight would be better (this is theme's property so in the webapp we have theme.isLight)

Yeah that's good as well but I have no strong opinion. We can also keep it as is since now I know why it was named that way and I am all for consistency :) @esethna , thoughts?

streamer45 · 2021-09-02T12:48:30Z

store/sqlstore/upgrade.go

+	disableThemeSync := `
+		INSERT INTO preferences (userid, category, name, value)
+		SELECT userid, '` + model.PreferenceCategoryDisableThemeSync + `', name, 'true'
+		FROM preferences
+		WHERE category = 'theme'
+		`
+	if _, err := sqlStore.GetMaster().ExecNoTimeout(disableThemeSync); err != nil {
+		mlog.Error("Error inserting "+model.PreferenceCategoryDisableThemeSync+" preferences", mlog.Err(err))
+	}


Is this actually needed? I am thinking that if nothing special is found we should consider the feature disabled for the user. Unless we are explicitly making it an opt-out thing.

I was thinking the same. In addition to this; if we auto enable this, we will need to set a dark_theme by default as well.

@streamer45 Intention of this DB migration is to disable synchronization for users who already have edited their theme #15975 (comment)
@isacikgoz dark_theme works same way as theme works already. When no dark_theme is saved, client apps use default dark theme.

streamer45 · 2021-09-02T13:33:01Z

model/preference.go

-	PreferenceCategoryTheme = "theme"
+	PreferenceCategoryTheme            = "theme"
+	PreferenceCategoryThemeDark        = "theme_dark"
+	PreferenceCategoryDisableThemeSync = "disable_theme_sync"


This goes along with my other comment. Maybe we should consider an enable_theme_sync boolean setting instead.

By default, theme synchronization should be enabled. Only users who have changed their theme before introduction of this feature should have synchronization disabled.

enable_theme_sync ~~will force us to save this setting during new user registration~~.
disable_theme_sync is saved only when user explicitly disables / enables it in theme selection form.

So disable_theme_sync have three states:

empty (no row in db table) - use default behavior (sync with os), we know that user never has switched it in theme selection form.

true - not sync with os

false - sync with os

@streamer45
Now I see, the same is doable with enable_theme_sync, and it's easier 🙃:
empty (no row in db table) - use default behavior (sync with os), we know that user never has switched it in theme selection form.
true - sync with os
false - not sync with os

Sounds good. Also I think that defaulting to positive logic (enable vs disable) in variable/setting naming is usually better and easier to digest.

…os-theme-sync

mattermod · 2021-09-13T01:01:40Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

streamer45

One change needed but otherwise looks great, thanks @komik966 👍

streamer45 · 2021-10-05T15:15:43Z

store/sqlstore/upgrade.go

+	disableThemeSync := `
+		INSERT INTO preferences (userid, category, name, value)
+		SELECT userid, '` + model.PreferenceCategoryEnableThemeSync + `', name, 'false'
+		FROM preferences
+		WHERE category = 'theme'
+		`
+	if _, err := sqlStore.GetMaster().ExecNoTimeout(disableThemeSync); err != nil {
+		mlog.Error("Error inserting "+model.PreferenceCategoryEnableThemeSync+" preferences", mlog.Err(err))
+	}


This should be moved to the upgradeDatabaseToVersion610 method now.

moved to 610

@komik966 By now this is scheduled for 6.2.0 so it should be moved again. You may have to create a new method for it. Let me know if you need help with that.

esethna

Approving to not block this, I will review on the webapp PR

isacikgoz

Nothing on top of @streamer45's point. Anticipating this feature for a long time, awesome work @komik966 🎉

mattermod · 2021-10-22T01:01:47Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

esethna · 2021-10-26T22:02:15Z

Adding @jgilliam17 for QA review.

@komik966 once the changes for webapp are ready we can spin up a test server and as long as the branch names are the same on the webapp and server PRs our test servers will pick up the changes together for testing

FYI @jgilliam17 there's also a mobile PR: mattermost/mattermost-mobile#5635

…os-theme-sync

esethna · 2021-11-17T23:18:42Z

@komik966 just circling back on this as I was out for a few weeks. What are the next steps for the server, webapp and mobile PRs? Let us know when/if they are ready for reviews and I can help chase them down

komik966 · 2021-11-17T23:48:05Z

@esethna

server pr is ready - only migration's server version adjustment is required when we will know which release will add os theme sync
mobile pr is waiting for further review (MM-23853 Support dark mode and light mode syncing with OS theme setting mattermost-mobile#5635 (comment))
web pr - Due to lack of time, I did not finish fixing it yet. I hope to find time for it this Sunday.

esethna · 2021-11-18T20:05:23Z

Great, thanks for the update @komik966! We will wait to merge this until it can be tested with the webapp PR cc// @jgilliam17

mattermod · 2021-11-30T01:01:46Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

esethna · 2022-02-01T22:37:41Z

Hey @komik966, let us know if you're still working on this. Would love to see it in the product!

komik966 · 2022-04-05T20:11:41Z

#15975 (comment)

MM-23853 Support dark mode and light mode syncing with OS theme setting

a2c18f6

mm-cloud-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 14, 2021

Merge remote-tracking branch 'upstream/master' into MM-23853-support-…

ba39b78

…os-theme-sync

komik966 mentioned this pull request Aug 22, 2021

MM-23853 Support dark mode and light mode syncing with OS theme setting mattermost/mattermost-mobile#5635

Closed

3 tasks

Merge remote-tracking branch 'upstream/master' into MM-23853-support-…

3624ac9

…os-theme-sync

esethna added this to the v6.1.0 milestone Aug 23, 2021

Merge remote-tracking branch 'upstream/master' into MM-23853-support-…

45bb3fd

…os-theme-sync

streamer45 requested review from streamer45 and isacikgoz August 30, 2021 11:27

streamer45 added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Aug 30, 2021

mattermost deleted a comment from mattermod Sep 2, 2021

streamer45 reviewed Sep 2, 2021

View reviewed changes

streamer45 requested a review from esethna September 2, 2021 13:34

komik966 added 2 commits September 2, 2021 20:34

Merge remote-tracking branch 'upstream/master' into MM-23853-support-…

af8427b

…os-theme-sync

rename disable_theme_sync to enable_theme_sync

aec78e3

komik966 mentioned this pull request Sep 8, 2021

MM-23853 Support dark mode and light mode syncing with OS theme setting mattermost/mattermost-webapp#8804

Closed

mattermod added the Lifecycle/1:stale label Sep 13, 2021

hmhealey requested a review from streamer45 October 5, 2021 14:13

streamer45 removed the Lifecycle/1:stale label Oct 5, 2021

streamer45 approved these changes Oct 6, 2021

View reviewed changes

esethna added the Hacktoberfest label Oct 6, 2021

esethna approved these changes Oct 6, 2021

View reviewed changes

esethna removed the 1: PM Review Requires review by a product manager label Oct 6, 2021

esethna modified the milestones: v6.1.0, v6.2.0 Oct 6, 2021

isacikgoz approved these changes Oct 11, 2021

View reviewed changes

mattermod added the Lifecycle/1:stale label Oct 22, 2021

esethna removed the 2: Dev Review Requires review by a developer label Oct 26, 2021

esethna requested a review from jgilliam17 October 26, 2021 22:00

Merge remote-tracking branch 'upstream/master' into MM-23853-support-…

e15a8ac

…os-theme-sync

amyblais removed this from the v6.2.0 milestone Nov 15, 2021

esethna removed the Lifecycle/1:stale label Nov 18, 2021

mattermod added the Lifecycle/1:stale label Nov 30, 2021

esethna requested a review from laneycs February 8, 2022 22:36

laneycs approved these changes Apr 5, 2022

View reviewed changes

komik966 closed this Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-23853 Support dark mode and light mode syncing with OS theme setting #18140

MM-23853 Support dark mode and light mode syncing with OS theme setting #18140

komik966 commented Aug 14, 2021

mattermod commented Aug 14, 2021

komik966 commented Aug 14, 2021

streamer45 left a comment •

edited

streamer45 Sep 2, 2021

komik966 Sep 2, 2021

streamer45 Sep 2, 2021

komik966 Sep 2, 2021

komik966 Sep 2, 2021

komik966 Sep 2, 2021

streamer45 Sep 2, 2021

streamer45 Sep 2, 2021

isacikgoz Sep 2, 2021

komik966 Sep 2, 2021

streamer45 Sep 2, 2021

komik966 Sep 2, 2021 •

edited

streamer45 Sep 2, 2021

mattermod commented Sep 13, 2021

streamer45 left a comment

streamer45 Oct 5, 2021

komik966 Nov 1, 2021

streamer45 Nov 2, 2021

esethna left a comment

isacikgoz left a comment

mattermod commented Oct 22, 2021

esethna commented Oct 26, 2021

esethna commented Nov 17, 2021

komik966 commented Nov 17, 2021

esethna commented Nov 18, 2021 •

edited

mattermod commented Nov 30, 2021

esethna commented Feb 1, 2022

komik966 commented Apr 5, 2022

MM-23853 Support dark mode and light mode syncing with OS theme setting #18140

MM-23853 Support dark mode and light mode syncing with OS theme setting #18140

Conversation

komik966 commented Aug 14, 2021

Summary

Ticket Link

Release Note

mattermod commented Aug 14, 2021

komik966 commented Aug 14, 2021

streamer45 left a comment • edited

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

Choose a reason for hiding this comment

komik966 Sep 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattermod commented Sep 13, 2021

streamer45 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

esethna left a comment

Choose a reason for hiding this comment

isacikgoz left a comment

Choose a reason for hiding this comment

mattermod commented Oct 22, 2021

esethna commented Oct 26, 2021

esethna commented Nov 17, 2021

komik966 commented Nov 17, 2021

esethna commented Nov 18, 2021 • edited

mattermod commented Nov 30, 2021

esethna commented Feb 1, 2022

komik966 commented Apr 5, 2022

streamer45 left a comment •

edited

komik966 Sep 2, 2021 •

edited

esethna commented Nov 18, 2021 •

edited