Add theme properties for candidate label/text/comment color #647

DEAN-Cherry · 2024-12-10T17:05:24Z

Adding new Theme propety candidateTextColor to control candidate text color. The default value is as same as the keyTextColor

添加新的 Theme 属性 candidateTextColor，以控制候选文本颜色。默认值与 keyTextColor 相同。

Adding new theme propety `candidateTextColor` to control candidate text color. The default value is as same as the keyTextColor

rocka

Thanks for your contribution! However I can't merge this PR in current state since it does not meet some of the design pattern in the code base. I'll explain all of my considerations.

To begin with, Theme is not an arbitary JSON object, it's a versioned structure with migration strategies:

fcitx5-android/app/src/main/java/org/fcitx/fcitx5/android/data/theme/CustomThemeSerializer.kt

Lines 55 to 79 in 78cfe81

    
           private val strategies: List<MigrationStrategy> = 
        
               // Add migrations here 
        
               listOf( 
        
                   MigrationStrategy("2.0") { 
        
                       JsonObject(it.toMutableMap().apply { 
        
                           if (get("backgroundImage") != null) { 
        
                               val popupBkgColor = if (getValue("isDark").jsonPrimitive.boolean) { 
        
                                   ThemePreset.PixelDark.popupBackgroundColor 
        
                               } else { 
        
                                   ThemePreset.PixelLight.popupBackgroundColor 
        
                               } 
        
                               put("popupBackgroundColor", JsonPrimitive(popupBkgColor)) 
        
                               put("popupTextColor", getValue("keyTextColor")) 
        
                               put("genericActiveBackgroundColor", getValue("accentKeyBackgroundColor")) 
        
                               put("genericActiveForegroundColor", getValue("accentKeyTextColor")) 
        
                           } else { 
        
                               put("popupBackgroundColor", getValue("barColor")) 
        
                               put("popupTextColor", getValue("keyTextColor")) 
        
                               put("genericActiveBackgroundColor", getValue("accentKeyBackgroundColor")) 
        
                               put("genericActiveForegroundColor", getValue("accentKeyTextColor")) 
        
                           } 
        
                       }) 
        
                   }, 
        
                   MigrationStrategy("1.0", EmptyTransform) 
        
               )

which means that you should bump the theme version and implement migration strategy when adding new properties, in order to keep compatibility with older theme files.

Additionally, I feel like it's kind of not "worth it" to bump the version for just one new property. In fact I'm also planning to add more theme properties in this version, including toolbarIconColor, candidateLabelColor, candidateWindowBorderColor and so on.

Seeing you've also submitted fcitx5-android/theme-designer#5 , I suggest following changes:

Add 2 additional theme properties candidateLabelColor and candidateCommentColor
Bump theme version to 2.1

Apply those new theme properites to CandidateView as well

fcitx5-android/app/src/main/java/org/fcitx/fcitx5/android/input/candidates/floating/LabeledCandidateItemUi.kt

Lines 29 to 42 in 78cfe81

    
           fun update(candidate: FcitxEvent.Candidate, active: Boolean) { 
        
               val fg = if (active) theme.genericActiveForegroundColor else theme.keyTextColor 
        
               val altFg = if (active) theme.genericActiveForegroundColor else theme.altKeyTextColor 
        
               root.text = buildSpannedString { 
        
                   color(fg) { append(candidate.label) } 
        
                   color(fg) { append(candidate.text) } 
        
                   if (candidate.comment.isNotBlank()) { 
        
                       append(" ") 
        
                       color(altFg) { append(candidate.comment) } 
        
                   } 
        
               } 
        
               val bg = if (active) theme.genericActiveBackgroundColor else Color.TRANSPARENT 
        
               root.backgroundColor = bg 
        
           }

- Add candidateTextColor, candidateLabelColor, and candidateCommentColor to Theme interface - Update CandidateItemUi and LabeledCandidateItemUi to use new candidate text colors - Add migration strategy for upgrading themes to version 2.1 - Update ThemePreset with default values for new candidate text colors

# Conflicts: # app/src/main/java/org/fcitx/fcitx5/android/data/theme/Theme.kt # app/src/main/java/org/fcitx/fcitx5/android/data/theme/ThemePreset.kt

DEAN-Cherry · 2024-12-27T19:26:23Z

Thank you so much for the detailed review @rocka ! I have incorporated all your suggestions and modified the code. Could you please take another look? Again, thanks a lot for your thorough review!

Under my basic test, the mirgration strategy works well. But I am a liitle confused about the migration over more than two versions. It seems I only need to add the new strategy based on the latest version and this funcation will do the rest.

fcitx5-android/app/src/main/java/org/fcitx/fcitx5/android/data/theme/CustomThemeSerializer.kt

Lines 44 to 48 in 78cfe81

    
           private fun applyStrategy(oldVersion: String, obj: JsonObject) = 
        
               strategies 
        
                   .takeWhile { it.version != oldVersion } 
        
                   .foldRight(EmptyTransform) { it, acc -> it.transformation compose acc } 
        
                   .invoke(obj)

Images

`candidateLabelColor`

`candidateCommentColor`

`candidateTextColor`

rocka

Some little suggestions: simplify the migration strategy and actually apply candidateLabelColor

app/src/main/java/org/fcitx/fcitx5/android/input/candidates/floating/LabeledCandidateItemUi.kt

app/src/main/java/org/fcitx/fcitx5/android/data/theme/CustomThemeSerializer.kt

…oating/LabeledCandidateItemUi.kt Co-authored-by: rocka <i@rocka.me>

…emeSerializer.kt Co-authored-by: rocka <i@rocka.me>

…oating/LabeledCandidateItemUi.kt Co-authored-by: rocka <i@rocka.me>

DEAN-Cherry · 2024-12-28T06:45:30Z

Thanks for your review, I didn't realize I could use candidateTextColor to specify the text color for LabelUI. I overlooked that.

also add candidate text preview on the bar related: fcitx5-android/fcitx5-android#647 Co-authored-by: Rocka <i@rocka.me>

…ndroid#647) bump theme verison to 2.1 Co-authored-by: rocka <i@rocka.me>

feat(Candidate & Theme): Adding control of Candidate Text

1135988

Adding new theme propety `candidateTextColor` to control candidate text color. The default value is as same as the keyTextColor

DEAN-Cherry changed the title ~~feat(Candidate & Theme): Adding control of Candidate Text~~ feat(Candidate & Theme): Adding control of candidate text color Dec 10, 2024

DEAN-Cherry changed the title ~~feat(Candidate & Theme): Adding control of candidate text color~~ feat(Candidate & Theme): Add control of candidate text color Dec 10, 2024

DEAN-Cherry mentioned this pull request Dec 11, 2024

feat: support candidate label/text/comment properties fcitx5-android/theme-designer#5

Merged

rocka requested changes Dec 16, 2024

View reviewed changes

DEAN-Cherry added 2 commits December 28, 2024 03:13

Merge branch 'feat/theme/candidate' into feat/candidateTextColor

75e9750

# Conflicts: # app/src/main/java/org/fcitx/fcitx5/android/data/theme/Theme.kt # app/src/main/java/org/fcitx/fcitx5/android/data/theme/ThemePreset.kt

DEAN-Cherry requested a review from rocka December 27, 2024 19:28

rocka requested changes Dec 28, 2024

View reviewed changes

DEAN-Cherry and others added 4 commits December 28, 2024 14:34

Update app/src/main/java/org/fcitx/fcitx5/android/input/candidates/fl…

83f282b

…oating/LabeledCandidateItemUi.kt Co-authored-by: rocka <i@rocka.me>

Update app/src/main/java/org/fcitx/fcitx5/android/data/theme/CustomTh…

d48d7c0

…emeSerializer.kt Co-authored-by: rocka <i@rocka.me>

Update app/src/main/java/org/fcitx/fcitx5/android/data/theme/CustomTh…

a1bba50

…emeSerializer.kt Co-authored-by: rocka <i@rocka.me>

Update app/src/main/java/org/fcitx/fcitx5/android/input/candidates/fl…

07233b7

…oating/LabeledCandidateItemUi.kt Co-authored-by: rocka <i@rocka.me>

DEAN-Cherry requested a review from rocka December 28, 2024 10:35

rocka approved these changes Dec 29, 2024

View reviewed changes

rocka changed the title ~~feat(Candidate & Theme): Add control of candidate text color~~ Add theme properties for candidate label/text/comment color Dec 29, 2024

rocka merged commit fda9ecb into fcitx5-android:master Dec 29, 2024
6 checks passed

rocka added a commit to fcitx5-android/theme-designer that referenced this pull request Dec 29, 2024

feat: support candidate label/text/comment properties (#5)

1af8ce7

also add candidate text preview on the bar related: fcitx5-android/fcitx5-android#647 Co-authored-by: Rocka <i@rocka.me>

DEAN-Cherry deleted the feat/candidateTextColor branch December 29, 2024 13:34

mokapsing pushed a commit to mokapsing/fcitx5-android that referenced this pull request Dec 30, 2024

Add theme properties for candidate label/text/comment color (fcitx5-a…

8aee3b0

…ndroid#647) bump theme verison to 2.1 Co-authored-by: rocka <i@rocka.me>

wxyzh pushed a commit to wxyzh/fcitx5-android that referenced this pull request Jan 2, 2025

Add theme properties for candidate label/text/comment color (fcitx5-a…

4fff6ca

…ndroid#647) bump theme verison to 2.1 Co-authored-by: rocka <i@rocka.me>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add theme properties for candidate label/text/comment color #647

Add theme properties for candidate label/text/comment color #647

Uh oh!

DEAN-Cherry commented Dec 10, 2024 •

edited

Loading

Uh oh!

rocka left a comment

Uh oh!

DEAN-Cherry commented Dec 27, 2024 •

edited

Loading

`candidateLabelColor`

`candidateCommentColor`

`candidateTextColor`

Uh oh!

rocka left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DEAN-Cherry commented Dec 28, 2024

Uh oh!

Uh oh!

Uh oh!

	private val strategies: List<MigrationStrategy> =
	// Add migrations here
	listOf(
	MigrationStrategy("2.0") {
	JsonObject(it.toMutableMap().apply {
	if (get("backgroundImage") != null) {
	val popupBkgColor = if (getValue("isDark").jsonPrimitive.boolean) {
	ThemePreset.PixelDark.popupBackgroundColor
	} else {
	ThemePreset.PixelLight.popupBackgroundColor
	}
	put("popupBackgroundColor", JsonPrimitive(popupBkgColor))
	put("popupTextColor", getValue("keyTextColor"))
	put("genericActiveBackgroundColor", getValue("accentKeyBackgroundColor"))
	put("genericActiveForegroundColor", getValue("accentKeyTextColor"))
	} else {
	put("popupBackgroundColor", getValue("barColor"))
	put("popupTextColor", getValue("keyTextColor"))
	put("genericActiveBackgroundColor", getValue("accentKeyBackgroundColor"))
	put("genericActiveForegroundColor", getValue("accentKeyTextColor"))
	}
	})
	},
	MigrationStrategy("1.0", EmptyTransform)
	)

	fun update(candidate: FcitxEvent.Candidate, active: Boolean) {
	val fg = if (active) theme.genericActiveForegroundColor else theme.keyTextColor
	val altFg = if (active) theme.genericActiveForegroundColor else theme.altKeyTextColor
	root.text = buildSpannedString {
	color(fg) { append(candidate.label) }
	color(fg) { append(candidate.text) }
	if (candidate.comment.isNotBlank()) {
	append(" ")
	color(altFg) { append(candidate.comment) }
	}
	}
	val bg = if (active) theme.genericActiveBackgroundColor else Color.TRANSPARENT
	root.backgroundColor = bg
	}

Add theme properties for candidate label/text/comment color #647

Add theme properties for candidate label/text/comment color #647

Uh oh!

Conversation

DEAN-Cherry commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rocka left a comment

Choose a reason for hiding this comment

Uh oh!

DEAN-Cherry commented Dec 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

candidateLabelColor

candidateCommentColor

candidateTextColor

Uh oh!

rocka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DEAN-Cherry commented Dec 28, 2024

Uh oh!

Uh oh!

Uh oh!

DEAN-Cherry commented Dec 10, 2024 •

edited

Loading

DEAN-Cherry commented Dec 27, 2024 •

edited

Loading

`candidateLabelColor`

`candidateCommentColor`

`candidateTextColor`