Fix and improvements to Flow<String>.asMarkdownState()#564
Open
MizzleDK wants to merge 2 commits into
Open
Conversation
MizzleDK
changed the title
3 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Flow<String>.asMarkdownState() so a single underlying MarkdownStateImpl is reused across upstream emissions instead of being re-created (and the previous emitAll of a never-completing StateFlow no longer swallows subsequent updates). Also adds a sample page/viewmodel and unit tests around the new behavior.
Changes:
- Rework
Flow<String>.asMarkdownState()to share oneMarkdownStateImpl, callingupdateInput+parseper upstream value and switching the emitted state viaflatMapLatest. - Add
AsMarkdownStateTest(andkotlinx-coroutines-testdependency) covering Loading/Success emissions, empty/complex content, and incremental updates. - Add a
FlowMarkdownPage+FlowMarkdownViewModelsample (with a newFlowicon and top-bar entry) demonstratingretainStatetoggling and auto-updating content.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
multiplatform-markdown-renderer/src/commonMain/kotlin/com/mikepenz/markdown/model/MarkdownState.kt |
Rewrites asMarkdownState() to reuse a single MarkdownStateImpl via onEach + flatMapLatest. |
multiplatform-markdown-renderer/src/commonTest/kotlin/com/mikepenz/markdown/model/AsMarkdownStateTest.kt |
New tests covering loading/success behavior and incremental updates. |
multiplatform-markdown-renderer/build.gradle.kts |
Adds hard-coded kotlinx-coroutines-test:1.10.2 test dependency. |
sample/shared/src/commonMain/kotlin/com/mikepenz/markdown/sample/FlowMarkdownViewModel.kt |
New sample VM driving a MutableStateFlow<String> through asMarkdownState with retain/auto-update toggles. |
sample/shared/src/commonMain/kotlin/com/mikepenz/markdown/sample/FlowMarkdownPage.kt |
Compose page rendering the VM state with switches/buttons and a Markdown view. |
sample/shared/src/commonMain/kotlin/com/mikepenz/markdown/sample/App.kt |
Wires up a third showFlow page mode mutually exclusive with debug/licenses. |
sample/shared/src/commonMain/kotlin/com/mikepenz/markdown/sample/TopAppBar.kt |
Adds a flowClick parameter and matching IconButton. |
sample/shared/src/commonMain/kotlin/com/mikepenz/markdown/sample/icon/Flow.kt |
New three-wave Flow ImageVector icon. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+291
to
326
| val markdownState = MarkdownStateImpl( | ||
| Input( | ||
| content = "", | ||
| lookupLinks = lookupLinks, | ||
| flavour = flavour, | ||
| parser = parser, | ||
| referenceLinkHandler = referenceLinkHandler, | ||
| retainState = retainState, | ||
| ) | ||
| ) | ||
| var isFirst = true | ||
| return transform { | ||
| if (isFirst || !retainState) { | ||
| emit(State.Loading(referenceLinkHandler)) | ||
| isFirst = false | ||
| } | ||
| val markdownState = MarkdownStateImpl( | ||
|
|
||
| return onEach { content -> | ||
| // Update the state with new content | ||
| markdownState.updateInput( | ||
| Input( | ||
| content = it, | ||
| content = content, | ||
| lookupLinks = lookupLinks, | ||
| flavour = flavour, | ||
| parser = parser, | ||
| referenceLinkHandler = referenceLinkHandler, | ||
| retainState = retainState, | ||
| ) | ||
| ) | ||
|
|
||
| markdownState.parse() | ||
| emitAll(markdownState.state) | ||
| }.flatMapLatest { | ||
| // Emit all state changes from the state flow | ||
| flow { | ||
| if (isFirst) { | ||
| emit(State.Loading(referenceLinkHandler)) | ||
| isFirst = false | ||
| } | ||
| emitAll(markdownState.state) | ||
| } | ||
| } |
Comment on lines
+303
to
326
| return onEach { content -> | ||
| // Update the state with new content | ||
| markdownState.updateInput( | ||
| Input( | ||
| content = it, | ||
| content = content, | ||
| lookupLinks = lookupLinks, | ||
| flavour = flavour, | ||
| parser = parser, | ||
| referenceLinkHandler = referenceLinkHandler, | ||
| retainState = retainState, | ||
| ) | ||
| ) | ||
|
|
||
| markdownState.parse() | ||
| emitAll(markdownState.state) | ||
| }.flatMapLatest { | ||
| // Emit all state changes from the state flow | ||
| flow { | ||
| if (isFirst) { | ||
| emit(State.Loading(referenceLinkHandler)) | ||
| isFirst = false | ||
| } | ||
| emitAll(markdownState.state) | ||
| } | ||
| } |
Comment on lines
+63
to
+77
| fun setAutoUpdate(enabled: Boolean) { | ||
| _autoUpdate.value = enabled | ||
|
|
||
| if (enabled) { | ||
| autoUpdateJob = viewModelScope.launch { | ||
| while (true) { | ||
| delay(2000) | ||
| updateContent() | ||
| } | ||
| } | ||
| } else { | ||
| autoUpdateJob?.cancel() | ||
| autoUpdateJob = null | ||
| } | ||
| } |
|
|
||
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| class FlowMarkdownViewModel { | ||
| private val viewModelScope = CoroutineScope(Dispatchers.Main) |
| import kotlinx.coroutines.flow.toList | ||
| import kotlinx.coroutines.launch | ||
| import kotlinx.coroutines.runBlocking | ||
| import kotlinx.coroutines.test.advanceUntilIdle |
Comment on lines
+111
to
+166
| @Test | ||
| fun asMarkdownState_incrementalContentBuildup() = runBlocking { | ||
| // Given a flow that gradually builds up markdown content | ||
| val contentFlow = MutableStateFlow("# Title") | ||
| val states = mutableListOf<State>() | ||
|
|
||
| // Create a SINGLE asMarkdownState flow and collect from it continuously | ||
| val stateFlow = contentFlow.asMarkdownState(retainState = false) | ||
|
|
||
| // Start collecting in background | ||
| val job = launch { | ||
| stateFlow.collect { state -> | ||
| states.add(state) | ||
| } | ||
| } | ||
|
|
||
| // Wait for initial emissions (Loading + Success) - use real delay since parse uses Dispatchers.Default | ||
| delay(500) | ||
| assertTrue(states.size >= 2, "Should have at least Loading + Success for first emission, got ${states.size}: $states") | ||
| assertIs<State.Loading>(states[0], "First state should be Loading") | ||
| assertIs<State.Success>(states[1], "Second state should be Success") | ||
| assertEquals("# Title", (states[1] as State.Success).content) | ||
|
|
||
| val stateCountAfterFirst = states.size | ||
|
|
||
| // Update with more content - this tests that the flow continues to work | ||
| contentFlow.value = "# Title\n\nSome paragraph text." | ||
| delay(500) | ||
|
|
||
| // Verify we got NEW states (Loading + Success for the second emission) | ||
| assertTrue(states.size > stateCountAfterFirst, "Should have new states after second emission, got ${states.size} total states") | ||
| val newStates = states.subList(stateCountAfterFirst, states.size) | ||
| assertTrue(newStates.any { it is State.Loading }, "Should have Loading state for second update, got: $newStates") | ||
| assertTrue(newStates.any { it is State.Success && it.content.contains("paragraph text") }, | ||
| "Should have Success state with new content, got: $newStates") | ||
|
|
||
| val stateCountAfterSecond = states.size | ||
|
|
||
| // Third update | ||
| contentFlow.value = "# Title\n\nSome paragraph text.\n\n- Item 1\n- Item 2" | ||
| delay(500) | ||
|
|
||
| // Verify we got ANOTHER set of new states | ||
| assertTrue(states.size > stateCountAfterSecond, "Should have new states after third emission, got ${states.size} total states") | ||
| val thirdUpdateStates = states.subList(stateCountAfterSecond, states.size) | ||
| assertTrue(thirdUpdateStates.any { it is State.Loading }, "Should have Loading state for third update, got: $thirdUpdateStates") | ||
| val finalSuccess = thirdUpdateStates.filterIsInstance<State.Success>().lastOrNull() | ||
| assertTrue(finalSuccess != null && finalSuccess.content.contains("- Item 2"), | ||
| "Should have Success state with list items, got: $thirdUpdateStates") | ||
|
|
||
| job.cancel() | ||
|
|
||
| // Verify the pattern: we should have multiple Loading states (one per update with retainState=false) | ||
| val loadingStates = states.filterIsInstance<State.Loading>() | ||
| assertTrue(loadingStates.size >= 3, "Should have at least 3 Loading states (one per emission), got ${loadingStates.size}: $states") | ||
| } |
| } | ||
| commonTest.dependencies { | ||
| implementation(kotlin("test")) | ||
| implementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.10.2") |
Comment on lines
271
to
290
| /** | ||
| * Transforms a [Flow] of markdown content strings into a [Flow] of parsed [State] for use in non-composable contexts like view models. | ||
| * As soon as the flow is collected, it will start parsing the content, and emit the state once ready. | ||
| * | ||
| * @param lookupLinks Whether to lookup links in the parsed tree or not | ||
| * @param retainState Whether to retain the state of the [MarkdownState] or not, when the input changes | ||
| * @param flavour The [MarkdownFlavourDescriptor] to use for parsing. | ||
| * @param parser The [MarkdownParser] to use for parsing. | ||
| * @param referenceLinkHandler The [ReferenceLinkHandler] to use for storing links. | ||
| * | ||
| * @return A [Flow] of [State] that represents the parsed markdown state. | ||
| */ | ||
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| fun Flow<String>.asMarkdownState( | ||
| lookupLinks: Boolean = true, | ||
| retainState: Boolean = false, | ||
| flavour: MarkdownFlavourDescriptor = GFMFlavourDescriptor(), | ||
| parser: MarkdownParser = MarkdownParser(flavour), | ||
| referenceLinkHandler: ReferenceLinkHandler = ReferenceLinkHandlerImpl(), | ||
| ): Flow<State> { |
Comment on lines
+21
to
+23
| path( | ||
| fill = SolidColor(Color.Black) | ||
| ) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
After creating this issue, I looked into coming up with a proper solution.
I discovered two issues with
Flow<String>.asMarkdownState():emitAll(markdownState.state)). This flow never completes and means only the very first emission from the string flow will be parsed and emitted as a markdown state.MarkdownStateImplon every emission from the string flow. This can cause the markdown component to blink when it goes from Loading -> Success, and it's wasteful when you can just create a single instance and reuse / update it.Also added a demo page that allows users to update the flow and toggle
retainState. Quick video below:Screen_recording_20260512_223619.webm
Note that the parsed markdown flashes briefly when retainState is set to false. This is because of the rapid state changes (Loading -> Success). When retainState is set to true, it'll skip the Loading state and won't flash, as evident in the video.
Fixes #563
Type of change
How Has This Been Tested?
I've verified that it works as expected via emulator testing alongside console logs on every state update.
Also added a set of unit tests. The first 4 work both before and after my changes. The fifth one, which tests continuous emissions, doesn't work prior to my changes, but works after my changes.
Checklist: