-
Notifications
You must be signed in to change notification settings - Fork 632
Fixes #6086: Add transparent image detection and math tag validation #6144
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
Open
nikhilkumarpanigrahi
wants to merge
26
commits into
oppia:introduce-asset-download-script
Choose a base branch
from
nikhilkumarpanigrahi:lesson-asset-validation-v4
base: introduce-asset-download-script
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 7 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
17910d7
Add transparent image detection and math tag validation for lesson as…
nikhilkumarpanigrahi 8db3e0e
Fix regex line length to comply with ktlint 100-char limit
nikhilkumarpanigrahi 2609c7f
Remove KDoc comment to match codebase style
nikhilkumarpanigrahi e27e6ce
Address review: remove SVG fallback check, fix summary, add tests
nikhilkumarpanigrahi 2ac8193
Fix RAW_LATEX_REGEX to correctly match standard Oppia math content fo…
nikhilkumarpanigrahi 50b8ace
Fix Bazel cross-module visibility for math tag validation tests
nikhilkumarpanigrahi 7c5b892
Fix ktlint formatting for MATH_TAG_REGEX multiline expression
nikhilkumarpanigrahi 5c2a9ab
Fix #6086: address review by sharing math tag parsing utility
nikhilkumarpanigrahi 9911111
Fix #6086: make math tag regex non-greedy in LocalizationTracker
nikhilkumarpanigrahi 3003dae
Fix #6086: fix scripts ktlint max-line-length issues
nikhilkumarpanigrahi e9bbb0d
Fix #6086: address reviewer follow-ups in GAE scripts
nikhilkumarpanigrahi 707c802
Address scripts static check issues for GAE proto updates
nikhilkumarpanigrahi 17b72b1
Fix oppia#6086: Address reviewer feedback - revert unrelated changes …
nikhilkumarpanigrahi 561738b
Merge remote-tracking branch 'upstream/introduce-asset-download-scrip…
nikhilkumarpanigrahi 7d97b71
Fix scripts CI failures after base sync
nikhilkumarpanigrahi 24f6060
Fix buildifier warning in scripts proto tests BUILD file
nikhilkumarpanigrahi f008503
Fix moshi runtime deps for Proguard builds
nikhilkumarpanigrahi b74ec9f
Fix static checks and shard test failures
nikhilkumarpanigrahi 9ff0341
Fix ktlint line length issues
nikhilkumarpanigrahi dfee115
Handle maven repo key fallback for aar artifacts
nikhilkumarpanigrahi db7d94e
Use textproto exemption paths in static checks
nikhilkumarpanigrahi 1ca158e
Enhance test coverage for math-tag and image validation
nikhilkumarpanigrahi cab5739
Fix CI failures in maven dependency checks
nikhilkumarpanigrahi 8759fad
Remove unrelated TestEnvironmentConfig exemption
nikhilkumarpanigrahi 825ee16
Revert "Remove unrelated TestEnvironmentConfig exemption"
nikhilkumarpanigrahi 9e6794d
Clean PR scope: revert unrelated changes from #6144
nikhilkumarpanigrahi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,19 @@ class ImageRepairer { | |
| return areImagesEqual(image1, image2) | ||
| } | ||
|
|
||
| fun hasTransparentPixels(imageData: ByteArray, extension: String): Boolean { | ||
| if (extension.equals("svg", ignoreCase = true)) return false | ||
| val image = imageData.inputStream().use { ImageIO.read(it) } ?: return false | ||
BenHenning marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (!image.colorModel.hasAlpha()) return false | ||
| for (y in 0 until image.height) { | ||
| for (x in 0 until image.width) { | ||
| val alpha = (image.getRGB(x, y) shr 24) and 0xff | ||
| if (alpha < FULLY_OPAQUE_ALPHA) return true | ||
| } | ||
|
Comment on lines
+58
to
+62
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion. I’m keeping this PR scoped to the reviewer-requested correctness fixes and not adding performance refactors here. I can follow up with a separate optimization PR for the transparent-pixel scan if needed. |
||
| } | ||
| return false | ||
| } | ||
|
|
||
| sealed class RepairedImage { | ||
| data class RenderedSvg( | ||
| val pngContents: List<Byte>, | ||
|
|
@@ -75,6 +88,7 @@ class ImageRepairer { | |
| private val WIDTH_REGEX by lazy { "width_(\\d+)".toRegex() } | ||
| private val HEIGHT_REGEX by lazy { "height_(\\d+)".toRegex() } | ||
| private val TRANSPARENT = Color(/* r = */ 0, /* g = */ 0, /* b = */ 0, /* a = */ 0) | ||
| private const val FULLY_OPAQUE_ALPHA = 255 | ||
|
|
||
| private const val REFERENCE_MONITOR_PPI = 81.589f | ||
| private const val RELATIVE_SIZE_ADJUSTMENT_FACTOR = 0.15f | ||
|
|
||
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
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
15 changes: 15 additions & 0 deletions
15
scripts/src/javatests/org/oppia/android/scripts/assets/BUILD.bazel
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| """ | ||
| Tests corresponding to asset transformation scripts. | ||
| """ | ||
|
|
||
| load("@io_bazel_rules_kotlin//kotlin:jvm.bzl", "kt_jvm_test") | ||
|
|
||
| kt_jvm_test( | ||
| name = "ImageRepairerTest", | ||
| srcs = ["ImageRepairerTest.kt"], | ||
| deps = [ | ||
| "//scripts/src/java/org/oppia/android/scripts/assets:image_repairer", | ||
| "//third_party:com_google_truth_truth", | ||
| "//third_party:org_jetbrains_kotlin_kotlin-test-junit", | ||
| ], | ||
| ) |
95 changes: 95 additions & 0 deletions
95
scripts/src/javatests/org/oppia/android/scripts/assets/ImageRepairerTest.kt
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| package org.oppia.android.scripts.assets | ||
|
|
||
| import com.google.common.truth.Truth.assertThat | ||
| import org.junit.Test | ||
| import java.awt.Color | ||
| import java.awt.image.BufferedImage | ||
| import java.io.ByteArrayOutputStream | ||
| import javax.imageio.ImageIO | ||
|
|
||
| // Function name: test names are conventionally named with underscores. | ||
| @Suppress("FunctionName") | ||
| class ImageRepairerTest { | ||
| private val imageRepairer = ImageRepairer() | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_opaqueImage_returnsFalse() { | ||
| val imageData = createPngImageData(BufferedImage.TYPE_INT_ARGB) { graphics -> | ||
| graphics.color = Color(255, 0, 0, 255) | ||
| graphics.fillRect(0, 0, 2, 2) | ||
| } | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "png")).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_fullyTransparentImage_returnsTrue() { | ||
| val imageData = createPngImageData(BufferedImage.TYPE_INT_ARGB) { graphics -> | ||
| graphics.color = Color(0, 0, 0, 0) | ||
| graphics.fillRect(0, 0, 2, 2) | ||
| } | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "png")).isTrue() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_partiallyTransparentPixel_returnsTrue() { | ||
| val imageData = createPngImageData(BufferedImage.TYPE_INT_ARGB) { graphics -> | ||
| graphics.color = Color(255, 0, 0, 128) | ||
| graphics.fillRect(0, 0, 2, 2) | ||
| } | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "png")).isTrue() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_imageWithoutAlphaChannel_returnsFalse() { | ||
| val imageData = createPngImageData(BufferedImage.TYPE_INT_RGB) { graphics -> | ||
| graphics.color = Color.RED | ||
| graphics.fillRect(0, 0, 2, 2) | ||
| } | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "png")).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_svgExtension_returnsFalse() { | ||
| val imageData = "<svg></svg>".toByteArray() | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "svg")).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_svgExtensionUpperCase_returnsFalse() { | ||
| val imageData = "<svg></svg>".toByteArray() | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "SVG")).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun testHasTransparentPixels_mixedOpaqueAndTransparent_returnsTrue() { | ||
| val image = BufferedImage(2, 2, BufferedImage.TYPE_INT_ARGB) | ||
| image.setRGB(0, 0, Color(255, 0, 0, 255).rgb) | ||
| image.setRGB(1, 0, Color(0, 255, 0, 255).rgb) | ||
| image.setRGB(0, 1, Color(0, 0, 255, 100).rgb) | ||
| image.setRGB(1, 1, Color(255, 255, 0, 255).rgb) | ||
| val imageData = ByteArrayOutputStream().also { | ||
| ImageIO.write(image, "png", it) | ||
| }.toByteArray() | ||
|
|
||
| assertThat(imageRepairer.hasTransparentPixels(imageData, "png")).isTrue() | ||
| } | ||
|
|
||
| private fun createPngImageData( | ||
| imageType: Int, | ||
| draw: (java.awt.Graphics2D) -> Unit | ||
| ): ByteArray { | ||
| val image = BufferedImage(2, 2, imageType) | ||
| val graphics = image.createGraphics() | ||
| draw(graphics) | ||
| graphics.dispose() | ||
| return ByteArrayOutputStream().also { | ||
| ImageIO.write(image, "png", it) | ||
| }.toByteArray() | ||
| } | ||
| } |
16 changes: 16 additions & 0 deletions
16
scripts/src/javatests/org/oppia/android/scripts/gae/compat/BUILD.bazel
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| """ | ||
| Tests corresponding to structure compatibility checking scripts. | ||
| """ | ||
|
|
||
| load("@io_bazel_rules_kotlin//kotlin:jvm.bzl", "kt_jvm_test") | ||
|
|
||
| kt_jvm_test( | ||
| name = "StructureCompatibilityCheckerTest", | ||
| srcs = ["StructureCompatibilityCheckerTest.kt"], | ||
| deps = [ | ||
| "//scripts/src/java/org/oppia/android/scripts/gae/compat", | ||
| "//scripts/src/java/org/oppia/android/scripts/gae/proto:localization_tracker", | ||
| "//third_party:com_google_truth_truth", | ||
| "//third_party:org_jetbrains_kotlin_kotlin-test-junit", | ||
| ], | ||
| ) |
85 changes: 85 additions & 0 deletions
85
...s/src/javatests/org/oppia/android/scripts/gae/compat/StructureCompatibilityCheckerTest.kt
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| package org.oppia.android.scripts.gae.compat | ||
|
|
||
| import com.google.common.truth.Truth.assertThat | ||
| import org.junit.Test | ||
| import org.oppia.android.scripts.gae.compat.StructureCompatibilityChecker.Companion.checkMathTagsForLatex | ||
| import org.oppia.android.scripts.gae.compat.StructureCompatibilityChecker.CompatibilityFailure.MathTagMissingRawLatex | ||
| import org.oppia.android.scripts.gae.proto.LocalizationTracker.ContainerId | ||
|
|
||
| // Function name: test names are conventionally named with underscores. | ||
| @Suppress("FunctionName") | ||
| class StructureCompatibilityCheckerTest { | ||
| private val testOrigin: ContainerId = ContainerId.Exploration("test_exp_id") | ||
| private val testContentId = "test_content_id" | ||
|
|
||
| @Test | ||
| fun testCheckMathTagsForLatex_htmlWithNoMathTags_returnsNoFailures() { | ||
| val html = "<p>Simple text with no math.</p>" | ||
|
|
||
| val failures = checkMathTagsForLatex(html, testOrigin, testContentId) | ||
|
|
||
| assertThat(failures).isEmpty() | ||
| } | ||
|
|
||
| @Test | ||
| fun testCheckMathTagsForLatex_validMathTagWithRawLatex_returnsNoFailures() { | ||
| val html = buildMathTagHtml(rawLatex = "\\frac{1}{2}") | ||
|
|
||
| val failures = checkMathTagsForLatex(html, testOrigin, testContentId) | ||
|
|
||
| assertThat(failures).isEmpty() | ||
| } | ||
|
|
||
| @Test | ||
| fun testCheckMathTagsForLatex_mathTagWithEmptyRawLatex_returnsMissingRawLatexFailure() { | ||
| val html = buildMathTagHtml(rawLatex = "") | ||
|
|
||
| val failures = checkMathTagsForLatex(html, testOrigin, testContentId) | ||
|
|
||
| assertThat(failures).hasSize(1) | ||
| assertThat(failures.first()).isInstanceOf(MathTagMissingRawLatex::class.java) | ||
| assertThat((failures.first() as MathTagMissingRawLatex).contentId) | ||
| .isEqualTo(testContentId) | ||
| } | ||
|
|
||
| @Test | ||
| fun testCheckMathTagsForLatex_mathTagWithMissingContent_returnsMissingRawLatexFailure() { | ||
| val html = | ||
| "<oppia-noninteractive-math></oppia-noninteractive-math>" | ||
|
|
||
| val failures = checkMathTagsForLatex(html, testOrigin, testContentId) | ||
|
|
||
| assertThat(failures).hasSize(1) | ||
| assertThat(failures.first()).isInstanceOf(MathTagMissingRawLatex::class.java) | ||
| } | ||
|
|
||
| @Test | ||
| fun testCheckMathTagsForLatex_multipleMathTags_oneInvalid_returnsOneFailure() { | ||
| val validTag = buildMathTagHtml(rawLatex = "x^2") | ||
| val invalidTag = | ||
| "<oppia-noninteractive-math></oppia-noninteractive-math>" | ||
| val html = "$validTag $invalidTag" | ||
|
|
||
| val failures = checkMathTagsForLatex(html, testOrigin, testContentId) | ||
|
|
||
| assertThat(failures).hasSize(1) | ||
| assertThat(failures.first()).isInstanceOf(MathTagMissingRawLatex::class.java) | ||
| } | ||
|
|
||
| @Test | ||
| fun testCheckMathTagsForLatex_multipleMathTags_allValid_returnsNoFailures() { | ||
| val tag1 = buildMathTagHtml(rawLatex = "x^2") | ||
| val tag2 = buildMathTagHtml(rawLatex = "\\sqrt{2}") | ||
| val html = "$tag1 $tag2" | ||
|
|
||
| val failures = checkMathTagsForLatex(html, testOrigin, testContentId) | ||
|
|
||
| assertThat(failures).isEmpty() | ||
| } | ||
|
|
||
| private fun buildMathTagHtml(rawLatex: String): String { | ||
| val escapedContent = "{&quot;raw_latex&quot;:&quot;$rawLatex&quot;}" | ||
| return "<oppia-noninteractive-math math_content-with-value=\"$escapedContent\">" + | ||
| "</oppia-noninteractive-math>" | ||
| } | ||
| } |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.