-
-
Notifications
You must be signed in to change notification settings - Fork 378
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
Add support for third-party GUI framework plugins #1524
Conversation
6ec1eb2
to
1d81262
Compare
POC Design
Notes
|
PoC success!!
Notes
Next Steps
Thoughts welcome on this approach....especially on this high-level design and scope. |
That seems like a reasonable assumption, unless we fully divest all responsibility for template app generation to the backend.
The GUI framework is probably needed for identification purposes, but I'm not 100% convinced it's needed in the template context. If we have it in the template context, then we're still going to have "if toga..." logic in the app template, which won't actually make our lives that much easier. If the framework plugin exposes all the places that are currently
I can see how this could work, but it would essentially require picking the framework first, so the right framework backend would run the app generation logic. I'm also a little wary of YAGNI here. Yes, we could allow any backend to completely customise the app generation workflow... but is that actually required? Asking additional questions, maybe - but changing the logic of how an app_name is selected? I'm having difficulty imagining why this would be required.
I'm not sure "without drawing" is actually a test. We need to validate that a GUI window can be displayed by the running app.
An interesting idea, but I wonder if that just bifurcates our maintenance overhead. For example - let's say we add a new top-level app property. Right now, we add that to the template once. If we give each framework update its own base template, we'd need to update that property on every base template. I think the better approach is to provide the mechanism to cross-mix the two. Provide a base app template that covers the common structure, but also provides the extension points so that frameworks can inject platform-specific details as required.
I agree that it would be good to avoid meta-templating... but I'm not sure how we can avoid at least some meta templating, because we need to include the app name in the generate app content.
My initial inclination is to provide them in the Briefcase repo, and register them as part of installing Briefcase. As you've noted in #1523, the one thing we don't need is more repositories :-) |
Thanks for this bit; it's definitely helped evolve my thinking on this. To that end, a lot of the plugin needs to provide content for For instance, Alternatively, the plugin could return a string of the same information and Briefcase could insert it verbatim in to the TOML. The advantage of this approach is metadata that will be difficult to capture in a Python datatype. For instance, consider
These comments are valuable to end-users to understand why it's included in their rolled out project. Therefore, my inclination is to allow plugins to do either and return the format that makes sense for them. @freakboy3742, do you have any opinions or additional insight for this before I move forward? Thanks. |
The only other thought I've had is that we could switch to tomlkit, which is a style-preserving TOML library. It allows you to annotate comments onto nodes in the TOML document... but it (a) only works on TOML, and (b) would require all plugins to also use tomlkit. However, this gets us to the point of supporting str, native types and tomlkit "DOM" nodes as inputs... which is even more complexity for even less gain. Honestly - returning a simple |
cf4677a
to
fdc9f3e
Compare
fdc9f3e
to
63818c8
Compare
023a273
to
84b6632
Compare
I pivoted this PR to just focus on adding support for third-party GUI framework plugins. A subsequent PR will build on this to support running apps from |
A few initial discussion points:
|
FWIW, I don't hate wizard as a name - my biggest objection is that it's not clear that it's the "new project" wizard (not that we have other wizards, but in theory we could).
Agreed that it would make more sense in the Whatever name we choose, I think it should align with the name we use for the entry point. I'm a little hesitant on "frameworks", because (a) it's arguable whether a GUI library is a "framework", and (b) what we're doing here is a little more that just the framework. The "Positron" use case is "Toga, but with a very specific project layout and bootstrap". "Toga" is a GUI framework; "Positron" is then a layer on top of Toga. But we need to be able to distribute "Positron" independent of "Toga", and the two need to be able to co-exist. For that matter, Toga itself might end up offering multiple plugins - one for a "simple" app, one for a Document app, one for a system tray app, and so on. I'm wondering if "bootstrap" (
Honestly, I'd be surprised if there wasn't a base class :-) Using the "default empty implementation" approach seems a reasonable approach. |
84b6632
to
e443741
Compare
This is a good choice to me; getting away from both "wizard" and "framework" sheds a lot of out-of-place connotations. This PR is stable at this point. I may add some more tests or tweak things with more testing. I now need to get I've been using this example plugin for my testing....which, IMO, also nicely demonstrates how easy it'll be to extend the default Toga project. I'm not super happy with the "bootstrap prompt" code....it isn't too terrible but it's certainly a little awkward. |
e443741
to
0825b04
Compare
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 broad strokes of this look really promising.
One idea/suggestion: What happens when a bootstrap needs an option that isn't on this list, or doesn't want to include a setting that is? Say they need to inject a Dockerfile fragment on Linux, or omit the web_style_framework
setting completely - this setup won't let the bootstrap do that. Unless our API blesses a specific setting, they can't include it; and if we bless it, it must exist.
I'm wondering if it might be better to make the backends provide the entire configuration block for a platform, rather than explicitly enumerating each setting we think is important. And then, if the bootstrap doesn't define a configuration endpoint (e.g., PySide on iOS/Android/Web), the template outputs "not supported". That way we've only got 1 API point per deployment target (6... or 12 if we explicitly include the various Linux options), rather than one for each possible setting; and the expansion path for the future will be "one API endpoint for each new platform we support", rather than "one per configuration item that we think is important".
It might also be worth passing in the Briefcase version somewhere, so that third-party plugins can adapt to settings that need to exist (or not exist) on different Briefcase versions. I guess this could be extracted from the context that is passed to constructor, but for that particular detail, it might be better to be explicit.
src/briefcase/bootstraps/base.py
Outdated
"android_supported", | ||
"web_requires", | ||
"web_supported", | ||
"web_style_framework", |
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.
This one should hopefully disappear - #1166 includes code that makes the style framework used by web deployments a feature of the GUI toolkit.
However that raises another question - details in the overall comment.
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.
Revamped bootstrap interface based on feedback.
Those are good points. I was also considering if there should be a plugin method for asking additional questions....although, technically, a plugin could drop in more user prompts anywhere. And was thinking about a plugin adding entirely new sections to the TOML....probably most useful if a plugin is also being used for a custom platform/format as well. Mostly note to myself....but I'll want to confirm how Briefcase responds to commands for a section if a plugin leaves that section blank....probably want to avoid Briefcase bombing out too badly. |
Status of the dependency among PRsAll of the PRs are dependent on each other and likely will fail if they are not all on the same branch. The CI workflows in beeware/.github#68 are currently hardcoded to always use the relevant pieces of all this development; this will obviously all need to be replaced prior to merging and I've tried to clearly mark them all in the PRs. But it should mean that the CI running for these PRs is accurate insofar as meaning everything's compatible. As for merging in to
Once those repos look good, the template PRs can go. |
cbfecdb
to
ed2c2a5
Compare
ed2c2a5
to
7d14f05
Compare
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.
Except for the beeware/.github update, this is all looking good. Marking as approved, conditional on the changes that are needed as part of the landing strategy.
In terms of the landing strategy - this is what I understand the landing strategy will be:
- Land Add the ability to override app configurations at the command line #1542, to expose the
--config
option. - Address the feedback on Add reusable actions to create and/or verify a new project .github#68. That should (I think) clear up the issues currently causing test failures on briefcase and briefcase-template; but there will still be test failures on the .github PR because the briefcase install tests won't work in their current state.
- Rerun CI on briefcase and briefcase-template. With the fixes from (2), they should pass clean.
- Push the updates to briefcase, briefcase-template and .github to reset the references to in-progress beeware/.github repos. We know this will break CI (because the repos are co-dependent), so we don't wait for a green board.
- Merge briefcase, briefcase-template and .github in rapid succession. There's a minor risk that .github might fail because we couldn't see a green board prior to merging Briefcase, but we can hot fix that as needed.
- Update the template repos to reset the beeware/.github references, wait for green CI, then merge.
- Open beverage of choice :-)
Have I missed or misunderstood anything in this?
Notwithstanding the inevitable issues from merging CI changes (especially this extensive), yeah, this is the plan. Definitely feel free to ping me to be around during the merges. |
note to self: add changenote for dropping PySide2 support. |
Ack. I'm guessing #1542 won't be merged until early next week (Monday my time, Sunday for you); but once that's there, I'll ping you before I start merging anything. If we're lucky, that will be Monday morning after the dependabot updates; but failing that, I'll put it as my "first thing in the morning" set of merges on a day that works for you. |
71699da
to
adc818f
Compare
adc818f
to
981bdaf
Compare
53d0e32
to
c0bf88f
Compare
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.
Approved for final ship.
Changes
briefcase.bootstraps
entry pointbeeware/briefcase-template
, the bootstrap must implementBaseGuiBootstrap
interface--template
Dependencies
BRIEFCASE_REPO: https://github.com/rmartin16/briefcase
BRIEFCASE_REF: run-app-in-ci-testing
BRIEFCASE_TEMPLATE_REPO: https://github.com/rmartin16/briefcase-template
BRIEFCASE_TEMPLATE_REF: gui-plugin-support
PR Checklist: