Spezi Alignment #189

pauljohanneskraft · 2025-03-12T15:59:34Z

Spezi Alignment

♻️ Current situation & Problem

This pull request improves on the current implementation to realize an aligned API with considerate deviations for a platform-native feel. It aims to align the API both more to Kotlin guidelines/best practices while providing feature parity for non-platform-specific code in the Spezi packages foundation-*, core-* and ui-*.

⚙️ Release Notes

Add a bullet point list summary of the feature and possible migration guides if this is a breaking change so this section can be added to the release notes.
Include code snippets that provide examples of the feature implemented or links to the documentation if it appends or changes the public interface.

📚 Documentation

Please ensure that you properly document any additions in conformance to Spezi Documentation Guide.
You can use this section to describe your solution, but we encourage contributors to document your reasoning and changes using in-line documentation.

✅ Testing

Please ensure that the PR meets the testing requirements set by CodeCov and that new functionality is appropriately tested.
This section describes important information about the tests and why some elements might not be testable.

📝 Code of Conduct & Contributing Guidelines

By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

I agree to follow the Code of Conduct and Contributing Guidelines.

codecov · 2025-03-12T16:17:36Z

Codecov Report

Attention: Patch coverage is 33.02658% with 655 lines in your changes missing coverage. Please review.

Project coverage is 39.59%. Comparing base (5b72cf6) to head (e40be3d).

Files with missing lines	Patch %	Lines
...ford/spezi/ui/personalinfo/OutlinedNameFieldRow.kt	23.02%	75 Missing and 22 partials ⚠️
...tanford/spezi/ui/personalinfo/BasicNameFieldRow.kt	0.00%	92 Missing ⚠️
...anford/spezi/ui/personalinfo/BasicNameTextField.kt	0.00%	83 Missing ⚠️
...edu/stanford/spezi/ui/personalinfo/NameFieldRow.kt	27.28%	52 Missing and 20 partials ⚠️
...stanford/spezi/ui/validation/ValidatedTextField.kt	27.56%	46 Missing and 25 partials ⚠️
...ord/spezi/ui/personalinfo/OutlinedNameTextField.kt	47.37%	15 Missing and 45 partials ⚠️
...du/stanford/spezi/ui/personalinfo/NameTextField.kt	51.00%	10 Missing and 39 partials ⚠️
.../spezi/ui/validation/OutlinedValidatedTextField.kt	38.03%	19 Missing and 25 partials ⚠️
...main/kotlin/edu/stanford/spezi/ui/ImageResource.kt	0.00%	28 Missing ⚠️
...otlin/edu/stanford/spezi/ui/validation/Validate.kt	59.10%	4 Missing and 5 partials ⚠️
... and 17 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #189      +/-   ##
============================================
- Coverage     40.26%   39.59%   -0.67%     
+ Complexity      866      863       -3     
============================================
  Files           295      300       +5     
  Lines         11521    12184     +663     
  Branches       1732     2022     +290     
============================================
+ Hits           4638     4823     +185     
- Misses         6425     6749     +324     
- Partials        458      612     +154

Flag	Coverage Δ
uitests	`32.00% <32.96%> (-0.80%)`	⬇️
unittests	`36.68% <3.81%> (-0.34%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../kotlin/edu/stanford/spezi/contact/ContactsList.kt	`57.15% <ø> (ø)`
.../spezi/modules/education/videos/EducationScreen.kt	`40.91% <ø> (ø)`
.../stanford/spezi/ui/personalinfo/KeyboardOptions.kt	`100.00% <100.00%> (ø)`
...ford/spezi/ui/personalinfo/PersonNameComponents.kt	`44.00% <ø> (+0.87%)`	⬆️
...u/stanford/spezi/ui/validation/CompositionLocal.kt	`100.00% <100.00%> (ø)`
...u/stanford/spezi/ui/validation/ValidationEngine.kt	`87.28% <100.00%> (+0.74%)`	⬆️
...d/spezi/ui/validation/internal/CompositionLocal.kt	`100.00% <100.00%> (ø)`
...stanford/spezi/ui/validation/internal/Constants.kt	`100.00% <100.00%> (ø)`
.../main/kotlin/edu/stanford/spezi/ui/SpeziKtTheme.kt	`0.00% <ø> (ø)`
...main/kotlin/edu/stanford/spezi/ui/SuspendButton.kt	`62.97% <ø> (-1.85%)`	⬇️
... and 27 more

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b72cf6...e40be3d. Read the comment docs.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

eldcn · 2025-03-16T09:20:29Z

ui-personalinfo/src/main/kotlin/edu/stanford/spezi/ui/personalinfo/BasicNameTextField.kt

+    BasicTextField(
+        value = value,
+        onValueChange = onValueChange,
+        modifier = modifier.testIdentifier(


I think this wrapping to just set the test identifier is not needed, on demand, the test tag can be set on caller side and not internally as this way we are making it impossible for clients to set their own test tag as we always overwrite and they would have to know the internal implementation details of this component

Yes, let's remove both all NameTextField and NameFieldRow variations, as they do not provide a lot of functionality anyways.

eldcn · 2025-03-16T09:25:45Z

ui-personalinfo/src/main/kotlin/edu/stanford/spezi/ui/personalinfo/KeyboardOptions.kt

+import androidx.compose.ui.text.input.KeyboardCapitalization
+import androidx.compose.ui.text.input.KeyboardType
+
+val KeyboardOptions.Companion.NameDefault


this can be marked internal and moved under /internal pacakge

We may want to keep exposing this as it is actually the only real override for the NameTextField - so replacing a NameTextField from Swift to Kotlin would be using a regular TextField and setting this one value.

eldcn · 2025-03-16T10:01:09Z

ui-personalinfo/src/main/kotlin/edu/stanford/spezi/ui/personalinfo/BasicNameFieldRow.kt

@@ -0,0 +1,141 @@
+package edu.stanford.spezi.ui.personalinfo


A general comment about this module (can be ignored since we want to align with iOS)

I see the whole setup of wrapping and duplicating all params of base text field components to just achieve a row layout not ideal and flexible. We would have to go for the same replications in case we would need to support a column layout of description and text fields. And again the same replications if we would need to support RTL languages.

Additionally, the description content seems a bit redundant to me and deviating from material design while all the text fields support already the label setup. While the text fields will be usually put in a column / lazy column, with this approach we can't guarantee a consistent layout unless we set fix weights to the description and text fields. See the difference between the current approach vs using material design components directly

Also, the approach with manipulating the kproperty of the builder I see not needed and not common in android. Having to do this kind of builder setup for every component as we are doing for person name is a solution that does not scale. Compose is already providing the base setup and every use case can do that directly based on its design requirements. I can share this setup that I did in heart beat in a single file / component that support three styles: https://github.com/StanfordSpezi/SpeziKt/pull/143/files#diff-2010cd3c435eaece095576f781cc7cdccc913dbeb8078505fedf39bac6621a2bR33

DescriptionGridRow, NameTextField and NameFieldRow can be removed from this package.

eldcn · 2025-03-16T10:12:09Z

ui-personalinfo/src/main/kotlin/edu/stanford/spezi/ui/personalinfo/UserProfile.kt

 import edu.stanford.spezi.ui.SpeziTheme
 import edu.stanford.spezi.ui.StringResource
 import edu.stanford.spezi.ui.ThemePreviews
 import edu.stanford.spezi.ui.lighten
 import kotlin.math.min

+interface UserProfileImageLoader {


Let's consider including coil in a follow up. We can extend our Image resource with a case data class Remote(val url ...), and handle this internally to use coils AsyncImage. It also supports api for fallbacks and place holders in case loading fails. And we dont need to create custom loaders for every use case then. For instance, we can apply it for Contact use case also

Yes, we can provide an AsyncImageResource in the ui-package and make use of it here - I would actually keep it separate from ImageResource though to make a clear separation between cases where we allow for asynchronous loading and others where async loading would need to be handled separately and we actually expect an image to be available immediately.

eldcn · 2025-03-16T10:14:54Z

ui-personalinfo/src/main/kotlin/edu/stanford/spezi/ui/personalinfo/UserProfile.kt

 ) {
    var size by remember { mutableStateOf(IntSize.Zero) }
    var loadedImage by remember(imageLoader) { mutableStateOf<ImageResource?>(null) }

-    LaunchedEffect(Unit) {
-        loadedImage = runCatching { imageLoader() }
+    LaunchedEffect(imageLoader) {


Again, this setup would not be needed if we use coil, as it will make sure to launch coroutines and take care of caching internally

eldcn · 2025-03-16T10:28:32Z

ui-validation/src/main/kotlin/edu/stanford/spezi/ui/validation/ValidationRule.kt

+    // Properties
+
+    @Suppress("detekt:VariableMinLength")
+    internal val id: UUID = UUID()


feel free to adapt detekt config file to exclude id from the variable min length rule

Fine with that

eldcn · 2025-03-16T10:29:56Z

ui-validation/src/test/kotlin/edu/stanford/spezi/ui/validation/SpeziValidationTest.kt

-        val engine = ValidationEngineImpl(rules = listOf(ValidationRule.nonEmpty))
+        val engine = ValidationEngineImpl(
+            rules = listOf(ValidationRule.nonEmpty),
+            coroutineScope = GlobalScope


We should use test scopes for tests, consider SpeziTestScope or TestScope directly

Sure, didn't know about that - sounds good!

eldcn · 2025-03-16T10:31:37Z

ui/src/main/kotlin/edu/stanford/spezi/ui/ComposableContent.kt

+import androidx.compose.runtime.Composable
+import androidx.compose.ui.Modifier
+
+interface ComposableContent {


Consider adding

@Composable fun Content() { Content(Modifier) }

in case of default modifiers, as default values are not supported in composable interface methods

eldcn · 2025-03-16T10:36:01Z

ui/src/main/kotlin/edu/stanford/spezi/ui/ImageResource.kt

+    }
+
+    @Composable
+    fun Content(modifier: Modifier, tint: Color) {


Consider introducing this typealias https://github.com/StanfordSpezi/SpeziKt/pull/143/files#diff-5e1b1e9082809c2ba95dc5c5fbdf0b7d44d4167b9c9e8ea87dd50e1076a21290R19

See doc comments there, you can have than `tint: ComposeValue get() { Colors.primary } as an interface requirement with default value, this way we can stick to the base ComposableContent interface, and can extend the type instead of introducing new methods for every config option

I imagine ImageResource to not really make assumptions about the appearance in UI, but more like an object to know how to get some image data... In the UserProfile for example, we want to use the image as a mask and then apply a custom tint that isn't provided by an app developer but instead set by spezi:ui-personalinfo.

We may want to think about having two interfaces ComposableContent and ConfigurableComposableContent<Configuration> maybe, where one allows to have a generic constraint with a given configuration to set some values during composition. In this case, we could then create a data class ImageResourceConfiguration with the tint as one of the properties - or sth like that.

eldcn · 2025-03-16T10:39:21Z

contact/src/main/kotlin/edu/stanford/spezi/contact/ContactContent.kt

 *
 * @see Contact
 * @see ContactOptionCard
 * @see AddressCard
 */
 @OptIn(ExperimentalLayoutApi::class)
 @Composable
-fun ContactComposable(contact: Contact, modifier: Modifier = Modifier) {
+fun Contact.Content(modifier: Modifier = Modifier) {


Anything against conforming Contact directly to ComposableContent? It is indeed a ui layer model as it already has ui properties such as image resource and string resources

Nope, just that we would then need to put it in the file of the Contact itself and wouldn't have separated UI code from Compose stuff, but actually I'm fine with moving it, if you want.

ui-personalinfo

b2acd99

pauljohanneskraft self-assigned this Mar 12, 2025

pauljohanneskraft added the enhancement New feature or request label Mar 12, 2025

adapt

41cce75

pauljohanneskraft added 10 commits March 12, 2025 20:03

ui-validation

2ea5512

update

aa893fc

update

2ed6211

update

1a7d11e

update

4ec1556

Fix tests

0578636

Increase code coverage by using different types of text fields

478008d

contact

124a888

contact

1abed8f

update

e40be3d

eldcn reviewed Mar 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spezi Alignment #189

Spezi Alignment #189

pauljohanneskraft commented Mar 12, 2025 •

edited

Loading

codecov bot commented Mar 12, 2025 •

edited

Loading

eldcn Mar 16, 2025

pauljohanneskraft Mar 19, 2025

eldcn Mar 16, 2025

pauljohanneskraft Mar 19, 2025

eldcn Mar 16, 2025

pauljohanneskraft Mar 19, 2025

eldcn Mar 16, 2025

pauljohanneskraft Mar 19, 2025

eldcn Mar 16, 2025

pauljohanneskraft Mar 19, 2025

eldcn Mar 16, 2025

pauljohanneskraft Mar 19, 2025

eldcn Mar 16, 2025

pauljohanneskraft Mar 19, 2025

eldcn Mar 16, 2025

eldcn Mar 16, 2025

pauljohanneskraft Mar 19, 2025

eldcn Mar 16, 2025

pauljohanneskraft Mar 19, 2025

Spezi Alignment #189

Are you sure you want to change the base?

Spezi Alignment #189

Conversation

pauljohanneskraft commented Mar 12, 2025 • edited Loading

Spezi Alignment

♻️ Current situation & Problem

⚙️ Release Notes

📚 Documentation

✅ Testing

📝 Code of Conduct & Contributing Guidelines

codecov bot commented Mar 12, 2025 • edited Loading

Codecov Report

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

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

pauljohanneskraft commented Mar 12, 2025 •

edited

Loading

codecov bot commented Mar 12, 2025 •

edited

Loading