Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 55
ADFA-2790 | Fix ScrollView parser crash on multiple children#975
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
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
068c0ea
fix(parser): prevent crash when parsing ScrollView with multiple chil…
jatezzz 3402034
fix(parser): prevent premature function exit in XML parsing loop
jatezzz d20b95d
feat: add `NestedScrollView` to `isSingleChildContainer` func
jatezzz f5b27c7
feat(layout-editor): improve xml validation to collect and display mu…
jatezzz 30070f3
fix(layout-editor): preserve IDs in nested includes and handle null root
jatezzz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
37 changes: 36 additions & 1 deletion
37 app/src/main/java/com/itsaky/androidide/actions/etc/PreviewLayoutAction.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
2 changes: 1 addition & 1 deletion
2 ...c/main/java/org/appdevforall/codeonthego/layouteditor/activities/PreviewLayoutActivity.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
5 changes: 3 additions & 2 deletions
5 layouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/editor/DesignEditor.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
127 changes: 99 additions & 28 deletions
127 ...uteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/tools/XmlLayoutParser.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 |
|---|---|---|
| @@ -5,6 +5,8 @@ import android.util.Log | ||
| import android.view.View | ||
| import android.view.ViewGroup | ||
| import android.widget.FrameLayout | ||
| import android.widget.HorizontalScrollView | ||
| import android.widget.ScrollView | ||
| import androidx.constraintlayout.widget.ConstraintLayout | ||
| import com.google.gson.Gson | ||
| import com.google.gson.reflect.TypeToken | ||
| @@ -13,6 +15,7 @@ import org.appdevforall.codeonthego.layouteditor.editor.initializer.AttributeMap | ||
| import org.appdevforall.codeonthego.layouteditor.editor.positioning.restorePositionsAfterLoad | ||
| import org.appdevforall.codeonthego.layouteditor.managers.IdManager.addNewId | ||
| import org.appdevforall.codeonthego.layouteditor.managers.IdManager.clear | ||
| import org.appdevforall.codeonthego.layouteditor.R | ||
| import org.appdevforall.codeonthego.layouteditor.utils.Constants | ||
| import org.appdevforall.codeonthego.layouteditor.utils.Constants.ATTR_INITIAL_POS | ||
| import org.appdevforall.codeonthego.layouteditor.utils.FileUtil | ||
| @@ -25,12 +28,24 @@ import org.xmlpull.v1.XmlPullParserFactory | ||
| import java.io.IOException | ||
| import java.io.File | ||
| import java.io.StringReader | ||
| import androidx.core.view.isNotEmpty | ||
| import androidx.core.widget.NestedScrollView | ||
| sealed class ValidationResult { | ||
| object Success : ValidationResult() | ||
| data class Error(val errors: List<String>) : ValidationResult() { | ||
| val formattedMessage: String get() = errors.joinToString(separator = "\n\n• ", prefix = "• ") | ||
| } | ||
| } | ||
| class XmlLayoutParser( | ||
| context: Context, | ||
| private val basePath: String? = null, | ||
| private val basePath: String? = null, | ||
| private val isRoot: Boolean = true | ||
| ) { | ||
| val viewAttributeMap: HashMap<View, AttributeMap> = HashMap() | ||
| private val validationErrors = mutableListOf<String>() | ||
| private val initializer: AttributeInitializer | ||
| private val listViews: MutableList<View> = ArrayList() | ||
| @@ -39,6 +54,7 @@ class XmlLayoutParser( | ||
| const val MARKER_IS_INCLUDE = "tools:is_xml_include" | ||
| const val MARKER_IS_FRAGMENT = "tools:is_xml_fragment" | ||
| const val MARKER_IS_MERGE = "tools:is_xml_merge" | ||
| const val TAG = "XmlLayoutParser" | ||
| } | ||
| enum class CustomAttrs(val key: String) { | ||
| @@ -64,26 +80,37 @@ class XmlLayoutParser( | ||
| val root: View? | ||
| get() = listViews.getOrNull(0) | ||
| fun parseFromXml( | ||
| fun validateXml( | ||
| xml: String, | ||
| context: Context, | ||
| ) { | ||
| ): ValidationResult { | ||
| listViews.clear() | ||
| viewAttributeMap.clear() | ||
| clear() | ||
| validationErrors.clear() | ||
| if (isRoot) clear() | ||
| try { | ||
| return try { | ||
| val factory = XmlPullParserFactory.newInstance() | ||
| val parser = factory.newPullParser() | ||
| parser.setFeature(XmlPullParser.FEATURE_PROCESS_NAMESPACES, false) | ||
| parser.setInput(StringReader(xml)) | ||
| parseFromXml(parser, context) | ||
| if (validationErrors.isEmpty()) { | ||
| ValidationResult.Success | ||
| } else { | ||
| ValidationResult.Error(validationErrors.toList()) | ||
| } | ||
| } catch (e: XmlPullParserException) { | ||
| e.printStackTrace() | ||
| ValidationResult.Error(listOf(context.getString(R.string.xml_error_parse, e.message ?: ""))) | ||
| } catch (e: IOException) { | ||
| e.printStackTrace() | ||
| ValidationResult.Error(listOf(context.getString(R.string.xml_error_io, e.message ?: ""))) | ||
| } catch (e: Exception) { | ||
| ValidationResult.Error(listOf(context.getString(R.string.xml_error_generic, e.message ?: ""))) | ||
| } | ||
| } | ||
| fun parseFromXml() { | ||
| for ((view, map) in viewAttributeMap) { | ||
| if (map.contains("android:id")) { | ||
| addNewId(view, map.getValue("android:id")) | ||
| @@ -92,6 +119,18 @@ class XmlLayoutParser( | ||
| } | ||
| } | ||
| fun processXml(xml: String, context: Context): ValidationResult { | ||
| val result = validateXml(xml, context) | ||
| if (result is ValidationResult.Success) { | ||
| parseFromXml() | ||
| } else if (result is ValidationResult.Error) { | ||
| Log.e(TAG, "Failed to parse layout. Errors:\n${result.formattedMessage}") | ||
| } | ||
| return result | ||
| } | ||
| private fun parseFromXml( | ||
| parser: XmlPullParser, | ||
| context: Context, | ||
| @@ -104,7 +143,7 @@ class XmlLayoutParser( | ||
| // Skip NavigationView to avoid invalid parent crash | ||
| if (tagName == "com.google.android.material.navigation.NavigationView") { | ||
| Log.w( | ||
| "XmlParser", | ||
| TAG, | ||
| "Skipping NavigationView tag to avoid drawer hierarchy crash", | ||
| ) | ||
| parser.next() | ||
| @@ -226,16 +265,15 @@ class XmlLayoutParser( | ||
| * view will become empty. | ||
| */ | ||
| XmlPullParser.END_TAG -> { | ||
| XmlPullParser.END_TAG -> run { | ||
| val depth = parser.depth | ||
| if (depth >= 2 && listViews.size >= 2) { | ||
| val parent = listViews.getOrNull(depth - 2) ?: return | ||
| val child = listViews.getOrNull(depth - 1) ?: return | ||
| if (parent is ViewGroup) { | ||
| parent.addView(child) | ||
| listViews.removeAt(depth - 1) | ||
| } | ||
| } | ||
| if (depth < 2 || listViews.size < 2) return@run | ||
| val parent = listViews.getOrNull(depth - 2) as? ViewGroup ?: return@run | ||
| val child = listViews.getOrNull(depth - 1) ?: return@run | ||
| parent.tryAddChild(child) | ||
| listViews.removeAt(depth - 1) | ||
| } | ||
| } | ||
| parser.next() | ||
| @@ -246,6 +284,36 @@ class XmlLayoutParser( | ||
| } | ||
| } | ||
| private fun ViewGroup.tryAddChild(child: View) { | ||
| if (isSingleChildContainer() && isNotEmpty()) { | ||
| val errorMsg = context.getString( | ||
| R.string.xml_error_single_child_container, | ||
| this::class.simpleName | ||
| ) | ||
| Log.w(TAG, errorMsg) | ||
| validationErrors.add(errorMsg) | ||
| viewAttributeMap.remove(child) | ||
| return | ||
| } | ||
| runCatching { | ||
| addView(child) | ||
| }.onFailure { e -> | ||
| val errorMsg = context.getString( | ||
| R.string.xml_error_add_child_failed, | ||
| child::class.simpleName, | ||
| this::class.simpleName | ||
| ) | ||
| Log.e(TAG, errorMsg, e) | ||
| validationErrors.add(errorMsg) | ||
| viewAttributeMap.remove(child) | ||
| } | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| private fun ViewGroup.isSingleChildContainer(): Boolean { | ||
| return this is ScrollView || this is HorizontalScrollView || this is NestedScrollView | ||
| } | ||
| private fun applyInitialPosition(target: View, attrs: AttributeMap) { | ||
| if (attrs.contains("android:layout_marginStart")) return | ||
| @@ -290,7 +358,7 @@ class XmlLayoutParser( | ||
| val attr = initializer.getAttributeFromKey(key, allAttrs) | ||
| if (attr == null) { | ||
| Log.w( | ||
| "XmlParser", | ||
| TAG, | ||
| "Could not find attribute $key for view ${target.javaClass.simpleName}", | ||
| ) | ||
| continue | ||
| @@ -300,7 +368,7 @@ class XmlLayoutParser( | ||
| val className = attr[Constants.KEY_CLASS_NAME].toString() | ||
| val value = attributeMap.getValue(key) | ||
| Log.d("applyAttributes", "Applying attribute $key to view $target with value $value") | ||
| Log.d(TAG, "Applying attribute $key to view $target with value $value") | ||
| invokeMethod(methodName, className, target, value, target.context) | ||
| } | ||
| } | ||
| @@ -369,7 +437,7 @@ class XmlLayoutParser( | ||
| ): View? { | ||
| if (layoutAttr == null || basePath == null) { | ||
| Log.w( | ||
| "XmlParser", | ||
| TAG, | ||
| "Skipping include. layoutAttr=$layoutAttr basePath=$basePath" | ||
| ) | ||
| return null | ||
| @@ -380,7 +448,7 @@ class XmlLayoutParser( | ||
| if (!file.exists()) { | ||
| Log.e( | ||
| "XmlParser", | ||
| TAG, | ||
| "Included file not found: ${file.absolutePath}" | ||
| ) | ||
| return null | ||
| @@ -389,17 +457,20 @@ class XmlLayoutParser( | ||
| return try { | ||
| val xml = file.readText() | ||
| val converted = | ||
| ConvertImportedXml(xml) | ||
| .getXmlConverted(context) | ||
| ?: xml | ||
| val converted = ConvertImportedXml(xml).getXmlConverted(context) ?: xml | ||
| val parser = XmlLayoutParser(context, basePath, false) | ||
| val result = parser.processXml(converted, context) | ||
| if (result is ValidationResult.Error) { | ||
| Log.e(TAG, "Included layout has errors: ${result.formattedMessage}") | ||
| return null | ||
| } | ||
| val parser = XmlLayoutParser(context, basePath) | ||
| parser.parseFromXml(converted, context) | ||
| parser.root | ||
| } catch (e: Exception) { | ||
| Log.e( | ||
| "XmlParser", | ||
| TAG, | ||
| "Failed to parse include: $layoutName", | ||
| e | ||
| ) | ||
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
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.