-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add ResourceContentHash annotation to generated resource accessors #5357
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
209913f
to
d8f7123
Compare
d8f7123
to
700d707
Compare
700d707
to
8bbf398
Compare
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.
Looks really promising to me!
Here are two concerns that I have:
-
I am curious about the feature flag; it seems to bring additional complexity and integration problems. I assume that the argument is binary size, but I fail to see how a few bytes compared to the actual resource would make a difference.
-
I am little concerned about the use of hash functions and the 32-bit hashes potentially colliding. I would be more relaxed if we could use 64bit and proper hashes (but thsi can be done later)
...sources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceContentHash.kt
Show resolved
Hide resolved
gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/GeneratedResClassSpec.kt
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.
LGTM but I'd like to change tests a bit
accessorBuilder.addAnnotation( | ||
AnnotationSpec.builder(resourceContentHashAnnotationClass) | ||
.useSiteTarget(AnnotationSpec.UseSiteTarget.DELEGATE) | ||
.addMember("%L", items.fold(0){acc, item -> ((acc * 31) + item.contentHash)}).build() |
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.
fix indentions, please
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.
fixed
@@ -110,7 +110,8 @@ internal abstract class GenerateResourceAccessorsTask : IdeaImportTask() { | |||
} | |||
|
|||
val type = ResourceType.fromString(typeString) ?: error("Unknown resource type: '$typeString'.") | |||
return listOf(ResourceItem(type, qualifiers, file.nameWithoutExtension.asUnderscoredIdentifier(), path)) | |||
return listOf(ResourceItem(type, qualifiers, file.nameWithoutExtension.asUnderscoredIdentifier(), path, | |||
file.readBytes().contentHashCode())) |
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.
let's put every parameter on a new line?
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.
fixed
@@ -141,7 +142,8 @@ internal abstract class GenerateResourceAccessorsTask : IdeaImportTask() { | |||
path: Path | |||
): ResourceItem { | |||
val record = ValueResourceRecord.createFromString(recordString) | |||
return ResourceItem(record.type, qualifiers, record.key.asUnderscoredIdentifier(), path, offset, size) | |||
return ResourceItem(record.type, qualifiers, record.key.asUnderscoredIdentifier(), path, | |||
record.content.hashCode(), offset, size) |
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.
here as well
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.
fixed
@@ -578,6 +578,17 @@ class ResourcesTest : GradlePluginTestBase() { | |||
} | |||
} | |||
|
|||
@Test | |||
fun testGeneratedAccessorsAnnotatedWithResourceContentHash(): Unit = with(testProject("misc/commonResourcesAnnotatedWithResourceContentHash")) { |
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.
Instead of the new test project, we discussed using an existed one (commonResources). You just need to add new expected directory: https://github.com/JetBrains/compose-multiplatform/tree/master/gradle-plugins/compose/src/test/test-projects/misc/commonResources
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.
fixed
Used by Compose Hot Reload to mark a related accessor as dirty if the hash is changed after reloading of new accessors.
…em property is set for the Gradle task.
a243779
to
ffd1d57
Compare
@delegate:ResourceContentHash(1_620_038_668) | ||
internal val Res.drawable._3_strange_name: DrawableResource by lazy { | ||
DrawableResource("drawable:_3_strange_name", setOf( | ||
ResourceItem(setOf(), "${MD}drawable/3-strange-name.xml", -1, -1), | ||
)) | ||
} | ||
|
||
@delegate:ResourceContentHash(1_620_038_668) | ||
internal val Res.drawable.camelCaseName: DrawableResource by lazy { | ||
DrawableResource("drawable:camelCaseName", setOf( | ||
ResourceItem(setOf(), "${MD}drawable/camelCaseName.xml", -1, -1), | ||
)) | ||
} | ||
|
||
@delegate:ResourceContentHash(1_620_038_668) |
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.
Is it expected that hashes are equal?
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.
As underlying files seem to be the same by content then yes, the hashes will be equal.
ffd1d57
to
81524ae
Compare
return if (extension.lowercase() == "xml") { | ||
// Text files have different line endings on Windows/*nix, | ||
// so let's ignore them in the hashcode calculation | ||
Files.readAllLines(toPath()).joinToString().hashCode() | ||
} else { |
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.
sorry, I don't understand it. how does it affect our usecase?
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.
Tests on Windows fail without this because when xml files are checked out they are not equal by content with the same files on Linux/Mac (because of different line endings).
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 same should happen for any text files, right? I think a correct way is to fix tests instead of hacks here
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
b74756c
to
1892dba
Compare
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.
Could you check how the build time is being changed?
I see, we count hashes even without the flag.
A project for the check: https://github.com/terrakok/fluentui-system-icons
to run the demo ./gradlew :demo:run
No differences noted: 1.9.0-beta03 and this version both build around 37s on my machine after |
Used by Compose Hot Reload to mark a related accessor as dirty if the hash is changed after reloading of new accessors.
Fixes CMP-8513
Release Notes
N/A