Skip to content
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

Fix #3972: Added TextViewStyleCheck script #5599

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions scripts/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -312,3 +312,10 @@ java_binary(
main_class = "org.oppia.android.scripts.telemetry.DecodeUserStudyEventStringKt",
runtime_deps = ["//scripts/src/java/org/oppia/android/scripts/telemetry:decode_user_study_event_string_lib"],
)

kt_jvm_binary(
name = "check_textview_styles",
testonly = True,
main_class = "org.oppia.android.scripts.xml.TextViewStyleCheckKt",
runtime_deps = ["//scripts/src/java/org/oppia/android/scripts/xml:check_textview_styles"],
)
11 changes: 11 additions & 0 deletions scripts/src/java/org/oppia/android/scripts/xml/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,14 @@ kt_jvm_library(
"//scripts/src/java/org/oppia/android/scripts/common:repository_file",
],
)

kt_jvm_library(
name = "check_textview_styles",
testonly = True,
srcs = ["TextViewStyleCheck.kt"],
visibility = ["//scripts:oppia_script_binary_visibility"],
deps = [
":xml_syntax_error_handler",
"//scripts/src/java/org/oppia/android/scripts/common:repository_file",
],
)
173 changes: 173 additions & 0 deletions scripts/src/java/org/oppia/android/scripts/xml/TextViewStyleCheck.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
package org.oppia.android.scripts.xml

import org.w3c.dom.Element
import java.io.File
import javax.xml.parsers.DocumentBuilderFactory

/**
* Script to ensure all TextView elements in layout XML files use centrally managed styles.
*
* Usage:
* bazel run //scripts:check_textview_styles -- <path_to_repository_root>
*
* Arguments:
* - path_to_repository_root: The root path of the repository.
*
* Example:
* bazel run //scripts:check_textview_styles -- $(pwd)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this script should be included as part of the CI checks.

*/
fun main(vararg args: String) {
require(args.isNotEmpty()) {
"Usage: bazel run //scripts:check_textview_styles -- <path_to_repository_root>"
}

val repoRoot = File(args[0])
require(repoRoot.exists()) { "Repository root path does not exist: ${args[0]}" }

val resDir = File(repoRoot, "app/src/main/res")
require(resDir.exists()) { "Resource directory does not exist: ${resDir.path}" }

val xmlFiles = resDir.listFiles { file -> file.isDirectory && file.name.startsWith("layout") }
?.flatMap { dir -> dir.walkTopDown().filter { it.extension == "xml" } }
?: emptyList()

val styleChecker = TextViewStyleCheck(repoRoot)
styleChecker.checkFiles(xmlFiles)
}

private class TextViewStyleCheck(private val repoRoot: File) {
private val errors = mutableListOf<String>()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - val errors could have a more descriptive name, as it's quite generic and could refer to other exceptions too.

private val legacyDirectionalityWarnings = mutableListOf<String>()
private val builderFactory = DocumentBuilderFactory.newInstance()
private val styles: Map<String, Element> by lazy { loadStyles() }

private fun loadStyles(): Map<String, Element> {
val stylesFile = File(repoRoot, "app/src/main/res/values/styles.xml")
require(stylesFile.exists()) { "Styles file does not exist: ${stylesFile.path}" }

val document = builderFactory.newDocumentBuilder().parse(stylesFile)
val styleNodes = document.getElementsByTagName("style")
return (0 until styleNodes.length).associate { i ->
val element = styleNodes.item(i) as Element
element.getAttribute("name") to element
}
}
/** Checks XML files for TextView elements to ensure compliance with style requirements. */
fun checkFiles(xmlFiles: List<File>) {
xmlFiles.forEach { file -> processXmlFile(file) }
printResults()
}

private fun processXmlFile(file: File) {
val document = builderFactory.newDocumentBuilder().parse(file)
val textViewNodes = document.getElementsByTagName("TextView")

for (i in 0 until textViewNodes.length) {
val element = textViewNodes.item(i) as Element
validateTextViewElement(element, file.path)
}
}

private fun validateTextViewElement(element: Element, filePath: String) {
val styleAttribute = element.attributes.getNamedItem("style")?.nodeValue
val idAttribute = element.attributes.getNamedItem("android:id")?.nodeValue ?: "No ID"

if (!isExemptFromStyleRequirement(element)) {
if (styleAttribute?.startsWith("@style/") == true) {
validateStyle(styleAttribute, idAttribute, filePath)
} else {
errors.add("$filePath: TextView ($idAttribute) requires central style.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think including line numbers in error and warning logs could be helpful, especially when views are missing IDs.

Something like:

ERROR: Missing style attribute in file: app/src/main/res/layout/example.xml, line 45. 
WARNING: Hardcoded left/right attribute in file: app/src/main/res/layout/example.xml, line 52. Consider using start/end.

Ref: #4128 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out! I’ve addressed this in the recent changes by using the Locator from the SAX parser to track and report line numbers accurately. PTAL at the updated implementation.

}
}

checkForLegacyDirectionality(element, filePath)
}
// Validate if the referenced style exists and contains necessary RTL/LTR properties.
private fun validateStyle(styleAttribute: String, idAttribute: String, filePath: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the check for the existence of the referenced style is helpful, I wonder if it's possible to avoid redundant RTL/LTR settings checks, as they seem to repeat the same element validations multiple times.

val styleName = styleAttribute.removePrefix("@style/")
val styleElement = styles[styleName] ?: run {
errors.add("$filePath: TextView ($idAttribute) references non-existent style: $styleName")
return
}

val items = styleElement.getElementsByTagName("item")
val hasRtlProperties = (0 until items.length).any { i ->
val item = items.item(i) as Element
when (item.getAttribute("name")) {
"android:textAlignment",
"android:gravity",
"android:layoutDirection",
"android:textDirection",
"android:textSize" -> true
else -> false
}
}

if (!hasRtlProperties) {
errors.add(
"$filePath: TextView ($idAttribute) style '$styleName' lacks RTL/LTR properties"
)
}
}
// Determines if a TextView is exempt from requiring a centrally managed style.
private fun isExemptFromStyleRequirement(element: Element): Boolean {
if (element.getAttribute("android:gravity")?.contains("center") == true) return true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows TextViews with gravity=center_vertical and gravity=center_horizontal too.

@adhiamboperes, can you clarify if gravity=center_vertical and gravity=center_horizontal are also direction-neutral? Also, which of them are useful for RTL/LTR compatibility? It would be helpful to understand the considerations for using gravity=start and gravity=end as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • gravity="center_vertical": This is direction-neutral because it only affects the vertical alignment of the content within its container.
  • gravity="center_horizontal": This is not completely direction-neutral because, while "center" remains consistent in both LTR and RTL layouts, combining it with other horizontal alignment values (like start or end) could have different effects depending on the layout direction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification, @Rd4dev and @adhiamboperes. Here’s the finalized implementation and explanation, as I understand it, for handling the gravity property.:

1. Direction-Neutrality

  • gravity=center_vertical: Fully direction-neutral as it only affects vertical alignment.
  • gravity=center_horizontal: Neutral when used alone, but not with start or end, which are direction-sensitive.

2. Considerations for start and end

  • gravity=start and gravity=end: Adapt dynamically to RTL/LTR layouts, unlike left and right.

Implementation

The following function would ensure that mixed cases (e.g., center|start) are not marked as direction-neutral:

private fun isGravityDirectionNeutral(element: Element): Boolean {
    val gravity = element.getAttribute("android:gravity") ?: return false
    val gravityValues = gravity.split("|")
    val directionSensitive = setOf("start", "end", "left", "right")
    val directionNeutral = setOf("center", "center_horizontal", "center_vertical")

    return gravityValues.any { it in directionNeutral } &&
           gravityValues.none { it in directionSensitive }
}

Let me know if further refinements are needed.

if (hasDynamicVisibility(element)) return true
if (element.hasAttribute("android:textSize")) return true
Comment on lines +192 to +195
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not entirely clear on the criteria for the exemptions. I saw that gravity=center was mentioned as an allowed criterion, but could you provide more details on dynamic visibility and text size? Should their inclusion exempt them from checking for centralized styling? Apologies if I missed any requirement description on this.


return !hasDirectionalAttributes(element)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? The elements with the directionAttributes should be exempted, right? (as they include the start/end attributes)

}

private fun hasDynamicVisibility(element: Element) =
element.getAttribute("android:visibility").let { it.contains("{") && it.contains("}") }

private fun hasDirectionalAttributes(element: Element): Boolean {
val directionAttributes = listOf(
"android:layout_alignParentStart",
"android:layout_alignParentEnd",
"android:layout_toStartOf",
"android:layout_toEndOf",
"android:paddingStart",
"android:paddingEnd",
"android:layout_marginStart",
"android:layout_marginEnd"
)
return directionAttributes.any { element.hasAttribute(it) }
}

private fun checkForLegacyDirectionality(element: Element, filePath: String) {
val legacyAttributes = listOf(
"android:paddingLeft",
"android:paddingRight",
"android:layout_marginLeft",
"android:layout_marginRight",
"android:layout_alignParentLeft",
"android:layout_alignParentRight",
"android:layout_toLeftOf",
"android:layout_toRightOf"
)

val foundLegacyAttributes = legacyAttributes.filter { element.hasAttribute(it) }
if (foundLegacyAttributes.isNotEmpty()) {
legacyDirectionalityWarnings.add(
"$filePath: TextView uses legacy" +
" directional attributes: ${foundLegacyAttributes.joinToString(", ")}"
)
}
}

private fun printResults() {
if (legacyDirectionalityWarnings.isNotEmpty()) {
println("\nWarnings - Legacy directionality attributes found:")
legacyDirectionalityWarnings.forEach { println(it) }
}

if (errors.isNotEmpty()) {
println("\nTextView Style Check FAILED:")
errors.forEach { println(it) }
throw Exception("Some TextView elements do not have centrally managed styles.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - For consistency in formatting and proper signaling of failures, capitalize the exception message and use throw Exception, e.g., throw Exception("TEXTVIEW STYLE CHECK FAILED.")

ref:

errorLines.forEach { println("- $it") }
println()
}
throw Exception("STRING RESOURCE VALIDATION CHECKS FAILED")
} else println("STRING RESOURCE VALIDATION CHECKS PASSED")

} else {
println("\nTextView Style Check PASSED.")
}
}
}
Loading