-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
@AppInventorWorkerBee ok to test |
// Copyright 2009-2011 Google, All Rights reserved | ||
// Copyright 2011-2024 MIT, All rights reserved |
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.
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 { |
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.
It's not clear that these exceptions should ever be thrown in this context.
} 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) { |
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.
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); |
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.
I believe this was addressed in #3279
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 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]); |
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.
We probably don't need to log every single file getting created.
createDir(context.getPaths().getResDir(), new File(parts[0]).getParent()), | ||
new File(parts[0]).getName()); |
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.
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.
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.
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.
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.
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).
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, 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
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 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?
Yes, I think you can just simplify it to something like:
if (!name.endsWith(".xml")) {
name += ".xml"
}
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.
Yes, I will add it additionally before validation.
@ewpatton Adding xml works fine. But there is a problem with the id for these files. If the library uses some drawable or layout by its id, it does not work. The id generated by the compiler is different from the one in the R file created for the given library. AppInventor has several fixed xml files so creating an R file here is easy. Is there a way to make the compiler index the xml files with the ids we want? I have an idea to predict the index, because the index is assigned in alphabetical order in a given folder. Is there another way? |
I don't really know. Can you provide a test project with the issue so I can take a look? |
Yes, I'll try to make an example project. Maybe I described it badly here. Libraries expect indexes from the R file from their package. However, the index in R files from the library package is different than the index in the R file created in the main application class, by aapt2. Because we deliver the R file for the library as an R.java file with the extension. |
@ewpatton So, I have prepared a short presentation showing what the problem is. Let's start with the aar sample library. I've built a simple library that sets the AppCompatImageView drawable from the resource: "drawable/media3_icon_settings.xml", using the Source:
Next I built a simple extension. It uses methods from the library to set the image:
The extension also returns the index of our drawable resource as assigned by aapt2 in the R file in the application class. Error without R.java: R source:
It's R for the library, so it has a library package. We also add the xml file itself to the extension using @UsesXmls or another method to get such content in the component_build_infos.json file:
Now application: We display the indexes from the library's R class file and the application's R class file. We also have three buttons that we will use to set our drawable image in ImageView. The first button sets the image directly in the library without an external id. The next two use the id returned by the extension. After decompiling the application, we can see that it has our drawables: The file is correct: The application contains classes from our library: The R file from our library contains an index assigned to our drawable, which we assigned in the R.java class whose source I showed above. Now let's look for the R file in the main class of our application. It's here: Let's look for our drawable and its index. As you can see, it's in line 186: But unfortunately the index generated by aapt2 is different from the one we saved in the R.java file. Now let's open our application on the phone: Both identifiers of our drawable are displayed. As you can see, they match what was visible in the R files. When we click the button that sets the image directly from the resource in the library, or the button that sets the image by specifying the resource index from the R library file, we will get the error: As we can see the error indicates that the index assigned in the R.java file added to our extension is incorrect. Now let's use the second button which will use the index from the R file of our application, which was generated by aapt2: This index is valid and the image has been displayed. So the conclusion is that the index of our drawable in the R file of the library should be the same as the index in the R file of the application. But the index cannot be predicted in advance, so we cannot save it in the R.java file. Probably the R file for the library should be generated automatically during compilation by aapt2, with the correct index. Aapt2 generates R files for the androidx libraries that the app inventor has, thanks to the R.txt files placed in the appropriate paths. Now the question is what should the extension do to achieve the same as the built-in aar libraries. Project AIA: |
General items:
ant tests
passes on my machineIf your code changes how something works on the device (i.e., it affects the companion):
ucr
ucr
as the baseFurther, if you've changed the blocks language or another user-facing designer/blocks API (added a SimpleProperty, etc.):
For all other changes:
master
master
as the baseWhat 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:
Example usage:
Reference:
https://developer.android.com/guide/topics/resources/providing-resources.html
Resolves #3246 .