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

Added @UsesXmls annotation which allows to create xml files in APK resources #3292

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

patryk84a
Copy link
Contributor

@patryk84a patryk84a commented Dec 14, 2024

General items:

If your code changes how something works on the device (i.e., it affects the companion):

  • I branched from ucr
  • My pull request has ucr as the base

Further, if you've changed the blocks language or another user-facing designer/blocks API (added a SimpleProperty, etc.):

  • I have updated the corresponding version number in appinventor/components/src/.../common/YaVersion.java
  • I have updated the corresponding upgrader in appinventor/appengine/src/.../client/youngandroid/YoungAndroidFormUpgrader.java (components only)
  • I have updated the corresponding entries in appinventor/blocklyeditor/src/versioning.js

For all other changes:

  • I branched from master
  • My pull request has master as the base

What does this PR accomplish?

I think this will solve some problems caused by lack of resources in APK.

We can only create xml files and only in these folders with different configurations:

layout, values, drawable, mipmap, xml, color, menu, animator, anim

Example usage:

@UsesXmls(xmls = {
    @XmlElement(dir = "xml",
                name = "automotive_app_desc.xml",
                content = "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n<automotiveApp>\n<uses name=\"media\"/>\n</automotiveApp>")
})

Reference:
https://developer.android.com/guide/topics/resources/providing-resources.html

Resolves #3246 .

@AppInventorWorkerBee
Copy link
Collaborator

Can one of the admins verify this patch?

@ewpatton
Copy link
Member

@AppInventorWorkerBee ok to test

Comment on lines 2 to 3
// Copyright 2009-2011 Google, All Rights reserved
// Copyright 2011-2024 MIT, All rights reserved
Copy link
Member

Choose a reason for hiding this comment

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

Note that since this is a new file you only need to have (c) 2024 MIT here.

// Transform a @XmlElement into an String for use later
// in creating xml files.
private static String xmlElementToString(XmlElement element)
throws IllegalAccessException, InvocationTargetException {
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear that these exceptions should ever be thrown in this context.

Comment on lines 1863 to 1867
} catch (IllegalAccessException e) {
messager.printMessage(Diagnostic.Kind.ERROR, "IllegalAccessException when gathering " +
"xml attributes and subelements for component " + componentInfo.name);
throw new RuntimeException(e);
} catch (InvocationTargetException e) {
Copy link
Member

Choose a reason for hiding this comment

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

See my note below about whether these exceptions can ever be thrown.

@@ -243,7 +244,7 @@ private boolean generateServices() {
private boolean generateContentProviders() {
try {
loadJsonInfo(context.getComponentInfo().getContentProvidersNeeded(),
ComponentDescriptorConstants.SERVICES_TARGET);
ComponentDescriptorConstants.CONTENT_PROVIDERS_TARGET);
Copy link
Member

Choose a reason for hiding this comment

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

I believe this was addressed in #3279

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so I'll remove this change.

for (Map.Entry<String, Set<String>> component : xmlsNeeded.entrySet()) {
for (String xml : component.getValue()) {
String[] parts = xml.split(":", 2);
context.getReporter().log("Creating " + parts[0]);
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't need to log every single file getting created.

Comment on lines +183 to +184
createDir(context.getPaths().getResDir(), new File(parts[0]).getParent()),
new File(parts[0]).getName());
Copy link
Member

Choose a reason for hiding this comment

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

We should probably do some validation of parts[0] here. My concern is that a maliciously crafted components.json might have something like "../../../../../aapt" or similar in an attempt to overwrite executable files outside of the build directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can't overwrite other files than XML because the file extension is hardcoded. UsesXmls only specifies the file name. But I assumed that the file will be saved only in a single folder nesting. I haven't tested it with a larger nesting. Maybe we need to validate by checking the first folder names and limit to a few hardcoded names like xml, drawable etc.

Copy link
Member

Choose a reason for hiding this comment

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

My point is that if you edit the JSON file you can make the filename be something else since the .xml is added at the point of the component processing when the extension is built, not at app build time, which is when this code is run. It would be less of a problem if you added the ".xml" here rather than in the component processor (which would be behavior that matches your statement).

Copy link
Contributor Author

@patryk84a patryk84a Dec 17, 2024

Choose a reason for hiding this comment

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

Ok, I've researched and tested a few things. We can create any folder but it won't be included in the android resources. We have to use android defined folders. A folder can't contain a subfolder. Names can contain lowercase and uppercase letters, numbers, underscore, dash and plus (including configuration), can't start with numbers. I will use this to return an error for the file or folder name if it is outside of this range.

path.matches("^(?:(layout|values|drawable|mipmap|xml|color|menu|animator|anim)[a-zA-Z0-9-+_]*/)?[a-z][a-zA-Z0-9-+_.]*$")

As for the .xml extension, I think to check it, if the file contains the .xml extension, we can leave it, if it does not contain the extension, I will add it. If it contains another extension, return an error or change it to xml?

Reference:
https://developer.android.com/guide/topics/resources/providing-resources.html

@patryk84a patryk84a requested a review from ewpatton December 18, 2024 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request to add a resource.
3 participants