Repository navigation
feat: markdown rendering - #10
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces Markdown rendering capabilities to the diary entry details screen, enabling rich text formatting for diary content. The implementation uses the multiplatform-markdown-renderer library with Coil 3 for image handling, including a custom fetcher for loading images from local storage.
Key Changes:
- Added Markdown rendering with image support using Material3 theme and Coil3 image transformer
- Implemented custom
LocalFileFetcherfor Coil to load images from the app's local file storage via thefileManager - Configured singleton ImageLoader with network (Ktor) and local file fetching support
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
gradle/libs.versions.toml |
Added dependency declarations for markdown-renderer (v0.27.0) and coil (v3.0.4) libraries |
composeApp/src/commonMain/kotlin/io/github/smiling_pixel/filesystem/LocalFileFetcher.kt |
New custom Coil fetcher implementing support for localfile:// URI scheme to load images from fileManager |
composeApp/src/commonMain/kotlin/io/github/smiling_pixel/ImageLoader.kt |
New ImageLoader factory function configuring Coil with local file and network fetching capabilities |
composeApp/src/commonMain/kotlin/io/github/smiling_pixel/EntryDetailsScreen.kt |
Replaced plain Text component with Markdown component for content display and added placeholder text for input fields |
composeApp/src/commonMain/kotlin/io/github/smiling_pixel/App.kt |
Initialized singleton ImageLoader factory at app startup |
composeApp/build.gradle.kts |
Added markdown and Coil library dependencies to commonMain source set |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Text( | ||
| text = entry.content, | ||
| style = MaterialTheme.typography.bodyMedium, | ||
| Markdown( |
There was a problem hiding this comment.
The Markdown component is missing a modifier parameter. In edit mode, the corresponding OutlinedTextField uses Modifier.fillMaxSize() to fill the available space. For consistency and to ensure the Markdown content is properly laid out, consider adding a modifier parameter such as modifier = Modifier.fillMaxWidth() to ensure the Markdown content has proper width constraints.
| Markdown( | |
| Markdown( | |
| modifier = Modifier.fillMaxWidth(), |
0d8766f to
830db3f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setSingletonImageLoaderFactory { context -> | ||
| getAsyncImageLoader(context) |
There was a problem hiding this comment.
The setSingletonImageLoaderFactory is called inside the App composable function without any remember or LaunchedEffect wrapper. This means it will be called on every recomposition of the App composable, which is inefficient. Consider wrapping it in a LaunchedEffect with an empty dependency list to ensure it's only called once, or moving it outside the composable to an initialization location.
| setSingletonImageLoaderFactory { context -> | |
| getAsyncImageLoader(context) | |
| LaunchedEffect(Unit) { | |
| setSingletonImageLoaderFactory { context -> | |
| getAsyncImageLoader(context) | |
| } |
| override fun list(dir: Path) = throw IOException("Not supported") | ||
| override fun listOrNull(dir: Path): List<Path>? = null | ||
| override fun openReadOnly(file: Path) = throw IOException("Not supported") | ||
| override fun openReadWrite(file: Path, mustCreate: Boolean, mustExist: Boolean) = throw IOException("Not supported") | ||
| override fun source(file: Path) = throw IOException("Not supported") | ||
| override fun sink(file: Path, mustCreate: Boolean) = throw IOException("Not supported") | ||
| override fun appendingSink(file: Path, mustExist: Boolean) = throw IOException("Not supported") | ||
| override fun createDirectory(dir: Path, mustCreate: Boolean) = throw IOException("Not supported") | ||
| override fun atomicMove(source: Path, target: Path) = throw IOException("Not supported") | ||
| override fun delete(path: Path, mustExist: Boolean) = throw IOException("Not supported") | ||
| override fun createSymlink(source: Path, target: Path) = throw IOException("Not supported") |
There was a problem hiding this comment.
The EmptyFileSystem implementation throws IOException for unsupported operations, but this exception type is imported from okio.IOException. While this is functional, consider if these operations should throw UnsupportedOperationException instead, which more accurately represents that these operations are intentionally not supported rather than an I/O failure occurred.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| text = entry.content, | ||
| style = MaterialTheme.typography.bodyMedium, | ||
| Markdown( | ||
| content = entry.content, |
There was a problem hiding this comment.
The Markdown component doesn't specify a modifier, which means it won't have any layout constraints applied. Consider adding a modifier parameter to control the layout behavior, especially since this is in a Column that could have scrolling or size requirements.
| content = entry.content, | |
| content = entry.content, | |
| modifier = Modifier.fillMaxWidth(), |
| override fun create(data: Uri, options: Options, imageLoader: ImageLoader): Fetcher? { | ||
| if (data.scheme == "localfile") { | ||
| // data.path might start with /, e.g. /image.jpg | ||
| val fileName = data.path?.trimStart('/') ?: return null |
There was a problem hiding this comment.
The Factory's create method returns null for non-matching schemes or when the path is null, but there's no validation that the fileName is not empty after trimming. An empty fileName could potentially cause issues with FileManager.read(). Consider adding validation to ensure the trimmed fileName is not empty before creating the fetcher.
| val fileName = data.path?.trimStart('/') ?: return null | |
| val fileName = data.path?.trimStart('/') ?: return null | |
| if (fileName.isBlank()) { | |
| return null | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import com.mikepenz.markdown.m3.Markdown | ||
| import com.mikepenz.markdown.coil3.Coil3ImageTransformerImpl | ||
| import kotlin.time.Clock | ||
| import kotlin.time.Instant |
There was a problem hiding this comment.
Incorrect import: This should be kotlinx.datetime.Instant, not kotlin.time.Instant. The code uses Instant.fromEpochMilliseconds() which is a method from kotlinx.datetime.Instant. Using the wrong import will cause a compilation error.
| import kotlin.time.Instant | |
| import kotlinx.datetime.Instant |
| import io.github.smiling_pixel.filesystem.FileRepository | ||
| import io.github.smiling_pixel.model.FileMetadata | ||
| import kotlinx.datetime.Instant | ||
| import kotlin.time.Instant |
There was a problem hiding this comment.
Incorrect import: This should be kotlinx.datetime.Instant, not kotlin.time.Instant. The code at line 189 uses Instant.fromEpochMilliseconds() which is a method from kotlinx.datetime.Instant. Using the wrong import will cause a compilation error.
| import kotlin.time.Instant | |
| import kotlinx.datetime.Instant |
This pull request adds Markdown rendering support with image handling to the diary entry details screen, and introduces infrastructure for asynchronous image loading from both local files and the network.