-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Bundle Compose Hot Reload Gradle plugin into the Compose Gradle plugin #5444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2195620
to
a800d3f
Compare
gradle-plugins/compose/src/test/test-projects/application/hotReload/build.gradle
Outdated
Show resolved
Hide resolved
gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/ComposePlugin.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for the approach with the 'preferred' dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach seems elegant to me.
Note: I am not reviewing the tests, but only the mechanics of how the 'bundling' works. Using a preferred versioned dependency seems like a good solution.
4be7a02
to
4fcf98d
Compare
gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/ComposePlugin.kt
Outdated
Show resolved
Hide resolved
gradle-plugins/compose/src/test/test-projects/application/hotReload/build.gradle
Outdated
Show resolved
Hide resolved
gradle-plugins/compose/src/test/test-projects/application/hotReload/build.gradle
Outdated
Show resolved
Hide resolved
...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
Outdated
Show resolved
Hide resolved
...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
Outdated
Show resolved
Hide resolved
...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
Show resolved
Hide resolved
...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
Show resolved
Hide resolved
topics to discuss:
|
ce75c3b
to
88a3982
Compare
"3." & "4." are addressed here. |
05aada2
to
f796ba0
Compare
...ose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/configureHotReload.kt
Outdated
Show resolved
Hide resolved
34fdad0
to
0c5274f
Compare
...ose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/configureHotReload.kt
Show resolved
Hide resolved
...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
Outdated
Show resolved
Hide resolved
...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
Outdated
Show resolved
Hide resolved
...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
Outdated
Show resolved
Hide resolved
...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
Show resolved
Hide resolved
...ose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/configureHotReload.kt
Outdated
Show resolved
Hide resolved
...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
Outdated
Show resolved
Hide resolved
gradle("prepareKotlinIdeaImport").checks { | ||
gradle("prepareKotlinIdeaImport", "-P$disabaleProperty=false").checks { | ||
val expected = if (System.getProperty("os.name").lowercase().contains("windows")) { | ||
// Windows has different line endings in comparison with Unixes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ ⚠️ ⚠️ BTW! Does the CHR plugin break reproducible builds between Win and Unix machines?
Can you please elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I could imagine is that different OS create different hashes for resources (which would be a bug?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I could imagine is that different OS create different hashes for resources (which would be a bug?)
Windows has different line endings in comparison with Unixes. So vector.xml
vector drawables become binary different when are they checked out from Git and thus produce different hashes.
Last time when we discussed it, we came into conclusion that the hashes must be different, because the content is different and fixed the test by providing different "expected" directories for Windows and non-Windows. However "reproducible builds" sounds like a valid contra-argument so I would handle it in the hash calculation. "Cons" of this approach was that we had to add a custom platform-specific handling logic that sounds fragile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handled this in the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! I strongly agree that any text-based resource should produce the same hashCode regardless of the formatting used. The 'hash' here is not an indicator of the actual text hash, but a hash for its 'semantic'. It should not change if something meaningless changed (e.g., if the format of line-endings changed).
However, I wonder how projects would end up with different XML files in their project? Is there some generation happening that produces different XML files (e.g., uses different line-endings on different platforms)?
...ins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResourceAccessorsTask.kt
Outdated
Show resolved
Hide resolved
2f47039
to
1bd3f05
Compare
…to the compose gradle plugin + use version catalog for the dependency
…oduce a property to disable it. Previously introduced API for enabling its generation is removed.
…tentHash generation.
597fdf8
to
b097888
Compare
...ins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResourceAccessorsTask.kt
Outdated
Show resolved
Hide resolved
return readText().replace("\r\n", "\n").toByteArray().contentHashCode() | ||
} else { | ||
// Once a new text resource file is introduced, we have to catch it and handle its line endings. | ||
check(type == ResourceType.DRAWABLE || type == ResourceType.FONT) { "Cannot calculate content hash for $type resource type!" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you check font type?
why should binary PNGs throw the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add all type of resources in the test case, please. to check stability of the hashes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we generate accessors only for font and drawable files (strings and raw files are handled separately). Here I am just checking this fact, so if we introduce a new text resource type, we could catch it and recall that we may need to do something with its line endings. It is reflected in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add all type of resources in the test case, please.
Do you mean add .svg
and raster images?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added two more files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree. If we add a new text based resource type with a new extension, it will fail tests because hashes will be different. We don't need resource type here. And we don't need to check type == ResourceType.DRAWABLE
as well. for any file based xml/svg resource we must do conversion on win machines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
(let's hope that we don't forget to add tests for new text files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But just to double-check:
Any project would just check in those text files using a common line-ending, right? Will the git checkout then provide different line endings on different platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
Compose Hot Reload Gradle plugin is bundled with Compose Gradle plugin.
Thus Compose Hot Reload is available from Compose Multiplatform projects with the desktop target.
Fixes CMP-8885
Testing
Integration tests added.
This should be tested by QA.
Release Notes
Highlights - Desktop