-
-
Notifications
You must be signed in to change notification settings - Fork 382
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
Allow Android apps to specify their own list of libraries #1610
Conversation
It just occurred to me that Possible options:
|
# list that was hard-coded in the Briefcase 0.3.16 Android template, prior to | ||
# the introduction of customizable system requirements for Android. | ||
try: | ||
dependencies = app.system_runtime_requires |
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.
Shouldn't this be app.dependencies
? Maybe this is why you couldn't import the navigation bar class.
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 just occurred to me that
dependencies
clashes with the PEP621 name
system_runtime_requires
seems like a good replacement, since it's conceptually doing the same kind of thing.
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 wasn't using this branch when I was getting the import failure; I was manually modifying the build.gradle after calling create.
But yes - this should be app.dependencies
(or whatever name we land on).
I agree that this is conceptually similar to system_runtime_requires
; however, all the variables we've added recently map as closely as possible to the "native platform naming". The benefit here is that we don't need to answer questions about how a Briefcase name maps to a platform specific name (e.g., explaining that "system_runtime_requires
maps to build.gradle
dependencies
) - the naming of the Android-specific customizations is at least slightly intuitive to Android users. My inclination is to go with gradle_dependencies
on that basis.
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.
Scratch that - for consistency with existing Android options, it should be build_gradle_dependencies
.
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, that makes sense.
@@ -180,6 +200,7 @@ def output_format_template_context(self, app: AppConfig): | |||
for path in (app.test_sources or []) | |||
if (name := Path(path).name) | |||
), | |||
"dependencies": {"implementation": dependencies}, |
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.
Since we won't be using any of the other dependency types in the foreseeable future, adding an extra implementation
layer seems unnecessary, and loses the simplicity of having the cookiecutter context match directly with the pyproject.toml structure.
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.
Unfortunately, it is needed. Cookiecutter doesn't let you pass a list as a context value - if you do, it's interpreted as a choice, and it gives you the "default" (the first item). Passing a dictionary with a single key is how you work around this; in this case, it just happens that there is a convenient name available for that key, even if we're not going to use any of the alternative key values.
base_theme = "Theme.MaterialComponents.Light.DarkActionBar" | ||
|
||
build_gradle_dependencies = [ | ||
"com.google.android.material:material:1.11.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.
The dependency list gets a lot simpler under material, because appcompat and constraintlayout are both dependencies of material.
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 it would be better to keep pinning the version of appcompat, because we use that directly in MainActivity and in toga_android, so allowing it to default to the newest version could cause unpredictable behavior changes.
On the other hand, we don't actually use constraintlayout at all. Its only reference is in the Android template at app/src/main/res/layout/activity_main.xml, but that's just an example layout which we don't use anywhere. So to avoid confusion, I suggest updating beeware/briefcase-android-gradle-template#82 to remove it.
beeware/toga#2346 has raised the prospect that we might need to add an extra library to the list in the app template. Rather than make a special case of that one library for Toga, this PR adds a general feature, and includes the extra library as part of the Toga bootstrap.
Any existing project will inherit the list of requirements that was present in the Briefcase 0.3.16 Android template.
Requires the use of beeware/briefcase-android-gradle-template#82
Refs #485.
PR Checklist: