You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/07/05 11:30:51 UTC
[GitHub] [beam] alexeyinkin opened a new pull request, #22158: Declarative theming, Remove duplicate PlaygroundState for embedded page, Do not re-create CodeController on language change (#22147)
alexeyinkin opened a new pull request, #22158:
URL: https://github.com/apache/beam/pull/22158
Resolves #22147
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #22158: Declarative theming, Remove duplicate PlaygroundState for embedded page, Do not re-create CodeController on language change (#22147)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #22158:
URL: https://github.com/apache/beam/pull/22158#discussion_r913705885
##########
playground/frontend/pubspec.yaml:
##########
@@ -45,7 +45,10 @@ environment:
# versions available, run `flutter pub outdated`.
dependencies:
aligned_dialog: ^0.0.6
- code_text_field: ^1.0.0
+ code_text_field:
+ git:
+ url: https://github.com/BertrandBev/code_field.git
+ ref: 9e2c9fe52a69481f038f4b6609e8a0a776429437
Review Comment:
This PR relies on the recent change we made to the code editor package we are using. They are merged to the package's `master` but are not yet released as a new version on pub.dev. The maintainer is slow in communication. Is it OK to refer to the package like this for now? We tested this commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on pull request #22158: Declarative theming, Remove duplicate PlaygroundState for embedded page, Do not re-create CodeController on language change (#22147)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on PR #22158:
URL: https://github.com/apache/beam/pull/22158#issuecomment-1174970086
R: @damondouglas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem merged pull request #22158: Declarative theming, Remove duplicate PlaygroundState for embedded page, Do not re-create CodeController on language change (#22147)
Posted by GitBox <gi...@apache.org>.
pabloem merged PR #22158:
URL: https://github.com/apache/beam/pull/22158
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #22158: Declarative theming, Remove duplicate PlaygroundState for embedded page, Do not re-create CodeController on language change (#22147)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #22158:
URL: https://github.com/apache/beam/pull/22158#discussion_r915958605
##########
playground/frontend/pubspec.yaml:
##########
@@ -45,7 +45,10 @@ environment:
# versions available, run `flutter pub outdated`.
dependencies:
aligned_dialog: ^0.0.6
- code_text_field: ^1.0.0
+ code_text_field:
+ git:
+ url: https://github.com/BertrandBev/code_field.git
+ ref: 9e2c9fe52a69481f038f4b6609e8a0a776429437
Review Comment:
The maintainer has promised to publish it on pub.dev in a couple of days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on pull request #22158: Declarative theming, Remove duplicate PlaygroundState for embedded page, Do not re-create CodeController on language change (#22147)
Posted by GitBox <gi...@apache.org>.
pabloem commented on PR #22158:
URL: https://github.com/apache/beam/pull/22158#issuecomment-1178220725
lgtm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #22158: Declarative theming, Remove duplicate PlaygroundState for embedded page, Do not re-create CodeController on language change (#22147)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #22158:
URL: https://github.com/apache/beam/pull/22158#discussion_r913698903
##########
playground/frontend/lib/pages/playground/components/playground_page_providers.dart:
##########
@@ -85,16 +73,93 @@ class PlaygroundPageProviders extends StatelessWidget {
);
}
- ExampleModel? _getExample(
+ void _onExampleStateChanged(
ExampleState exampleState,
- PlaygroundState playground,
+ PlaygroundState playgroundState,
) {
+ // This property currently doubles as a flag of initialization
+ // because it is initialized when an example is ready
+ // and is filled with a null-object if not showing any example.
+ //
+ // TODO: Add a dedicated flag of initialization or make
+ // PlaygroundState listen for examples and init itself.
+ if (playgroundState.selectedExample != null) {
+ return; // Already initialized.
+ }
+
+ if (_isEmbedded()) {
+ _initEmbedded(exampleState, playgroundState);
+ } else {
+ _initNonEmbedded(exampleState, playgroundState);
+ }
+ }
+
+ bool _isEmbedded() {
+ return Uri.base.toString().contains(kIsEmbedded);
+ }
+
+ Future<void> _initEmbedded(
+ ExampleState exampleState,
+ PlaygroundState playgroundState,
+ ) async {
+ final example = _getEmbeddedExample();
+
+ if (example.path.isEmpty) {
+ String source = Uri.base.queryParameters[kSourceCode] ?? '';
+ example.setSource(source);
+ playgroundState.setExample(example);
+ } else {
+ final loadedExample = await exampleState.getExample(
+ example.path,
+ playgroundState.sdk,
+ );
+
+ final exampleWithInfo = await exampleState.loadExampleInfo(
+ loadedExample,
+ playgroundState.sdk,
+ );
+
+ playgroundState.setExample(exampleWithInfo);
+ }
+ }
+
+ ExampleModel _getEmbeddedExample() {
final examplePath = Uri.base.queryParameters[kExampleParam];
- if (exampleState.defaultExamplesMap.isEmpty) {
- exampleState.loadDefaultExamples();
+ return ExampleModel(
+ name: 'Embedded_Example',
+ path: examplePath ?? '',
+ description: '',
+ type: ExampleType.example,
+ );
+ }
+
+ Future<void> _initNonEmbedded(
+ ExampleState exampleState,
+ PlaygroundState playgroundState,
+ ) async {
+ await exampleState.loadDefaultExamplesIfNot();
Review Comment:
This change expands the earlier inline callbacks so they are more readable. The behavior is preserved except that we now await for the default examples to load. Previously, this was not awaited, so right away the function had been terminating because the default example could not be found in the map. So yet another firing of `ExampleState` was required for default example to be fetched.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #22158: Declarative theming, Remove duplicate PlaygroundState for embedded page, Do not re-create CodeController on language change (#22147)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #22158:
URL: https://github.com/apache/beam/pull/22158#discussion_r913700768
##########
playground/frontend/lib/pages/routes.dart:
##########
@@ -17,29 +17,35 @@
*/
import 'package:flutter/material.dart';
-import 'package:playground/pages/embedded_playground/embedded_page_providers.dart';
+import 'package:playground/constants/params.dart';
+import 'package:playground/pages/embedded_playground/embedded_playground_page.dart';
import 'package:playground/pages/playground/playground_page.dart';
class Routes {
- static const String playground = '/';
static const String embedded = '/embedded';
static Route<dynamic> generateRoute(RouteSettings settings) {
final name = settings.name ?? '';
final queryIndex = name.indexOf('?');
final routePath =
name.substring(0, queryIndex < 0 ? name.length : queryIndex);
+
switch (routePath) {
- case Routes.playground:
- return Routes.renderRoute(const PlaygroundPage());
Review Comment:
It is the default route, so no need to handle it separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #22158: Declarative theming, Remove duplicate PlaygroundState for embedded page, Do not re-create CodeController on language change (#22147)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #22158:
URL: https://github.com/apache/beam/pull/22158#discussion_r913702588
##########
playground/frontend/lib/pages/playground/components/editor_textarea_wrapper.dart:
##########
@@ -141,23 +134,3 @@ class CodeTextAreaWrapper extends StatelessWidget {
state.resetError();
}
}
-
-class EditorKeyObject {
Review Comment:
This class was the hack that has been dropping the state with the text editor when the user had been selecting another example, or changing the SDK, or resetting the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org