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/11/23 16:17:41 UTC
[GitHub] [beam] alexeyinkin opened a new pull request, #24338: Show error messages when Playground requests fail (#24033)
alexeyinkin opened a new pull request, #24338:
URL: https://github.com/apache/beam/pull/24338
Addresses #24033
------------------------
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/get-started-contributing/#make-the-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)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043293462
##########
playground/frontend/lib/pages/embedded_playground/screen.dart:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../standalone_playground/widgets/playground_page_providers.dart';
+import 'state.dart';
+import 'widgets/embedded_actions.dart';
+import 'widgets/embedded_appbar_title.dart';
+import 'widgets/embedded_editor.dart';
+import 'widgets/embedded_split_view.dart';
+
+const kActionsWidth = 300.0;
+const kActionsHeight = 40.0;
+
+class EmbeddedPlaygroundScreen extends StatelessWidget {
+ final EmbeddedPlaygroundNotifier notifier;
+
+ const EmbeddedPlaygroundScreen(this.notifier);
+
+ @override
+ Widget build(BuildContext context) {
+ return PlaygroundPageProviders(
+ playgroundController: notifier.playgroundController,
+ child: ToastListenerWidget(
+ child: Scaffold(
+ appBar: AppBar(
+ automaticallyImplyLeading: false,
+ title: const EmbeddedAppBarTitle(),
+ actions: const [EmbeddedActions()],
+ ),
+ body: EmbeddedSplitView(
+ first: EmbeddedEditor(isEditable: notifier.isEditable),
+ second: Container(
+ color: Theme.of(context).backgroundColor,
Review Comment:
This is a moved code and not a regression, will be addressed later.
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031057683
##########
playground/frontend/lib/modules/examples/components/example_list/example_list.dart:
##########
@@ -22,19 +22,21 @@ import 'package:playground/pages/playground/states/example_selector_state.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
-class ExampleList extends StatelessWidget {
- final ScrollController controller;
- final AnimationController animationController;
- final OverlayEntry? dropdown;
+class ExampleList extends StatefulWidget {
+ final VoidCallback onSelected;
final ExampleBase selectedExample;
const ExampleList({
- Key? key,
Review Comment:
return `key`, please
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs/snippet_save_and_share_tabs.dart:
##########
@@ -16,32 +16,59 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
import 'package:playground/modules/editor/components/share_dropdown/share_tabs/example_share_tabs.dart';
import 'package:playground_components/playground_components.dart';
-class SnippetSaveAndShareTabs extends StatelessWidget {
+class SnippetSaveAndShareTabs extends StatefulWidget {
+ final VoidCallback onError;
final PlaygroundController playgroundController;
final TabController tabController;
const SnippetSaveAndShareTabs({
super.key,
+ required this.onError,
required this.playgroundController,
required this.tabController,
});
+ @override
+ State<SnippetSaveAndShareTabs> createState() => _SnippetSaveAndShareTabsState();
+}
+
+class _SnippetSaveAndShareTabsState extends State<SnippetSaveAndShareTabs> {
+ Future<String>? _future;
+
+ @override
+ void initState() {
+ super.initState();
+ unawaited(_initSaving());
+ }
+
+ Future<void> _initSaving() async {
+ try {
+ _future = widget.playgroundController.saveSnippet();
+ await _future;
+ } on Exception catch (ex) {
+ PlaygroundComponents.toastNotifier.addException(ex);
+ widget.onError();
Review Comment:
This error creates message.
> Error while saving
I think it could be more Informative:
> Error creating shared link
##########
playground/frontend/lib/modules/examples/components/example_list/category_expansion_panel.dart:
##########
@@ -27,18 +27,15 @@ import 'package:playground_components/playground_components.dart';
class CategoryExpansionPanel extends StatelessWidget {
Review Comment:
Why these changes are valuable?
##########
playground/frontend/lib/modules/examples/components/example_list/expansion_panel_item.dart:
##########
@@ -83,8 +81,7 @@ class ExpansionPanelItem extends StatelessWidget {
}
void _closeDropdown(ExampleCache exampleCache) {
- animationController.reverse();
- dropdown?.remove();
exampleCache.changeSelectorVisibility();
Review Comment:
`exampleCache.changeSelectorVisibility()` called here and at `ExampleSelector._closeDropdown`. Suppose one of it should be removed
##########
playground/frontend/lib/modules/examples/examples_dropdown_content.dart:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:flutter_gen/gen_l10n/app_localizations.dart';
+import 'package:playground/constants/links.dart';
+import 'package:playground/constants/sizes.dart';
+import 'package:playground/modules/examples/components/example_list/example_list.dart';
+import 'package:playground/modules/examples/components/filter/filter.dart';
+import 'package:playground/modules/examples/components/search_field/search_field.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:url_launcher/url_launcher.dart';
+
+class ExamplesDropdownContent extends StatefulWidget {
+ final VoidCallback onSelected;
+ final PlaygroundController playgroundController;
+
+ const ExamplesDropdownContent({
+ required this.onSelected,
+ required this.playgroundController,
+ });
+
+ @override
+ State<ExamplesDropdownContent> createState() =>
+ _ExamplesDropdownContentState();
+}
+
+class _ExamplesDropdownContentState extends State<ExamplesDropdownContent> {
+ final _searchController = TextEditingController();
+
+ @override
+ Widget build(BuildContext context) {
+ switch (widget.playgroundController.exampleCache.catalogStatus) {
+ case LoadingStatus.error:
+ return const LoadingErrorWidget();
+
+ case LoadingStatus.loading:
+ return const LoadingIndicator();
+
+ case LoadingStatus.done:
+ return _buildLoaded();
+ }
+ }
+
+ Widget _buildLoaded() {
Review Comment:
It's good practice to use stateless widget instead of methods
##########
playground/frontend/lib/modules/examples/examples_dropdown_content.dart:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:flutter_gen/gen_l10n/app_localizations.dart';
+import 'package:playground/constants/links.dart';
+import 'package:playground/constants/sizes.dart';
+import 'package:playground/modules/examples/components/example_list/example_list.dart';
+import 'package:playground/modules/examples/components/filter/filter.dart';
+import 'package:playground/modules/examples/components/search_field/search_field.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:url_launcher/url_launcher.dart';
+
+class ExamplesDropdownContent extends StatefulWidget {
+ final VoidCallback onSelected;
+ final PlaygroundController playgroundController;
+
+ const ExamplesDropdownContent({
Review Comment:
Use key in widget constructor
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -118,8 +88,16 @@ class _ExampleSelectorState extends State<ExampleSelector>
);
}
+ Future<void> _loadCatalogIfNot(PlaygroundController controller) async {
Review Comment:
As an option :)
```
void _loadCatalogIfNot(PlaygroundController controller) async {
await controller.exampleCache.loadAllPrecompiledObjectsIfNot().catchError(
// ignore: argument_type_not_assignable_to_error_handler
PlaygroundComponents.toastNotifier.addException,
);
}
```
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/catalog_default_example_loader.dart:
##########
@@ -30,13 +31,16 @@ class CatalogDefaultExampleLoader extends ExampleLoader {
required this.exampleCache,
});
+ @override
+ Sdk get sdk => descriptor.sdk;
+
@override
Future<Example> get future async {
- if (!exampleCache.hasCatalog) {
- throw Exception('Default example requires a catalog in ExampleState');
- }
+ await Future.wait([
Review Comment:
It's not a good practice to partially "cook" object in user of it's builder. Suggest to make method `getDefaultExampleBySdk()` and call it here. This method should contain this `Future.wait`
##########
playground/frontend/playground_components/lib/src/cache/example_cache.dart:
##########
@@ -175,55 +191,61 @@ class ExampleCache extends ChangeNotifier {
);
}
- Future<void> _loadCategories() async {
- final result = await _exampleRepository.getListOfExamples(
+ Future<void> _loadAllPrecompiledObjects() async {
+ final result = await _exampleRepository.getPrecompiledObjects(
const GetPrecompiledObjectsRequest(
Review Comment:
May create empty named constructor
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/examples_loader.dart:
##########
@@ -48,58 +50,98 @@ class ExamplesLoader {
_playgroundController = value;
}
+ /// Loads examples from [descriptor]'s immediate list.
+ ///
+ /// Sets empty editor for SDKs of failed examples.
Future<void> load(ExamplesLoadingDescriptor descriptor) async {
if (_descriptor == descriptor) {
return;
}
_descriptor = descriptor;
- await Future.wait(
- descriptor.descriptors.map(
- (one) => loadOne(group: descriptor, one: one),
- ),
- );
+ final loaders = descriptor.descriptors.map(_createLoader).whereNotNull();
+
+ try {
+ final loadFutures = loaders.map(_loadOne);
+ await Future.wait(loadFutures);
+ } on Exception catch (ex) {
+ _emptyMissing(loaders);
+ throw ExampleLoadingException(ex);
+ }
final sdk = descriptor.initialSdk;
if (sdk != null) {
_playgroundController!.setSdk(sdk);
}
}
- Future<void> loadDefaultIfAny(Sdk sdk) async {
- final group = _descriptor;
- final one = group?.lazyLoadDescriptors[sdk]?.firstOrNull;
+ ExampleLoader? _createLoader(ExampleLoadingDescriptor descriptor) {
+ final loader = defaultFactory.create(
+ descriptor: descriptor,
+ exampleCache: _playgroundController!.exampleCache,
+ );
- if (group == null || one == null) {
+ if (loader == null) {
+ // TODO: Log.
+ print('Cannot create example loader for $descriptor');
+ return null;
+ }
+
+ return loader;
+ }
+
+ void _emptyMissing(Iterable<ExampleLoader> loaders) {
+ loaders.forEach(_emptyIfMissing);
+ }
+
+ Future<void> _emptyIfMissing(ExampleLoader loader) async {
+ final sdk = loader.sdk;
+
+ if (sdk == null) {
return;
}
- return loadOne(
- group: group,
- one: one,
+ _playgroundController!.setEmptyIfNotExists(
+ sdk,
+ setCurrentSdk: _shouldSetCurrentSdk(sdk),
);
}
- Future<void> loadOne({
- required ExamplesLoadingDescriptor group,
- required ExampleLoadingDescriptor one,
- }) async {
- final loader = defaultFactory.create(
- descriptor: one,
- exampleCache: _playgroundController!.exampleCache,
- );
+ Future<void> loadDefaultIfAny(Sdk sdk) async {
+ final group = _descriptor;
Review Comment:
This variable can be omitted
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/empty_example_loader.dart:
##########
@@ -20,24 +20,32 @@ import '../../cache/example_cache.dart';
import '../../models/example.dart';
import '../../models/example_base.dart';
import '../../models/example_loading_descriptors/empty_example_loading_descriptor.dart';
+import '../../models/sdk.dart';
import 'example_loader.dart';
class EmptyExampleLoader extends ExampleLoader {
final EmptyExampleLoadingDescriptor descriptor;
+ final Example _example;
- const EmptyExampleLoader({
+ EmptyExampleLoader({
required this.descriptor,
// TODO(alexeyinkin): Remove when this lands: https://github.com/dart-lang/language/issues/1813
required ExampleCache exampleCache,
- });
+ }) : _example = createExample(descriptor.sdk);
@override
- Future<Example> get future async => Example(
- name: 'Embedded_Example',
- path: '',
- sdk: descriptor.sdk,
- source: '',
- tags: [],
- type: ExampleType.example,
- );
+ Sdk get sdk => _example.sdk;
+
+ @override
+ Future<Example> get future async => _example;
+
+ static Example createExample(Sdk sdk) {
Review Comment:
I think it's better to create named constructor, because it's also used in `PlaygroundController`. It isn't responsibility of loader to build objects
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/examples_loader.dart:
##########
@@ -48,58 +50,98 @@ class ExamplesLoader {
_playgroundController = value;
}
+ /// Loads examples from [descriptor]'s immediate list.
+ ///
+ /// Sets empty editor for SDKs of failed examples.
Future<void> load(ExamplesLoadingDescriptor descriptor) async {
if (_descriptor == descriptor) {
return;
}
_descriptor = descriptor;
- await Future.wait(
- descriptor.descriptors.map(
- (one) => loadOne(group: descriptor, one: one),
- ),
- );
+ final loaders = descriptor.descriptors.map(_createLoader).whereNotNull();
+
+ try {
+ final loadFutures = loaders.map(_loadOne);
+ await Future.wait(loadFutures);
+ } on Exception catch (ex) {
+ _emptyMissing(loaders);
+ throw ExampleLoadingException(ex);
+ }
final sdk = descriptor.initialSdk;
if (sdk != null) {
_playgroundController!.setSdk(sdk);
}
}
- Future<void> loadDefaultIfAny(Sdk sdk) async {
- final group = _descriptor;
- final one = group?.lazyLoadDescriptors[sdk]?.firstOrNull;
+ ExampleLoader? _createLoader(ExampleLoadingDescriptor descriptor) {
+ final loader = defaultFactory.create(
+ descriptor: descriptor,
+ exampleCache: _playgroundController!.exampleCache,
+ );
- if (group == null || one == null) {
+ if (loader == null) {
+ // TODO: Log.
+ print('Cannot create example loader for $descriptor');
+ return null;
+ }
+
+ return loader;
+ }
+
+ void _emptyMissing(Iterable<ExampleLoader> loaders) {
+ loaders.forEach(_emptyIfMissing);
+ }
+
+ Future<void> _emptyIfMissing(ExampleLoader loader) async {
+ final sdk = loader.sdk;
+
+ if (sdk == null) {
return;
}
- return loadOne(
- group: group,
- one: one,
+ _playgroundController!.setEmptyIfNotExists(
+ sdk,
+ setCurrentSdk: _shouldSetCurrentSdk(sdk),
);
}
- Future<void> loadOne({
- required ExamplesLoadingDescriptor group,
- required ExampleLoadingDescriptor one,
- }) async {
- final loader = defaultFactory.create(
- descriptor: one,
- exampleCache: _playgroundController!.exampleCache,
- );
+ Future<void> loadDefaultIfAny(Sdk sdk) async {
+ final group = _descriptor;
+ final one = group?.lazyLoadDescriptors[sdk]?.firstOrNull;
+ if (group == null || one == null) {
+ return;
+ }
+
+ final loader = _createLoader(one);
if (loader == null) {
- // TODO: Log.
- print('Cannot create example loader for $one');
return;
}
+ await _loadOne(loader);
+ }
+
+ Future<void> _loadOne(ExampleLoader loader) async {
final example = await loader.future;
_playgroundController!.setExample(
example,
- setCurrentSdk:
- example.sdk == group.initialSdk || group.initialSdk == null,
+ setCurrentSdk: _shouldSetCurrentSdk(example.sdk),
);
}
+
+ bool _shouldSetCurrentSdk(Sdk sdk) {
+ final descriptor = _descriptor;
Review Comment:
this variable can be omitted
##########
playground/frontend/playground_components/test/src/controllers/example_loaders/examples_loader_test.dart:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter_test/flutter_test.dart';
+import 'package:mockito/annotations.dart';
+import 'package:mockito/mockito.dart';
+import 'package:playground_components/playground_components.dart';
+
+import 'common.dart';
+import 'examples_loader_test.mocks.dart';
+
+@GenerateMocks([PlaygroundController, ExampleCache])
+void main() async {
+ late ExamplesLoader examplesLoader;
+ late MockPlaygroundController playgroundController;
+ final setExampleTrue = <String>[];
+ final setExampleFalse = <String>[];
+ final setEmptyIfNotExistsTrue = <Sdk>[];
+ final setEmptyIfNotExistsFalse = <Sdk>[];
+
+ setUp(() {
+ setExampleTrue.clear();
+ setExampleFalse.clear();
+ setEmptyIfNotExistsTrue.clear();
+ setEmptyIfNotExistsFalse.clear();
+
+ playgroundController = MockPlaygroundController();
+
+ when(playgroundController.setExample(any, setCurrentSdk: true))
+ .thenAnswer((realInvocation) {
+ final example = realInvocation.positionalArguments[0] as Example;
+ setExampleTrue.add(example.name);
+ });
+ when(playgroundController.setExample(any, setCurrentSdk: false))
+ .thenAnswer((realInvocation) {
+ final example = realInvocation.positionalArguments[0] as Example;
+ setExampleFalse.add(example.name);
+ });
+ when(playgroundController.setEmptyIfNotExists(any, setCurrentSdk: true))
+ .thenAnswer((realInvocation) {
+ final sdk = realInvocation.positionalArguments[0] as Sdk;
+ setEmptyIfNotExistsTrue.add(sdk);
+ });
+ when(playgroundController.setEmptyIfNotExists(any, setCurrentSdk: false))
+ .thenAnswer((realInvocation) {
+ final sdk = realInvocation.positionalArguments[0] as Sdk;
+ setEmptyIfNotExistsFalse.add(sdk);
+ });
+
+ final exampleCache = MockExampleCache();
+ when(playgroundController.exampleCache).thenReturn(exampleCache);
+
+ examplesLoader = ExamplesLoader();
+ examplesLoader.setPlaygroundController(playgroundController);
+ TestExampleLoader.register(examplesLoader.defaultFactory);
+ });
+
+ group('ExamplesLoader.', () {
+ group('load.', () {
+ group('Success.', () {
+ test('Race to set current SDK if not set', () async {
+ final descriptor = ExamplesLoadingDescriptor(
+ descriptors: const [
+ TestExampleLoadingDescriptor(Sdk.go),
+ TestExampleLoadingDescriptor(Sdk.python),
+ ],
+ lazyLoadDescriptors: {
+ Sdk.scio: const [TestExampleLoadingDescriptor(Sdk.scio)],
+ },
+ );
+
+ await examplesLoader.load(descriptor);
+
+ expect(setExampleTrue, [Sdk.go.id, Sdk.python.id]);
+ expect(setExampleFalse, []);
+ });
+
+ test('Example with initialSdk sets the current SDK', () async {
+ final descriptor = ExamplesLoadingDescriptor(
+ descriptors: const [
+ TestExampleLoadingDescriptor(Sdk.go),
+ TestExampleLoadingDescriptor(Sdk.python),
+ ],
+ lazyLoadDescriptors: {
+ Sdk.scio: const [TestExampleLoadingDescriptor(Sdk.scio)],
+ },
+ initialSdk: Sdk.python,
+ );
+
+ await examplesLoader.load(descriptor);
+
+ expect(setExampleTrue, [Sdk.python.id]);
+ expect(setExampleFalse, [Sdk.go.id]);
+ });
+ });
+
+ group('Error.', () {
+ test('Load empty example instead', () async {
+ Exception? thrown;
+ const descriptor = ExamplesLoadingDescriptor(
+ descriptors: [
+ TestExampleLoadingDescriptor(Sdk.go, ok: false),
+ TestExampleLoadingDescriptor(Sdk.python, ok: false),
+ ],
+ initialSdk: Sdk.python,
+ );
+
+ try {
+ await examplesLoader.load(descriptor);
+ } on ExampleLoadingException catch (ex) {
+ thrown = ex;
+ }
+
+ expect(thrown, isA<Exception>());
+ expect(setEmptyIfNotExistsTrue, [Sdk.python]);
+ expect(setEmptyIfNotExistsFalse, [Sdk.go]);
+ });
+ });
+
+ // TODO(alexeyinkin): Test lazy loading.
Review Comment:
please add issue
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/user_shared_example_loader.dart:
##########
@@ -19,18 +19,24 @@
import '../../cache/example_cache.dart';
import '../../models/example.dart';
import '../../models/example_loading_descriptors/user_shared_example_loading_descriptor.dart';
+import '../../models/sdk.dart';
import 'example_loader.dart';
class UserSharedExampleLoader extends ExampleLoader {
final UserSharedExampleLoadingDescriptor descriptor;
final ExampleCache exampleCache;
+ Example? _example;
UserSharedExampleLoader({
required this.descriptor,
required this.exampleCache,
});
@override
- Future<Example> get future =>
- exampleCache.loadSharedExample(descriptor.snippetId);
+ Sdk? get sdk => _example?.sdk;
+
+ @override
+ Future<Example> get future async {
Review Comment:
What if example already loaded? I think we should check on null here and return example if not
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -16,23 +16,21 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
-import 'package:flutter_gen/gen_l10n/app_localizations.dart';
-import 'package:playground/constants/links.dart';
import 'package:playground/constants/sizes.dart';
-import 'package:playground/modules/examples/components/examples_components.dart';
import 'package:playground/modules/examples/components/outside_click_handler.dart';
+import 'package:playground/modules/examples/examples_dropdown_content.dart';
import 'package:playground/modules/examples/models/popover_state.dart';
import 'package:playground/pages/playground/states/example_selector_state.dart';
import 'package:playground/utils/dropdown_utils.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
-import 'package:url_launcher/url_launcher.dart';
const int kAnimationDurationInMilliseconds = 80;
const Offset kAnimationBeginOffset = Offset(0.0, -0.02);
Review Comment:
If the animation really need to be removed, these constants should be moved closer to their using
##########
playground/frontend/lib/modules/examples/examples_dropdown_content.dart:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:flutter_gen/gen_l10n/app_localizations.dart';
+import 'package:playground/constants/links.dart';
+import 'package:playground/constants/sizes.dart';
+import 'package:playground/modules/examples/components/example_list/example_list.dart';
+import 'package:playground/modules/examples/components/filter/filter.dart';
+import 'package:playground/modules/examples/components/search_field/search_field.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:url_launcher/url_launcher.dart';
+
+class ExamplesDropdownContent extends StatefulWidget {
+ final VoidCallback onSelected;
+ final PlaygroundController playgroundController;
+
+ const ExamplesDropdownContent({
+ required this.onSelected,
+ required this.playgroundController,
Review Comment:
Could provided by consumer
##########
playground/frontend/lib/pages/playground/components/playground_page_providers.dart:
##########
@@ -34,9 +35,8 @@ class PlaygroundPageProviders extends StatelessWidget {
final Widget child;
const PlaygroundPageProviders({
- Key? key,
Review Comment:
Why are you removed keys? :) Linter creates new hints after that
##########
playground/frontend/lib/modules/messages/handlers/set_content_message_handler.dart:
##########
@@ -34,11 +37,21 @@ class SetContentMessageHandler extends AbstractMessageHandler {
return MessageHandleResult.notHandled;
}
- _handle(message);
+ unawaited(_handle(message));
return MessageHandleResult.handled;
}
- void _handle(SetContentMessage message) {
- playgroundController.examplesLoader.load(message.descriptor);
+ Future<void> _handle(SetContentMessage message) async {
+ final descriptor = message.descriptor;
+
+ try {
+ await playgroundController.examplesLoader.load(descriptor);
+ } on Exception catch (ex) {
+ PlaygroundComponents.toastNotifier.addException(ex);
Review Comment:
I think, creating toasts is not responsibility of request's users, because at least it will break DRY principle, if one request will use several another classes. May be move toast notifier to `examplesLoader`?
##########
playground/frontend/lib/modules/examples/examples_dropdown_content.dart:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:flutter_gen/gen_l10n/app_localizations.dart';
+import 'package:playground/constants/links.dart';
+import 'package:playground/constants/sizes.dart';
+import 'package:playground/modules/examples/components/example_list/example_list.dart';
+import 'package:playground/modules/examples/components/filter/filter.dart';
+import 'package:playground/modules/examples/components/search_field/search_field.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:url_launcher/url_launcher.dart';
+
+class ExamplesDropdownContent extends StatefulWidget {
+ final VoidCallback onSelected;
+ final PlaygroundController playgroundController;
+
+ const ExamplesDropdownContent({
+ required this.onSelected,
+ required this.playgroundController,
+ });
+
+ @override
+ State<ExamplesDropdownContent> createState() =>
+ _ExamplesDropdownContentState();
+}
+
+class _ExamplesDropdownContentState extends State<ExamplesDropdownContent> {
+ final _searchController = TextEditingController();
+
+ @override
+ Widget build(BuildContext context) {
+ switch (widget.playgroundController.exampleCache.catalogStatus) {
+ case LoadingStatus.error:
+ return const LoadingErrorWidget();
+
+ case LoadingStatus.loading:
+ return const LoadingIndicator();
+
+ case LoadingStatus.done:
+ return _buildLoaded();
+ }
+ }
+
+ Widget _buildLoaded() {
+ return Column(
+ children: [
+ SearchField(controller: _searchController),
+ const ExamplesFilter(),
+ ExampleList(
+ onSelected: widget.onSelected,
+ selectedExample: widget.playgroundController.selectedExample!,
Review Comment:
I think if can be situation when selected example is null, we should pass nullable value here and add empty state at `ExampleList`
##########
playground/frontend/lib/modules/examples/models/example_loading_descriptors/examples_loading_descriptor_factory.dart:
##########
@@ -24,7 +24,7 @@ import 'package:playground/modules/examples/models/example_token_type.dart';
import 'package:playground_components/playground_components.dart';
class ExamplesLoadingDescriptorFactory {
- static const _defaultSdk = Sdk.java;
+ static const defaultSdk = Sdk.java;
Review Comment:
Suggest to move to more general file, `params`
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -118,8 +88,16 @@ class _ExampleSelectorState extends State<ExampleSelector>
);
}
+ Future<void> _loadCatalogIfNot(PlaygroundController controller) async {
Review Comment:
To avoid typing ignore comment, may write `catchTypedError<T>` extension for future
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/content_example_loader.dart:
##########
@@ -31,16 +32,16 @@ class ContentExampleLoader extends ExampleLoader {
required ExampleCache exampleCache,
});
+ @override
+ Sdk get sdk => descriptor.sdk;
+
@override
Future<Example> get future async => Example(
complexity: descriptor.complexity,
- description: '',
- name: descriptor.name ?? 'Embedded_Example',
+ name: descriptor.name ?? 'Untitled Example',
Review Comment:
π
##########
playground/frontend/lib/modules/examples/components/example_list/category_expansion_panel.dart:
##########
@@ -27,18 +27,15 @@ import 'package:playground_components/playground_components.dart';
class CategoryExpansionPanel extends StatelessWidget {
final String categoryName;
final List<ExampleBase> examples;
+ final VoidCallback onSelected;
final ExampleBase selectedExample;
- final AnimationController animationController;
- final OverlayEntry? dropdown;
const CategoryExpansionPanel({
- Key? key,
Review Comment:
return `key`, please.
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -50,36 +48,9 @@ class ExampleSelector extends StatefulWidget {
State<ExampleSelector> createState() => _ExampleSelectorState();
}
-class _ExampleSelectorState extends State<ExampleSelector>
- with TickerProviderStateMixin {
- final GlobalKey selectorKey = LabeledGlobalKey('ExampleSelector');
- late OverlayEntry? examplesDropdown;
- late AnimationController animationController;
- late Animation<Offset> offsetAnimation;
-
- final TextEditingController textController = TextEditingController();
- final ScrollController scrollController = ScrollController();
-
- @override
- void initState() {
- super.initState();
- animationController = AnimationController(
Review Comment:
Why have you removed this animation? Looks like changing which should be executed in separate task
##########
playground/frontend/lib/pages/playground/components/playground_page_providers.dart:
##########
@@ -98,4 +98,19 @@ class PlaygroundPageProviders extends StatelessWidget {
child: child,
);
}
+
+ Future<void> _load(
Review Comment:
`_loadStartExamples`?
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/example_loader.dart:
##########
@@ -17,9 +17,12 @@
*/
import '../../models/example.dart';
+import '../../models/sdk.dart';
abstract class ExampleLoader {
const ExampleLoader();
+ Sdk? get sdk;
Review Comment:
May be simplify such way? Also it helps to unify descriptors (Only standard descriptor has not sdk)
```
abstract class ExampleLoader {
final ExampleLoadingDescriptor descriptor;
const ExampleLoader({required this.descriptor});
Sdk? get sdk => descriptor?.sdk;
Future<Example> get future;
}
```
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/user_shared_example_loader.dart:
##########
@@ -19,18 +19,24 @@
import '../../cache/example_cache.dart';
import '../../models/example.dart';
import '../../models/example_loading_descriptors/user_shared_example_loading_descriptor.dart';
+import '../../models/sdk.dart';
import 'example_loader.dart';
class UserSharedExampleLoader extends ExampleLoader {
final UserSharedExampleLoadingDescriptor descriptor;
final ExampleCache exampleCache;
+ Example? _example;
UserSharedExampleLoader({
required this.descriptor,
required this.exampleCache,
});
@override
- Future<Example> get future =>
- exampleCache.loadSharedExample(descriptor.snippetId);
+ Sdk? get sdk => _example?.sdk;
Review Comment:
May be load example here too before return sdk?
##########
playground/frontend/lib/modules/examples/components/example_list/expansion_panel_item.dart:
##########
@@ -83,8 +81,7 @@ class ExpansionPanelItem extends StatelessWidget {
}
void _closeDropdown(ExampleCache exampleCache) {
- animationController.reverse();
- dropdown?.remove();
exampleCache.changeSelectorVisibility();
+ onSelected();
Review Comment:
Looks very inconvenient to pass callback just for closing overlay through several layers. After some research I didn't found any way to do this more gracefully. Did you?
##########
playground/frontend/playground_components/lib/src/widgets/toasts/toast_listener.dart:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+
+import 'package:flutter/material.dart';
+import 'package:fluttertoast/fluttertoast.dart';
Review Comment:
`import 'package:fluttertoast/fluttertoast.dart' hide Toast;`
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/examples_loader.dart:
##########
@@ -48,58 +50,98 @@ class ExamplesLoader {
_playgroundController = value;
Review Comment:
convert to setter, please :)
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/examples_loader.dart:
##########
@@ -48,58 +50,98 @@ class ExamplesLoader {
_playgroundController = value;
}
+ /// Loads examples from [descriptor]'s immediate list.
+ ///
+ /// Sets empty editor for SDKs of failed examples.
Future<void> load(ExamplesLoadingDescriptor descriptor) async {
if (_descriptor == descriptor) {
return;
}
_descriptor = descriptor;
- await Future.wait(
- descriptor.descriptors.map(
- (one) => loadOne(group: descriptor, one: one),
- ),
- );
+ final loaders = descriptor.descriptors.map(_createLoader).whereNotNull();
+
+ try {
+ final loadFutures = loaders.map(_loadOne);
+ await Future.wait(loadFutures);
+ } on Exception catch (ex) {
+ _emptyMissing(loaders);
+ throw ExampleLoadingException(ex);
+ }
final sdk = descriptor.initialSdk;
if (sdk != null) {
_playgroundController!.setSdk(sdk);
}
}
- Future<void> loadDefaultIfAny(Sdk sdk) async {
- final group = _descriptor;
- final one = group?.lazyLoadDescriptors[sdk]?.firstOrNull;
+ ExampleLoader? _createLoader(ExampleLoadingDescriptor descriptor) {
+ final loader = defaultFactory.create(
+ descriptor: descriptor,
+ exampleCache: _playgroundController!.exampleCache,
+ );
- if (group == null || one == null) {
+ if (loader == null) {
+ // TODO: Log.
Review Comment:
Add issue number, please
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/standard_example_loader.dart:
##########
@@ -57,19 +61,13 @@ class StandardExampleLoader extends ExampleLoader {
);
}
- Future<ExampleBase?> _loadExampleWithoutInfo() {
- return exampleCache.hasCatalog
- ? exampleCache.getCatalogExampleByPath(descriptor.path)
- : _loadExampleFromRepository();
- }
-
- Future<ExampleBase?> _loadExampleFromRepository() async {
- final sdk = Sdk.tryParseExamplePath(descriptor.path);
+ Future<ExampleBase?> _loadExampleWithoutInfo() async {
Review Comment:
Why it calls `without info`? May be `loadBaseExample`? This name makes less questions
##########
playground/frontend/playground_components/test/src/controllers/example_loaders/example_loader_factory_test.dart:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter_test/flutter_test.dart';
+import 'package:mockito/annotations.dart';
Review Comment:
unused import
--
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] nausharipov commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
nausharipov commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031458192
##########
playground/frontend/lib/modules/examples/examples_dropdown_content.dart:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:flutter_gen/gen_l10n/app_localizations.dart';
+import 'package:playground/constants/links.dart';
+import 'package:playground/constants/sizes.dart';
+import 'package:playground/modules/examples/components/example_list/example_list.dart';
+import 'package:playground/modules/examples/components/filter/filter.dart';
+import 'package:playground/modules/examples/components/search_field/search_field.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:url_launcher/url_launcher.dart';
+
+class ExamplesDropdownContent extends StatefulWidget {
+ final VoidCallback onSelected;
+ final PlaygroundController playgroundController;
+
+ const ExamplesDropdownContent({
+ required this.onSelected,
+ required this.playgroundController,
+ });
+
+ @override
+ State<ExamplesDropdownContent> createState() =>
+ _ExamplesDropdownContentState();
+}
+
+class _ExamplesDropdownContentState extends State<ExamplesDropdownContent> {
+ final _searchController = TextEditingController();
+
+ @override
+ Widget build(BuildContext context) {
+ switch (widget.playgroundController.exampleCache.catalogStatus) {
+ case LoadingStatus.error:
+ return const LoadingErrorWidget();
+
+ case LoadingStatus.loading:
+ return const LoadingIndicator();
+
+ case LoadingStatus.done:
+ return _buildLoaded();
+ }
+ }
+
+ Widget _buildLoaded() {
Review Comment:
Yes, mostly for performance reasons. The only official explanation is here: https://www.youtube.com/watch?v=IOyq-eTRhvo
--
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] github-actions[bot] commented on pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24338:
URL: https://github.com/apache/beam/pull/24338#issuecomment-1329009776
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031614930
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/user_shared_example_loader.dart:
##########
@@ -19,18 +19,24 @@
import '../../cache/example_cache.dart';
import '../../models/example.dart';
import '../../models/example_loading_descriptors/user_shared_example_loading_descriptor.dart';
+import '../../models/sdk.dart';
import 'example_loader.dart';
class UserSharedExampleLoader extends ExampleLoader {
final UserSharedExampleLoadingDescriptor descriptor;
final ExampleCache exampleCache;
+ Example? _example;
UserSharedExampleLoader({
required this.descriptor,
required this.exampleCache,
});
@override
- Future<Example> get future =>
- exampleCache.loadSharedExample(descriptor.snippetId);
+ Sdk? get sdk => _example?.sdk;
+
+ @override
+ Future<Example> get future async {
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031599415
##########
playground/frontend/lib/modules/examples/examples_dropdown_content.dart:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:flutter_gen/gen_l10n/app_localizations.dart';
+import 'package:playground/constants/links.dart';
+import 'package:playground/constants/sizes.dart';
+import 'package:playground/modules/examples/components/example_list/example_list.dart';
+import 'package:playground/modules/examples/components/filter/filter.dart';
+import 'package:playground/modules/examples/components/search_field/search_field.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:url_launcher/url_launcher.dart';
+
+class ExamplesDropdownContent extends StatefulWidget {
+ final VoidCallback onSelected;
+ final PlaygroundController playgroundController;
+
+ const ExamplesDropdownContent({
+ required this.onSelected,
+ required this.playgroundController,
+ });
+
+ @override
+ State<ExamplesDropdownContent> createState() =>
+ _ExamplesDropdownContentState();
+}
+
+class _ExamplesDropdownContentState extends State<ExamplesDropdownContent> {
+ final _searchController = TextEditingController();
+
+ @override
+ Widget build(BuildContext context) {
+ switch (widget.playgroundController.exampleCache.catalogStatus) {
+ case LoadingStatus.error:
+ return const LoadingErrorWidget();
+
+ case LoadingStatus.loading:
+ return const LoadingIndicator();
+
+ case LoadingStatus.done:
+ return _buildLoaded();
+ }
+ }
+
+ Widget _buildLoaded() {
Review Comment:
Good point for many cases. But here we should not split this. Whenever this is rebuilt we should also check the catalog status to maybe go into another `case`. Splitting this will require an extra `AnimatedBuilder` on `playgroundController`. This helper method actually puts its disadvantage into use.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031600823
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/empty_example_loader.dart:
##########
@@ -20,24 +20,32 @@ import '../../cache/example_cache.dart';
import '../../models/example.dart';
import '../../models/example_base.dart';
import '../../models/example_loading_descriptors/empty_example_loading_descriptor.dart';
+import '../../models/sdk.dart';
import 'example_loader.dart';
class EmptyExampleLoader extends ExampleLoader {
final EmptyExampleLoadingDescriptor descriptor;
+ final Example _example;
- const EmptyExampleLoader({
+ EmptyExampleLoader({
required this.descriptor,
// TODO(alexeyinkin): Remove when this lands: https://github.com/dart-lang/language/issues/1813
required ExampleCache exampleCache,
- });
+ }) : _example = createExample(descriptor.sdk);
@override
- Future<Example> get future async => Example(
- name: 'Embedded_Example',
- path: '',
- sdk: descriptor.sdk,
- source: '',
- tags: [],
- type: ExampleType.example,
- );
+ Sdk get sdk => _example.sdk;
+
+ @override
+ Future<Example> get future async => _example;
+
+ static Example createExample(Sdk sdk) {
Review Comment:
`+`
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033190180
##########
playground/frontend/playground_components/test/src/controllers/example_loaders/common.dart:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:playground_components/playground_components.dart';
+
+import 'package:playground_components/src/controllers/example_loaders/example_loader.dart';
+import 'package:playground_components/src/controllers/example_loaders/example_loader_factory.dart';
+
+class TestExampleLoadingDescriptor extends ExampleLoadingDescriptor {
+ final Sdk? sdk;
+ final bool ok;
Review Comment:
I think it should be named more specifically
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033221940
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/examples_loader.dart:
##########
@@ -48,58 +50,98 @@ class ExamplesLoader {
_playgroundController = value;
}
+ /// Loads examples from [descriptor]'s immediate list.
+ ///
+ /// Sets empty editor for SDKs of failed examples.
Future<void> load(ExamplesLoadingDescriptor descriptor) async {
if (_descriptor == descriptor) {
return;
}
_descriptor = descriptor;
- await Future.wait(
- descriptor.descriptors.map(
- (one) => loadOne(group: descriptor, one: one),
- ),
- );
+ final loaders = descriptor.descriptors.map(_createLoader).whereNotNull();
+
+ try {
+ final loadFutures = loaders.map(_loadOne);
+ await Future.wait(loadFutures);
+ } on Exception catch (ex) {
+ _emptyMissing(loaders);
+ throw ExampleLoadingException(ex);
+ }
final sdk = descriptor.initialSdk;
if (sdk != null) {
_playgroundController!.setSdk(sdk);
}
}
- Future<void> loadDefaultIfAny(Sdk sdk) async {
- final group = _descriptor;
- final one = group?.lazyLoadDescriptors[sdk]?.firstOrNull;
+ ExampleLoader? _createLoader(ExampleLoadingDescriptor descriptor) {
+ final loader = defaultFactory.create(
+ descriptor: descriptor,
+ exampleCache: _playgroundController!.exampleCache,
+ );
- if (group == null || one == null) {
+ if (loader == null) {
+ // TODO: Log.
+ print('Cannot create example loader for $descriptor');
+ return null;
+ }
+
+ return loader;
+ }
+
+ void _emptyMissing(Iterable<ExampleLoader> loaders) {
+ loaders.forEach(_emptyIfMissing);
+ }
+
+ Future<void> _emptyIfMissing(ExampleLoader loader) async {
+ final sdk = loader.sdk;
+
+ if (sdk == null) {
return;
}
- return loadOne(
- group: group,
- one: one,
+ _playgroundController!.setEmptyIfNotExists(
+ sdk,
+ setCurrentSdk: _shouldSetCurrentSdk(sdk),
);
}
- Future<void> loadOne({
- required ExamplesLoadingDescriptor group,
- required ExampleLoadingDescriptor one,
- }) async {
- final loader = defaultFactory.create(
- descriptor: one,
- exampleCache: _playgroundController!.exampleCache,
- );
+ Future<void> loadDefaultIfAny(Sdk sdk) async {
+ final group = _descriptor;
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
pabloem commented on PR #24338:
URL: https://github.com/apache/beam/pull/24338#issuecomment-1352618126
thanks!
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033130147
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -50,36 +48,9 @@ class ExampleSelector extends StatefulWidget {
State<ExampleSelector> createState() => _ExampleSelectorState();
}
-class _ExampleSelectorState extends State<ExampleSelector>
- with TickerProviderStateMixin {
- final GlobalKey selectorKey = LabeledGlobalKey('ExampleSelector');
- late OverlayEntry? examplesDropdown;
- late AnimationController animationController;
- late Animation<Offset> offsetAnimation;
-
- final TextEditingController textController = TextEditingController();
- final ScrollController scrollController = ScrollController();
-
- @override
- void initState() {
- super.initState();
- animationController = AnimationController(
Review Comment:
Another dropdowns also uses this animation, but all of them uses separate widget for this: `AppDropdownButton`. So if you remove animation from this widget now, it breaks UX
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033229779
##########
playground/frontend/playground_components/lib/src/cache/saving_exception.dart:
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:easy_localization/easy_localization.dart';
+
+class SavingException implements Exception {
Review Comment:
Fixed.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033220438
##########
playground/frontend/lib/modules/examples/components/example_list/category_expansion_panel.dart:
##########
@@ -27,18 +27,15 @@ import 'package:playground_components/playground_components.dart';
class CategoryExpansionPanel extends StatelessWidget {
final String categoryName;
final List<ExampleBase> examples;
+ final VoidCallback onSelected;
final ExampleBase selectedExample;
- final AnimationController animationController;
- final OverlayEntry? dropdown;
const CategoryExpansionPanel({
- Key? key,
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031554228
##########
playground/frontend/lib/modules/examples/examples_dropdown_content.dart:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:flutter_gen/gen_l10n/app_localizations.dart';
+import 'package:playground/constants/links.dart';
+import 'package:playground/constants/sizes.dart';
+import 'package:playground/modules/examples/components/example_list/example_list.dart';
+import 'package:playground/modules/examples/components/filter/filter.dart';
+import 'package:playground/modules/examples/components/search_field/search_field.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:url_launcher/url_launcher.dart';
+
+class ExamplesDropdownContent extends StatefulWidget {
+ final VoidCallback onSelected;
+ final PlaygroundController playgroundController;
+
+ const ExamplesDropdownContent({
+ required this.onSelected,
+ required this.playgroundController,
+ });
+
+ @override
+ State<ExamplesDropdownContent> createState() =>
+ _ExamplesDropdownContentState();
+}
+
+class _ExamplesDropdownContentState extends State<ExamplesDropdownContent> {
+ final _searchController = TextEditingController();
+
+ @override
+ Widget build(BuildContext context) {
+ switch (widget.playgroundController.exampleCache.catalogStatus) {
+ case LoadingStatus.error:
+ return const LoadingErrorWidget();
+
+ case LoadingStatus.loading:
+ return const LoadingIndicator();
+
+ case LoadingStatus.done:
+ return _buildLoaded();
+ }
+ }
+
+ Widget _buildLoaded() {
+ return Column(
+ children: [
+ SearchField(controller: _searchController),
+ const ExamplesFilter(),
+ ExampleList(
+ onSelected: widget.onSelected,
+ selectedExample: widget.playgroundController.selectedExample!,
Review Comment:
Yes, this can happen. The whole dropdown must be refactored even deeper than that. But this has been moved from `example_selector.dart` as is, this is not a regression.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031605265
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/examples_loader.dart:
##########
@@ -48,58 +50,98 @@ class ExamplesLoader {
_playgroundController = value;
}
+ /// Loads examples from [descriptor]'s immediate list.
+ ///
+ /// Sets empty editor for SDKs of failed examples.
Future<void> load(ExamplesLoadingDescriptor descriptor) async {
if (_descriptor == descriptor) {
return;
}
_descriptor = descriptor;
- await Future.wait(
- descriptor.descriptors.map(
- (one) => loadOne(group: descriptor, one: one),
- ),
- );
+ final loaders = descriptor.descriptors.map(_createLoader).whereNotNull();
+
+ try {
+ final loadFutures = loaders.map(_loadOne);
+ await Future.wait(loadFutures);
+ } on Exception catch (ex) {
+ _emptyMissing(loaders);
+ throw ExampleLoadingException(ex);
+ }
final sdk = descriptor.initialSdk;
if (sdk != null) {
_playgroundController!.setSdk(sdk);
}
}
- Future<void> loadDefaultIfAny(Sdk sdk) async {
- final group = _descriptor;
- final one = group?.lazyLoadDescriptors[sdk]?.firstOrNull;
+ ExampleLoader? _createLoader(ExampleLoadingDescriptor descriptor) {
+ final loader = defaultFactory.create(
+ descriptor: descriptor,
+ exampleCache: _playgroundController!.exampleCache,
+ );
- if (group == null || one == null) {
+ if (loader == null) {
+ // TODO: Log.
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031610660
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/example_loader.dart:
##########
@@ -17,9 +17,12 @@
*/
import '../../models/example.dart';
+import '../../models/sdk.dart';
abstract class ExampleLoader {
const ExampleLoader();
+ Sdk? get sdk;
Review Comment:
1. If one subclass does not have a property, it is not a property of the superclass. This is the core of inheritance.
2. `UserSharedExampleLoadingDescriptor` also has no `Sdk`.
3. There are loaders with non-nullable `Sdk` which we will lose this way.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033220696
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -16,23 +16,21 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
-import 'package:flutter_gen/gen_l10n/app_localizations.dart';
-import 'package:playground/constants/links.dart';
import 'package:playground/constants/sizes.dart';
-import 'package:playground/modules/examples/components/examples_components.dart';
import 'package:playground/modules/examples/components/outside_click_handler.dart';
+import 'package:playground/modules/examples/examples_dropdown_content.dart';
import 'package:playground/modules/examples/models/popover_state.dart';
import 'package:playground/pages/playground/states/example_selector_state.dart';
import 'package:playground/utils/dropdown_utils.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
-import 'package:url_launcher/url_launcher.dart';
const int kAnimationDurationInMilliseconds = 80;
const Offset kAnimationBeginOffset = Offset(0.0, -0.02);
Review Comment:
`+`
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033130147
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -50,36 +48,9 @@ class ExampleSelector extends StatefulWidget {
State<ExampleSelector> createState() => _ExampleSelectorState();
}
-class _ExampleSelectorState extends State<ExampleSelector>
- with TickerProviderStateMixin {
- final GlobalKey selectorKey = LabeledGlobalKey('ExampleSelector');
- late OverlayEntry? examplesDropdown;
- late AnimationController animationController;
- late Animation<Offset> offsetAnimation;
-
- final TextEditingController textController = TextEditingController();
- final ScrollController scrollController = ScrollController();
-
- @override
- void initState() {
- super.initState();
- animationController = AnimationController(
Review Comment:
Another dropdowns also uses this animation, but all of them uses separate widget for this: `AppDropdownButton`. So if you remove animation from this widget now, it breaks UX. Suggest to return this 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
[GitHub] [beam] alexeyinkin commented on pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on PR #24338:
URL: https://github.com/apache/beam/pull/24338#issuecomment-1346838678
@damondouglas I normally comment pieces that I am satisfied with and perceive as non-self-documenting. Some examples of that are `PagePath` classes, posting messages with JavaScript, etc.
I normally do not document code that:
1. **Looks self-documenting** to me.
2. **Is up for refactoring**. Examples are current widgets for code sharing. We do have 5 widgets with `*ShareTab*` in their names, and their nesting is complex. Still I think it makes little point to document them because they do not have sound reasons to be separate, and I plan to simplify this in a few months.
3. **Is self-documented once a specific API is understood**. For example, most of Flutter developers are not familiar with Navigator 2.0 API and its `Page` classes, yet I think that a project's source is not the right place to educate on that.
This is my habit for consistency when commenting. Please help me adjust to yours if you think that any of the above categories deserve more comments.
Also I have just added comments to 45+ files.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043287010
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -89,27 +57,26 @@ class _ExampleSelectorState extends State<ExampleSelector>
color: Theme.of(context).dividerColor,
borderRadius: BorderRadius.circular(kSmBorderRadius),
),
- child: Consumer<PlaygroundController>(
- builder: (context, state, child) => TextButton(
- key: selectorKey,
+ child: ChangeNotifierProvider<PlaygroundController>.value(
+ value: widget.playgroundController,
+ builder: (context, child) => TextButton(
+ key: _selectorKey,
onPressed: () {
if (widget.isSelectorOpened) {
- animationController.reverse();
- examplesDropdown?.remove();
+ _overlayEntry?.remove();
+ widget.playgroundController.exampleCache.setSelectorOpened(false);
} else {
- animationController.forward();
- examplesDropdown = createExamplesDropdown();
- Overlay.of(context)?.insert(examplesDropdown!);
+ unawaited(_loadCatalogIfNot(widget.playgroundController));
+ _overlayEntry = _createExamplesDropdown();
+ Overlay.of(context)?.insert(_overlayEntry!);
Review Comment:
Does not look so.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043300901
##########
playground/frontend/playground_components/lib/src/controllers/playground_controller.dart:
##########
@@ -137,8 +140,38 @@ class PlaygroundController with ChangeNotifier {
selectedExample?.type != ExampleType.test &&
[Sdk.java, Sdk.python].contains(sdk);
+ /// If no SDK is selected, sets it to [sdk] and creates an empty state for it.
+ void setEmptyIfNoSdk(Sdk sdk) {
Review Comment:
This is `playground_components` package, it has no notion of a default SDK.
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033133472
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -16,23 +16,21 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
-import 'package:flutter_gen/gen_l10n/app_localizations.dart';
-import 'package:playground/constants/links.dart';
import 'package:playground/constants/sizes.dart';
-import 'package:playground/modules/examples/components/examples_components.dart';
import 'package:playground/modules/examples/components/outside_click_handler.dart';
+import 'package:playground/modules/examples/examples_dropdown_content.dart';
import 'package:playground/modules/examples/models/popover_state.dart';
import 'package:playground/pages/playground/states/example_selector_state.dart';
import 'package:playground/utils/dropdown_utils.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
-import 'package:url_launcher/url_launcher.dart';
const int kAnimationDurationInMilliseconds = 80;
const Offset kAnimationBeginOffset = Offset(0.0, -0.02);
Review Comment:
Yes, so I suggest to move these constants to `feedback_dropdown_icon_button.dart`. Or in general constants file, if you are sure that they will be used somewhere else
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033217883
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -50,36 +48,9 @@ class ExampleSelector extends StatefulWidget {
State<ExampleSelector> createState() => _ExampleSelectorState();
}
-class _ExampleSelectorState extends State<ExampleSelector>
- with TickerProviderStateMixin {
- final GlobalKey selectorKey = LabeledGlobalKey('ExampleSelector');
- late OverlayEntry? examplesDropdown;
- late AnimationController animationController;
- late Animation<Offset> offsetAnimation;
-
- final TextEditingController textController = TextEditingController();
- final ScrollController scrollController = ScrollController();
-
- @override
- void initState() {
- super.initState();
- animationController = AnimationController(
Review Comment:
Let this await here: https://github.com/apache/beam/issues/24371
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043285318
##########
playground/frontend/lib/modules/examples/components/example_list/example_list.dart:
##########
@@ -17,24 +17,27 @@
*/
import 'package:flutter/material.dart';
-import 'package:playground/modules/examples/components/examples_components.dart';
-import 'package:playground/pages/playground/states/example_selector_state.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
-class ExampleList extends StatelessWidget {
- final ScrollController controller;
- final AnimationController animationController;
- final OverlayEntry? dropdown;
+import '../../../../pages/standalone_playground/notifiers/example_selector_state.dart';
+import '../examples_components.dart';
+
+class ExampleList extends StatefulWidget {
+ final VoidCallback onSelected;
final ExampleBase selectedExample;
const ExampleList({
- Key? key,
- required this.controller,
+ required this.onSelected,
required this.selectedExample,
- required this.animationController,
- required this.dropdown,
- }) : super(key: key);
+ });
+
+ @override
+ State<ExampleList> createState() => _ExampleListState();
+}
+
+class _ExampleListState extends State<ExampleList> {
+ final _scrollController = ScrollController();
@override
Widget build(BuildContext context) {
Review Comment:
Should be addressed in https://github.com/apache/beam/issues/24371
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043307866
##########
playground/frontend/playground_components/lib/src/widgets/toasts/toast.dart:
##########
@@ -19,44 +19,52 @@
import 'package:flutter/material.dart';
import 'package:flutter_svg/flutter_svg.dart';
-import '../constants/sizes.dart';
+import '../../assets/assets.gen.dart';
+import '../../constants/colors.dart';
+import '../../constants/sizes.dart';
+import '../../models/toast.dart';
+import '../../models/toast_type.dart';
+import '../../playground_components.dart';
-const kNotificationBorderWidth = 4.0;
-const kMaxTextWidth = 300.0;
+const _borderWidth = 4.0;
+const _textWidth = 300.0;
-class BaseNotification extends StatelessWidget {
- final String title;
- final String notification;
- final Color color;
- final String asset;
+const _colors = UnmodifiableToastTypeMap(
+ error: BeamNotificationColors.error,
+ info: BeamNotificationColors.info,
+);
- const BaseNotification({
- super.key,
- required this.title,
- required this.notification,
- required this.color,
- required this.asset,
- });
+final _iconNames = UnmodifiableToastTypeMap(
+ error: Assets.notificationIcons.error,
+ info: Assets.notificationIcons.info,
+);
+
+class ToastWidget extends StatelessWidget {
+ final Toast toast;
+
+ const ToastWidget(this.toast);
@override
Widget build(BuildContext context) {
- return Stack(
- children: [
- _renderLeftBorder(context),
- _renderNotificationContent(context),
- ],
+ return Card(
+ child: Stack(
+ children: [
+ _leftBorder(context),
Review Comment:
This is a moved code, let's keep it as it is.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031509793
##########
playground/frontend/lib/modules/examples/components/example_list/category_expansion_panel.dart:
##########
@@ -27,18 +27,15 @@ import 'package:playground_components/playground_components.dart';
class CategoryExpansionPanel extends StatelessWidget {
final String categoryName;
final List<ExampleBase> examples;
+ final VoidCallback onSelected;
final ExampleBase selectedExample;
- final AnimationController animationController;
- final OverlayEntry? dropdown;
const CategoryExpansionPanel({
- Key? key,
Review Comment:
I think keys must be in library widgets where the user does not control the code. In apps keys can be added as needed. We only use keys on 5 occasions, most of them due to bad design.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031510859
##########
playground/frontend/lib/modules/examples/components/example_list/expansion_panel_item.dart:
##########
@@ -83,8 +81,7 @@ class ExpansionPanelItem extends StatelessWidget {
}
void _closeDropdown(ExampleCache exampleCache) {
- animationController.reverse();
- dropdown?.remove();
exampleCache.changeSelectorVisibility();
Review Comment:
Let's save this for https://github.com/apache/beam/issues/23249
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1044109158
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -89,27 +57,26 @@ class _ExampleSelectorState extends State<ExampleSelector>
color: Theme.of(context).dividerColor,
borderRadius: BorderRadius.circular(kSmBorderRadius),
),
- child: Consumer<PlaygroundController>(
- builder: (context, state, child) => TextButton(
- key: selectorKey,
+ child: ChangeNotifierProvider<PlaygroundController>.value(
+ value: widget.playgroundController,
+ builder: (context, child) => TextButton(
+ key: _selectorKey,
onPressed: () {
if (widget.isSelectorOpened) {
- animationController.reverse();
- examplesDropdown?.remove();
+ _overlayEntry?.remove();
+ widget.playgroundController.exampleCache.setSelectorOpened(false);
} else {
- animationController.forward();
- examplesDropdown = createExamplesDropdown();
- Overlay.of(context)?.insert(examplesDropdown!);
+ unawaited(_loadCatalogIfNot(widget.playgroundController));
+ _overlayEntry = _createExamplesDropdown();
+ Overlay.of(context)?.insert(_overlayEntry!);
Review Comment:
Hm, my method is non-nullable. Possible because i'm on `main` flutter branch
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031101449
##########
playground/frontend/lib/modules/examples/models/example_loading_descriptors/examples_loading_descriptor_factory.dart:
##########
@@ -24,7 +24,7 @@ import 'package:playground/modules/examples/models/example_token_type.dart';
import 'package:playground_components/playground_components.dart';
class ExamplesLoadingDescriptorFactory {
- static const _defaultSdk = Sdk.java;
+ static const defaultSdk = Sdk.java;
Review Comment:
Suggest to move to more general file, `params` may be
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031616979
##########
playground/frontend/playground_components/test/src/controllers/example_loaders/example_loader_factory_test.dart:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter_test/flutter_test.dart';
+import 'package:mockito/annotations.dart';
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031519898
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -118,8 +88,16 @@ class _ExampleSelectorState extends State<ExampleSelector>
);
}
+ Future<void> _loadCatalogIfNot(PlaygroundController controller) async {
Review Comment:
1. This is 4 vs 3 identifiers in a chain.
2. This mixes async with the Future API.
3. This relies on runtime type cast.
Lastly, Future API must be avoided when possible: https://medium.com/p/2e9c340aabfb
Of that article, this code is subject to uncaught exceptions in `loadAllPrecompiledObjectsIfNot` before the asynchronous gap.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031596782
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs/snippet_save_and_share_tabs.dart:
##########
@@ -16,32 +16,59 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
import 'package:playground/modules/editor/components/share_dropdown/share_tabs/example_share_tabs.dart';
import 'package:playground_components/playground_components.dart';
-class SnippetSaveAndShareTabs extends StatelessWidget {
+class SnippetSaveAndShareTabs extends StatefulWidget {
+ final VoidCallback onError;
final PlaygroundController playgroundController;
final TabController tabController;
const SnippetSaveAndShareTabs({
super.key,
+ required this.onError,
required this.playgroundController,
required this.tabController,
});
+ @override
+ State<SnippetSaveAndShareTabs> createState() => _SnippetSaveAndShareTabsState();
+}
+
+class _SnippetSaveAndShareTabsState extends State<SnippetSaveAndShareTabs> {
+ Future<String>? _future;
+
+ @override
+ void initState() {
+ super.initState();
+ unawaited(_initSaving());
+ }
+
+ Future<void> _initSaving() async {
+ try {
+ _future = widget.playgroundController.saveSnippet();
+ await _future;
+ } on Exception catch (ex) {
+ PlaygroundComponents.toastNotifier.addException(ex);
+ widget.onError();
Review Comment:
With this message, the exception can be reused for anything we may be saving in the future. Until I am comfortable with the change, I want less exception classes. I am not comfortable yet. Let's see Tour of Beam adopt the error messages too.
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033142944
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/examples_loader.dart:
##########
@@ -48,58 +50,98 @@ class ExamplesLoader {
_playgroundController = value;
}
+ /// Loads examples from [descriptor]'s immediate list.
+ ///
+ /// Sets empty editor for SDKs of failed examples.
Future<void> load(ExamplesLoadingDescriptor descriptor) async {
if (_descriptor == descriptor) {
return;
}
_descriptor = descriptor;
- await Future.wait(
- descriptor.descriptors.map(
- (one) => loadOne(group: descriptor, one: one),
- ),
- );
+ final loaders = descriptor.descriptors.map(_createLoader).whereNotNull();
+
+ try {
+ final loadFutures = loaders.map(_loadOne);
+ await Future.wait(loadFutures);
+ } on Exception catch (ex) {
+ _emptyMissing(loaders);
+ throw ExampleLoadingException(ex);
+ }
final sdk = descriptor.initialSdk;
if (sdk != null) {
_playgroundController!.setSdk(sdk);
}
}
- Future<void> loadDefaultIfAny(Sdk sdk) async {
- final group = _descriptor;
- final one = group?.lazyLoadDescriptors[sdk]?.firstOrNull;
+ ExampleLoader? _createLoader(ExampleLoadingDescriptor descriptor) {
+ final loader = defaultFactory.create(
+ descriptor: descriptor,
+ exampleCache: _playgroundController!.exampleCache,
+ );
- if (group == null || one == null) {
+ if (loader == null) {
+ // TODO: Log.
+ print('Cannot create example loader for $descriptor');
+ return null;
+ }
+
+ return loader;
+ }
+
+ void _emptyMissing(Iterable<ExampleLoader> loaders) {
+ loaders.forEach(_emptyIfMissing);
+ }
+
+ Future<void> _emptyIfMissing(ExampleLoader loader) async {
+ final sdk = loader.sdk;
+
+ if (sdk == null) {
return;
}
- return loadOne(
- group: group,
- one: one,
+ _playgroundController!.setEmptyIfNotExists(
+ sdk,
+ setCurrentSdk: _shouldSetCurrentSdk(sdk),
);
}
- Future<void> loadOne({
- required ExamplesLoadingDescriptor group,
- required ExampleLoadingDescriptor one,
- }) async {
- final loader = defaultFactory.create(
- descriptor: one,
- exampleCache: _playgroundController!.exampleCache,
- );
+ Future<void> loadDefaultIfAny(Sdk sdk) async {
+ final group = _descriptor;
+ final one = group?.lazyLoadDescriptors[sdk]?.firstOrNull;
+ if (group == null || one == null) {
+ return;
+ }
+
+ final loader = _createLoader(one);
if (loader == null) {
- // TODO: Log.
- print('Cannot create example loader for $one');
return;
}
+ await _loadOne(loader);
+ }
+
+ Future<void> _loadOne(ExampleLoader loader) async {
final example = await loader.future;
_playgroundController!.setExample(
example,
- setCurrentSdk:
- example.sdk == group.initialSdk || group.initialSdk == null,
+ setCurrentSdk: _shouldSetCurrentSdk(example.sdk),
);
}
+
+ bool _shouldSetCurrentSdk(Sdk sdk) {
+ final descriptor = _descriptor;
Review Comment:
Here I'm wrong
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033228994
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs/snippet_save_and_share_tabs.dart:
##########
@@ -16,32 +16,59 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
import 'package:playground/modules/editor/components/share_dropdown/share_tabs/example_share_tabs.dart';
import 'package:playground_components/playground_components.dart';
-class SnippetSaveAndShareTabs extends StatelessWidget {
+class SnippetSaveAndShareTabs extends StatefulWidget {
+ final VoidCallback onError;
final PlaygroundController playgroundController;
final TabController tabController;
const SnippetSaveAndShareTabs({
super.key,
+ required this.onError,
required this.playgroundController,
required this.tabController,
});
+ @override
+ State<SnippetSaveAndShareTabs> createState() => _SnippetSaveAndShareTabsState();
+}
+
+class _SnippetSaveAndShareTabsState extends State<SnippetSaveAndShareTabs> {
+ Future<String>? _future;
+
+ @override
+ void initState() {
+ super.initState();
+ unawaited(_initSaving());
+ }
+
+ Future<void> _initSaving() async {
+ try {
+ _future = widget.playgroundController.saveSnippet();
+ await _future;
+ } on Exception catch (ex) {
+ PlaygroundComponents.toastNotifier.addException(ex);
+ widget.onError();
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043274394
##########
playground/frontend/lib/controllers/factories.dart:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+
+import 'package:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../constants/params.dart';
+import '../modules/messages/handlers/messages_debouncer.dart';
+import '../modules/messages/handlers/messages_handler.dart';
+import '../modules/messages/listeners/messages_listener.dart';
+
+PlaygroundController createPlaygroundController(
Review Comment:
I don't think there is a benefit of a static method over a global function unless it's clearly more readable like `int.parse(...)`
There's even a lint against that: https://dart-lang.github.io/linter/lints/avoid_classes_with_only_static_members.html
Although we do not strictly follow it.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043281332
##########
playground/frontend/lib/locator.dart:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:app_state/app_state.dart';
+import 'package:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
+
+import 'config.g.dart';
+import 'pages/loading/page.dart';
+import 'router/page_factory.dart';
+import 'router/route_information_parser.dart';
+
+Future<void> initializeServiceLocator() async {
Review Comment:
I think `main.dart` should be as short as possible because it conveys no meaning. In this file, the file name is another hint of what it does. And also this way files have less reasons to change.
As for static methods, see above.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031153982
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs/snippet_save_and_share_tabs.dart:
##########
@@ -16,32 +16,59 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
import 'package:playground/modules/editor/components/share_dropdown/share_tabs/example_share_tabs.dart';
import 'package:playground_components/playground_components.dart';
-class SnippetSaveAndShareTabs extends StatelessWidget {
+class SnippetSaveAndShareTabs extends StatefulWidget {
+ final VoidCallback onError;
final PlaygroundController playgroundController;
final TabController tabController;
const SnippetSaveAndShareTabs({
super.key,
+ required this.onError,
required this.playgroundController,
required this.tabController,
});
+ @override
+ State<SnippetSaveAndShareTabs> createState() => _SnippetSaveAndShareTabsState();
+}
+
+class _SnippetSaveAndShareTabsState extends State<SnippetSaveAndShareTabs> {
+ Future<String>? _future;
+
+ @override
+ void initState() {
+ super.initState();
+ unawaited(_initSaving());
+ }
+
+ Future<void> _initSaving() async {
+ try {
Review Comment:
I agree this is odd, but this is the way it was made originally. Though:
1. This is declarative saving. This is easier to maintain than some imperative approach.
2. A widget test can cover this easily.
3. Saving is idemponent, so no problems so far. Not many users are quick enough to close the dropdown and reopen before saving completes.
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033120054
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs/snippet_save_and_share_tabs.dart:
##########
@@ -16,32 +16,59 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
import 'package:playground/modules/editor/components/share_dropdown/share_tabs/example_share_tabs.dart';
import 'package:playground_components/playground_components.dart';
-class SnippetSaveAndShareTabs extends StatelessWidget {
+class SnippetSaveAndShareTabs extends StatefulWidget {
+ final VoidCallback onError;
final PlaygroundController playgroundController;
final TabController tabController;
const SnippetSaveAndShareTabs({
super.key,
+ required this.onError,
required this.playgroundController,
required this.tabController,
});
+ @override
+ State<SnippetSaveAndShareTabs> createState() => _SnippetSaveAndShareTabsState();
+}
+
+class _SnippetSaveAndShareTabsState extends State<SnippetSaveAndShareTabs> {
+ Future<String>? _future;
+
+ @override
+ void initState() {
+ super.initState();
+ unawaited(_initSaving());
+ }
+
+ Future<void> _initSaving() async {
+ try {
+ _future = widget.playgroundController.saveSnippet();
+ await _future;
+ } on Exception catch (ex) {
+ PlaygroundComponents.toastNotifier.addException(ex);
+ widget.onError();
Review Comment:
I think that that display exceedingly clear exception message is essential part of user experience. So if every separate place requires unique message - it should be done. Through on practice I think that most of messages may be equal, but in this case is not
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031052205
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs/snippet_save_and_share_tabs.dart:
##########
@@ -16,32 +16,59 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
import 'package:playground/modules/editor/components/share_dropdown/share_tabs/example_share_tabs.dart';
import 'package:playground_components/playground_components.dart';
-class SnippetSaveAndShareTabs extends StatelessWidget {
+class SnippetSaveAndShareTabs extends StatefulWidget {
+ final VoidCallback onError;
final PlaygroundController playgroundController;
final TabController tabController;
const SnippetSaveAndShareTabs({
super.key,
+ required this.onError,
required this.playgroundController,
required this.tabController,
});
+ @override
+ State<SnippetSaveAndShareTabs> createState() => _SnippetSaveAndShareTabsState();
+}
+
+class _SnippetSaveAndShareTabsState extends State<SnippetSaveAndShareTabs> {
+ Future<String>? _future;
+
+ @override
+ void initState() {
+ super.initState();
+ unawaited(_initSaving());
+ }
+
+ Future<void> _initSaving() async {
+ try {
Review Comment:
I think that this logic should not be placed in a widget, because:
1. It breaks expectations on widget using. It will be surprise for users of this widget, that it makes request on creation.
2. This approach complicates potential testing.
3. It may reduce performance. What if the user closes popup while requests is running and open it again then?
I suggest to move this logic to playground controller. Run it when `Share my code` button is pressed and subscribe on it result here
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031052205
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs/snippet_save_and_share_tabs.dart:
##########
@@ -16,32 +16,59 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
import 'package:playground/modules/editor/components/share_dropdown/share_tabs/example_share_tabs.dart';
import 'package:playground_components/playground_components.dart';
-class SnippetSaveAndShareTabs extends StatelessWidget {
+class SnippetSaveAndShareTabs extends StatefulWidget {
+ final VoidCallback onError;
final PlaygroundController playgroundController;
final TabController tabController;
const SnippetSaveAndShareTabs({
super.key,
+ required this.onError,
required this.playgroundController,
required this.tabController,
});
+ @override
+ State<SnippetSaveAndShareTabs> createState() => _SnippetSaveAndShareTabsState();
+}
+
+class _SnippetSaveAndShareTabsState extends State<SnippetSaveAndShareTabs> {
+ Future<String>? _future;
+
+ @override
+ void initState() {
+ super.initState();
+ unawaited(_initSaving());
+ }
+
+ Future<void> _initSaving() async {
+ try {
Review Comment:
I think that this logic should not be placed in a widget, because:
1. It breaks expectations on widget using. It will be surprise for users of this widget, that it makes request on creation.
2. This approach complicates potential testing.
3. It may reduce performance. What if the user closes popup while requests is running and open it again then?
I suggest to move this logic to playground controller. Run it and subscribe on it result here
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031623351
##########
playground/frontend/lib/modules/messages/handlers/set_content_message_handler.dart:
##########
@@ -34,11 +37,21 @@ class SetContentMessageHandler extends AbstractMessageHandler {
return MessageHandleResult.notHandled;
}
- _handle(message);
+ unawaited(_handle(message));
return MessageHandleResult.handled;
}
- void _handle(SetContentMessage message) {
- playgroundController.examplesLoader.load(message.descriptor);
+ Future<void> _handle(SetContentMessage message) async {
+ final descriptor = message.descriptor;
+
+ try {
+ await playgroundController.examplesLoader.load(descriptor);
Review Comment:
Yes, but I cannot think of a good reusable class or method with this. Let's wait until we have more locations. 4 lines x 2 locations is not much of a duplication.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031616001
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/user_shared_example_loader.dart:
##########
@@ -19,18 +19,24 @@
import '../../cache/example_cache.dart';
import '../../models/example.dart';
import '../../models/example_loading_descriptors/user_shared_example_loading_descriptor.dart';
+import '../../models/sdk.dart';
import 'example_loader.dart';
class UserSharedExampleLoader extends ExampleLoader {
final UserSharedExampleLoadingDescriptor descriptor;
final ExampleCache exampleCache;
+ Example? _example;
UserSharedExampleLoader({
required this.descriptor,
required this.exampleCache,
});
@override
- Future<Example> get future =>
- exampleCache.loadSharedExample(descriptor.snippetId);
+ Sdk? get sdk => _example?.sdk;
Review Comment:
This must be synchronous.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043306583
##########
playground/frontend/playground_components/lib/src/widgets/toasts/toast.dart:
##########
@@ -66,27 +74,32 @@ class BaseNotification extends StatelessWidget {
);
}
- Widget _renderNotificationContent(BuildContext context) {
+ Widget _content(BuildContext context) {
final textTheme = Theme.of(context).textTheme.bodyText1;
Review Comment:
Do you mean the deprecation of Material 2 in favor of Material 3? A lot of apps depend on it, and it's hard to migrate.
M2 is not even deprecated in code with `@Deprecated` yet. There will be a lot of howtos when they decide to decommission it, we should wait.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043300242
##########
playground/frontend/playground_components/lib/src/cache/example_cache.dart:
##########
@@ -19,11 +19,16 @@
import 'dart:async';
import 'package:collection/collection.dart';
-import 'package:flutter/material.dart';
+import 'package:flutter/foundation.dart';
+import '../exceptions/catalog_loading_exception.dart';
+import '../exceptions/example_loading_exception.dart';
+import '../exceptions/snippet_saving_exception.dart';
import '../models/category_with_examples.dart';
import '../models/example.dart';
import '../models/example_base.dart';
+import '../models/example_loading_descriptors/user_shared_example_loading_descriptor.dart';
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043299128
##########
playground/frontend/lib/pages/standalone_playground/screen.dart:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../../components/logo/logo_component.dart';
+import '../../constants/sizes.dart';
+import '../../modules/actions/components/new_example_action.dart';
+import '../../modules/actions/components/reset_action.dart';
+import '../../modules/analytics/analytics_service.dart';
+import '../../modules/editor/components/pipeline_options_dropdown/pipeline_options_dropdown.dart';
+import '../../modules/examples/example_selector.dart';
+import '../../modules/sdk/components/sdk_selector.dart';
+import '../../modules/shortcuts/components/shortcuts_manager.dart';
+import '../../modules/shortcuts/constants/global_shortcuts.dart';
+import 'state.dart';
+import 'widgets/close_listener_nonweb.dart'
Review Comment:
Should be addressed in https://github.com/apache/beam/issues/24591
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043289317
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -89,27 +57,26 @@ class _ExampleSelectorState extends State<ExampleSelector>
color: Theme.of(context).dividerColor,
borderRadius: BorderRadius.circular(kSmBorderRadius),
),
- child: Consumer<PlaygroundController>(
Review Comment:
Compile-time guarantee.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031600146
##########
playground/frontend/lib/pages/playground/components/playground_page_providers.dart:
##########
@@ -98,4 +98,19 @@ class PlaygroundPageProviders extends StatelessWidget {
child: child,
);
}
+
+ Future<void> _load(
Review Comment:
`+`
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/catalog_default_example_loader.dart:
##########
@@ -30,13 +31,16 @@ class CatalogDefaultExampleLoader extends ExampleLoader {
required this.exampleCache,
});
+ @override
+ Sdk get sdk => descriptor.sdk;
+
@override
Future<Example> get future async {
- if (!exampleCache.hasCatalog) {
- throw Exception('Default example requires a catalog in ExampleState');
- }
+ await Future.wait([
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031594148
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/examples_loader.dart:
##########
@@ -48,58 +50,98 @@ class ExamplesLoader {
_playgroundController = value;
Review Comment:
Maybe later. Let's concentrate on the changes here.
--
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] nausharipov commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
nausharipov commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031460203
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/examples_loader.dart:
##########
@@ -48,58 +50,98 @@ class ExamplesLoader {
_playgroundController = value;
Review Comment:
Then linter asks to not add setters without getters.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031619922
##########
playground/frontend/playground_components/test/src/controllers/example_loaders/examples_loader_test.dart:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter_test/flutter_test.dart';
+import 'package:mockito/annotations.dart';
+import 'package:mockito/mockito.dart';
+import 'package:playground_components/playground_components.dart';
+
+import 'common.dart';
+import 'examples_loader_test.mocks.dart';
+
+@GenerateMocks([PlaygroundController, ExampleCache])
+void main() async {
+ late ExamplesLoader examplesLoader;
+ late MockPlaygroundController playgroundController;
+ final setExampleTrue = <String>[];
+ final setExampleFalse = <String>[];
+ final setEmptyIfNotExistsTrue = <Sdk>[];
+ final setEmptyIfNotExistsFalse = <Sdk>[];
+
+ setUp(() {
+ setExampleTrue.clear();
+ setExampleFalse.clear();
+ setEmptyIfNotExistsTrue.clear();
+ setEmptyIfNotExistsFalse.clear();
+
+ playgroundController = MockPlaygroundController();
+
+ when(playgroundController.setExample(any, setCurrentSdk: true))
+ .thenAnswer((realInvocation) {
+ final example = realInvocation.positionalArguments[0] as Example;
+ setExampleTrue.add(example.name);
+ });
+ when(playgroundController.setExample(any, setCurrentSdk: false))
+ .thenAnswer((realInvocation) {
+ final example = realInvocation.positionalArguments[0] as Example;
+ setExampleFalse.add(example.name);
+ });
+ when(playgroundController.setEmptyIfNotExists(any, setCurrentSdk: true))
+ .thenAnswer((realInvocation) {
+ final sdk = realInvocation.positionalArguments[0] as Sdk;
+ setEmptyIfNotExistsTrue.add(sdk);
+ });
+ when(playgroundController.setEmptyIfNotExists(any, setCurrentSdk: false))
+ .thenAnswer((realInvocation) {
+ final sdk = realInvocation.positionalArguments[0] as Sdk;
+ setEmptyIfNotExistsFalse.add(sdk);
+ });
+
+ final exampleCache = MockExampleCache();
+ when(playgroundController.exampleCache).thenReturn(exampleCache);
+
+ examplesLoader = ExamplesLoader();
+ examplesLoader.setPlaygroundController(playgroundController);
+ TestExampleLoader.register(examplesLoader.defaultFactory);
+ });
+
+ group('ExamplesLoader.', () {
+ group('load.', () {
+ group('Success.', () {
+ test('Race to set current SDK if not set', () async {
+ final descriptor = ExamplesLoadingDescriptor(
+ descriptors: const [
+ TestExampleLoadingDescriptor(Sdk.go),
+ TestExampleLoadingDescriptor(Sdk.python),
+ ],
+ lazyLoadDescriptors: {
+ Sdk.scio: const [TestExampleLoadingDescriptor(Sdk.scio)],
+ },
+ );
+
+ await examplesLoader.load(descriptor);
+
+ expect(setExampleTrue, [Sdk.go.id, Sdk.python.id]);
+ expect(setExampleFalse, []);
+ });
+
+ test('Example with initialSdk sets the current SDK', () async {
+ final descriptor = ExamplesLoadingDescriptor(
+ descriptors: const [
+ TestExampleLoadingDescriptor(Sdk.go),
+ TestExampleLoadingDescriptor(Sdk.python),
+ ],
+ lazyLoadDescriptors: {
+ Sdk.scio: const [TestExampleLoadingDescriptor(Sdk.scio)],
+ },
+ initialSdk: Sdk.python,
+ );
+
+ await examplesLoader.load(descriptor);
+
+ expect(setExampleTrue, [Sdk.python.id]);
+ expect(setExampleFalse, [Sdk.go.id]);
+ });
+ });
+
+ group('Error.', () {
+ test('Load empty example instead', () async {
+ Exception? thrown;
+ const descriptor = ExamplesLoadingDescriptor(
+ descriptors: [
+ TestExampleLoadingDescriptor(Sdk.go, ok: false),
+ TestExampleLoadingDescriptor(Sdk.python, ok: false),
+ ],
+ initialSdk: Sdk.python,
+ );
+
+ try {
+ await examplesLoader.load(descriptor);
+ } on ExampleLoadingException catch (ex) {
+ thrown = ex;
+ }
+
+ expect(thrown, isA<Exception>());
+ expect(setEmptyIfNotExistsTrue, [Sdk.python]);
+ expect(setEmptyIfNotExistsFalse, [Sdk.go]);
+ });
+ });
+
+ // TODO(alexeyinkin): Test lazy loading.
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031537962
##########
playground/frontend/lib/modules/examples/components/example_list/expansion_panel_item.dart:
##########
@@ -83,8 +81,7 @@ class ExpansionPanelItem extends StatelessWidget {
}
void _closeDropdown(ExampleCache exampleCache) {
- animationController.reverse();
- dropdown?.remove();
exampleCache.changeSelectorVisibility();
+ onSelected();
Review Comment:
No.
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033120534
##########
playground/frontend/lib/modules/examples/components/example_list/category_expansion_panel.dart:
##########
@@ -27,18 +27,15 @@ import 'package:playground_components/playground_components.dart';
class CategoryExpansionPanel extends StatelessWidget {
final String categoryName;
final List<ExampleBase> examples;
+ final VoidCallback onSelected;
final ExampleBase selectedExample;
- final AnimationController animationController;
- final OverlayEntry? dropdown;
const CategoryExpansionPanel({
- Key? key,
Review Comment:
So lets change out linter settings :)
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
pabloem merged PR #24338:
URL: https://github.com/apache/beam/pull/24338
--
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] nausharipov commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
nausharipov commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031420048
##########
playground/frontend/lib/modules/examples/examples_dropdown_content.dart:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:flutter_gen/gen_l10n/app_localizations.dart';
+import 'package:playground/constants/links.dart';
+import 'package:playground/constants/sizes.dart';
+import 'package:playground/modules/examples/components/example_list/example_list.dart';
+import 'package:playground/modules/examples/components/filter/filter.dart';
+import 'package:playground/modules/examples/components/search_field/search_field.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:url_launcher/url_launcher.dart';
+
+class ExamplesDropdownContent extends StatefulWidget {
+ final VoidCallback onSelected;
+ final PlaygroundController playgroundController;
+
+ const ExamplesDropdownContent({
+ required this.onSelected,
+ required this.playgroundController,
+ });
+
+ @override
+ State<ExamplesDropdownContent> createState() =>
+ _ExamplesDropdownContentState();
+}
+
+class _ExamplesDropdownContentState extends State<ExamplesDropdownContent> {
+ final _searchController = TextEditingController();
+
+ @override
+ Widget build(BuildContext context) {
+ switch (widget.playgroundController.exampleCache.catalogStatus) {
+ case LoadingStatus.error:
+ return const LoadingErrorWidget();
+
+ case LoadingStatus.loading:
+ return const LoadingIndicator();
+
+ case LoadingStatus.done:
+ return _buildLoaded();
+ }
+ }
+
+ Widget _buildLoaded() {
Review Comment:
It is not recommended to use helper methods over widgets: https://www.youtube.com/watch?v=IOyq-eTRhvo
##########
playground/frontend/playground_components/lib/src/cache/saving_exception.dart:
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:easy_localization/easy_localization.dart';
+
+class SavingException implements Exception {
Review Comment:
Why are `saving_exception` and `catalog_loading_exception` located in `cache`, while `example_loading_exception` is in` /exceptions`?
##########
playground/frontend/lib/modules/messages/handlers/set_content_message_handler.dart:
##########
@@ -34,11 +37,21 @@ class SetContentMessageHandler extends AbstractMessageHandler {
return MessageHandleResult.notHandled;
}
- _handle(message);
+ unawaited(_handle(message));
return MessageHandleResult.handled;
}
- void _handle(SetContentMessage message) {
- playgroundController.examplesLoader.load(message.descriptor);
+ Future<void> _handle(SetContentMessage message) async {
+ final descriptor = message.descriptor;
+
+ try {
+ await playgroundController.examplesLoader.load(descriptor);
Review Comment:
The code is repeated in `_load` of `playground_page_providers.dart`.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031513380
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -16,23 +16,21 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
-import 'package:flutter_gen/gen_l10n/app_localizations.dart';
-import 'package:playground/constants/links.dart';
import 'package:playground/constants/sizes.dart';
-import 'package:playground/modules/examples/components/examples_components.dart';
import 'package:playground/modules/examples/components/outside_click_handler.dart';
+import 'package:playground/modules/examples/examples_dropdown_content.dart';
import 'package:playground/modules/examples/models/popover_state.dart';
import 'package:playground/pages/playground/states/example_selector_state.dart';
import 'package:playground/utils/dropdown_utils.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
-import 'package:url_launcher/url_launcher.dart';
const int kAnimationDurationInMilliseconds = 80;
const Offset kAnimationBeginOffset = Offset(0.0, -0.02);
Review Comment:
They are used in another file for another purpose. :(
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031513092
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -50,36 +48,9 @@ class ExampleSelector extends StatefulWidget {
State<ExampleSelector> createState() => _ExampleSelectorState();
}
-class _ExampleSelectorState extends State<ExampleSelector>
- with TickerProviderStateMixin {
- final GlobalKey selectorKey = LabeledGlobalKey('ExampleSelector');
- late OverlayEntry? examplesDropdown;
- late AnimationController animationController;
- late Animation<Offset> offsetAnimation;
-
- final TextEditingController textController = TextEditingController();
- final ScrollController scrollController = ScrollController();
-
- @override
- void initState() {
- super.initState();
- animationController = AnimationController(
Review Comment:
This was too complicated and out of line with other dropdowns. If we need animation, we should make some code to reuse in all dropdowns and add animation there.
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031052205
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs/snippet_save_and_share_tabs.dart:
##########
@@ -16,32 +16,59 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
import 'package:playground/modules/editor/components/share_dropdown/share_tabs/example_share_tabs.dart';
import 'package:playground_components/playground_components.dart';
-class SnippetSaveAndShareTabs extends StatelessWidget {
+class SnippetSaveAndShareTabs extends StatefulWidget {
+ final VoidCallback onError;
final PlaygroundController playgroundController;
final TabController tabController;
const SnippetSaveAndShareTabs({
super.key,
+ required this.onError,
required this.playgroundController,
required this.tabController,
});
+ @override
+ State<SnippetSaveAndShareTabs> createState() => _SnippetSaveAndShareTabsState();
+}
+
+class _SnippetSaveAndShareTabsState extends State<SnippetSaveAndShareTabs> {
+ Future<String>? _future;
+
+ @override
+ void initState() {
+ super.initState();
+ unawaited(_initSaving());
+ }
+
+ Future<void> _initSaving() async {
+ try {
Review Comment:
I think that this logic should not be placed in a widget, because:
1. It breaks expectations on widget using. It will be surprise for users of this widget, that it makes request on creation.
2. This approach complicates potential testing.
3. It may reduce performance. What if the user closes popup while requests is running and open it again then?
I suggest to move this logic to playground controller. Run it and subscribe on it result here
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031538918
##########
playground/frontend/lib/modules/examples/examples_dropdown_content.dart:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:flutter_gen/gen_l10n/app_localizations.dart';
+import 'package:playground/constants/links.dart';
+import 'package:playground/constants/sizes.dart';
+import 'package:playground/modules/examples/components/example_list/example_list.dart';
+import 'package:playground/modules/examples/components/filter/filter.dart';
+import 'package:playground/modules/examples/components/search_field/search_field.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:url_launcher/url_launcher.dart';
+
+class ExamplesDropdownContent extends StatefulWidget {
+ final VoidCallback onSelected;
+ final PlaygroundController playgroundController;
+
+ const ExamplesDropdownContent({
+ required this.onSelected,
+ required this.playgroundController,
Review Comment:
I want to get rid of consumers completely at some point. See the Tour of Beam architecture.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031602814
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/examples_loader.dart:
##########
@@ -48,58 +50,98 @@ class ExamplesLoader {
_playgroundController = value;
}
+ /// Loads examples from [descriptor]'s immediate list.
+ ///
+ /// Sets empty editor for SDKs of failed examples.
Future<void> load(ExamplesLoadingDescriptor descriptor) async {
if (_descriptor == descriptor) {
return;
}
_descriptor = descriptor;
- await Future.wait(
- descriptor.descriptors.map(
- (one) => loadOne(group: descriptor, one: one),
- ),
- );
+ final loaders = descriptor.descriptors.map(_createLoader).whereNotNull();
+
+ try {
+ final loadFutures = loaders.map(_loadOne);
+ await Future.wait(loadFutures);
+ } on Exception catch (ex) {
+ _emptyMissing(loaders);
+ throw ExampleLoadingException(ex);
+ }
final sdk = descriptor.initialSdk;
if (sdk != null) {
_playgroundController!.setSdk(sdk);
}
}
- Future<void> loadDefaultIfAny(Sdk sdk) async {
- final group = _descriptor;
- final one = group?.lazyLoadDescriptors[sdk]?.firstOrNull;
+ ExampleLoader? _createLoader(ExampleLoadingDescriptor descriptor) {
+ final loader = defaultFactory.create(
+ descriptor: descriptor,
+ exampleCache: _playgroundController!.exampleCache,
+ );
- if (group == null || one == null) {
+ if (loader == null) {
+ // TODO: Log.
+ print('Cannot create example loader for $descriptor');
+ return null;
+ }
+
+ return loader;
+ }
+
+ void _emptyMissing(Iterable<ExampleLoader> loaders) {
+ loaders.forEach(_emptyIfMissing);
+ }
+
+ Future<void> _emptyIfMissing(ExampleLoader loader) async {
+ final sdk = loader.sdk;
+
+ if (sdk == null) {
return;
}
- return loadOne(
- group: group,
- one: one,
+ _playgroundController!.setEmptyIfNotExists(
+ sdk,
+ setCurrentSdk: _shouldSetCurrentSdk(sdk),
);
}
- Future<void> loadOne({
- required ExamplesLoadingDescriptor group,
- required ExampleLoadingDescriptor one,
- }) async {
- final loader = defaultFactory.create(
- descriptor: one,
- exampleCache: _playgroundController!.exampleCache,
- );
+ Future<void> loadDefaultIfAny(Sdk sdk) async {
+ final group = _descriptor;
Review Comment:
This is to avoid `!` on `_descriptor`. And it should be optimized away.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031599947
##########
playground/frontend/lib/modules/examples/models/example_loading_descriptors/examples_loading_descriptor_factory.dart:
##########
@@ -24,7 +24,7 @@ import 'package:playground/modules/examples/models/example_token_type.dart';
import 'package:playground_components/playground_components.dart';
class ExamplesLoadingDescriptorFactory {
- static const _defaultSdk = Sdk.java;
+ static const defaultSdk = Sdk.java;
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on PR #24338:
URL: https://github.com/apache/beam/pull/24338#issuecomment-1329006606
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] alexeyinkin commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043278965
##########
playground/frontend/lib/controllers/factories.dart:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+
+import 'package:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../constants/params.dart';
+import '../modules/messages/handlers/messages_debouncer.dart';
+import '../modules/messages/handlers/messages_handler.dart';
+import '../modules/messages/listeners/messages_listener.dart';
+
+PlaygroundController createPlaygroundController(
+ ExamplesLoadingDescriptor descriptor,
+) {
+ final exampleCache = ExampleCache(
+ exampleRepository: GetIt.instance.get<ExampleRepository>(),
+ );
+
+ final controller = PlaygroundController(
+ examplesLoader: ExamplesLoader(),
+ exampleCache: exampleCache,
+ codeRepository: GetIt.instance.get<CodeRepository>(),
+ );
+
+ unawaited(_loadExamples(controller, descriptor));
+
+ final handler = MessagesDebouncer(
+ handler: MessagesHandler(playgroundController: controller),
+ );
+ MessagesListener(handler: handler);
+
+ return controller;
+}
+
+Future<void> _loadExamples(
Review Comment:
It shows Playground's errors on exceptions and thus cannot be in the shared `playground_components` package.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043283902
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_button.dart:
##########
@@ -18,16 +18,21 @@
import 'package:flutter/material.dart';
import 'package:flutter_gen/gen_l10n/app_localizations.dart';
-import 'package:playground/components/dropdown_button/dropdown_button.dart';
-import 'package:playground/modules/editor/components/share_dropdown/share_dropdown_body.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../../components/dropdown_button/dropdown_button.dart';
+import 'share_dropdown_body.dart';
+
const _kShareDropdownHeight = 140.0;
const _kShareDropdownWidth = 460.0;
const _kButtonColorOpacity = 0.2;
class ShareButton extends StatelessWidget {
- const ShareButton({super.key});
+ final PlaygroundController playgroundController;
Review Comment:
I want to get rid of all consumers and providers. I will reconsider as soon as they have compile-time guarantee of presence of a provider in the tree. :)
In the sharing widgets `PlaygroundController` is really passed around too much, but that is because of poor separation between those widgets. We may want to refactor those later.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031153982
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs/snippet_save_and_share_tabs.dart:
##########
@@ -16,32 +16,59 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
import 'package:playground/modules/editor/components/share_dropdown/share_tabs/example_share_tabs.dart';
import 'package:playground_components/playground_components.dart';
-class SnippetSaveAndShareTabs extends StatelessWidget {
+class SnippetSaveAndShareTabs extends StatefulWidget {
+ final VoidCallback onError;
final PlaygroundController playgroundController;
final TabController tabController;
const SnippetSaveAndShareTabs({
super.key,
+ required this.onError,
required this.playgroundController,
required this.tabController,
});
+ @override
+ State<SnippetSaveAndShareTabs> createState() => _SnippetSaveAndShareTabsState();
+}
+
+class _SnippetSaveAndShareTabsState extends State<SnippetSaveAndShareTabs> {
+ Future<String>? _future;
+
+ @override
+ void initState() {
+ super.initState();
+ unawaited(_initSaving());
+ }
+
+ Future<void> _initSaving() async {
+ try {
Review Comment:
I agree this is odd, but this is the way it was made originally. Though:
1. This is declarative saving. This is easier to maintain than some imperative approach.
2. A widget test can cover this easily.
3. Saving is idemponent, so no problems so far. Also not many users are quick enough to close the dropdown and reopen before saving completes.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031579889
##########
playground/frontend/lib/modules/messages/handlers/set_content_message_handler.dart:
##########
@@ -34,11 +37,21 @@ class SetContentMessageHandler extends AbstractMessageHandler {
return MessageHandleResult.notHandled;
}
- _handle(message);
+ unawaited(_handle(message));
return MessageHandleResult.handled;
}
- void _handle(SetContentMessage message) {
- playgroundController.examplesLoader.load(message.descriptor);
+ Future<void> _handle(SetContentMessage message) async {
+ final descriptor = message.descriptor;
+
+ try {
+ await playgroundController.examplesLoader.load(descriptor);
+ } on Exception catch (ex) {
+ PlaygroundComponents.toastNotifier.addException(ex);
Review Comment:
`ExamplesLoader` is in `playground_components` package shared by Playground and Tour of Beam. This package allows for toasts but should not itself create ones because it will be hard to suppress them if Tour of Beam will ever require silent failures or its custom errors.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043277601
##########
playground/frontend/lib/controllers/factories.dart:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+
+import 'package:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../constants/params.dart';
+import '../modules/messages/handlers/messages_debouncer.dart';
+import '../modules/messages/handlers/messages_handler.dart';
+import '../modules/messages/listeners/messages_listener.dart';
+
+PlaygroundController createPlaygroundController(
+ ExamplesLoadingDescriptor descriptor,
+) {
+ final exampleCache = ExampleCache(
+ exampleRepository: GetIt.instance.get<ExampleRepository>(),
+ );
+
+ final controller = PlaygroundController(
+ examplesLoader: ExamplesLoader(),
+ exampleCache: exampleCache,
+ codeRepository: GetIt.instance.get<CodeRepository>(),
+ );
+
+ unawaited(_loadExamples(controller, descriptor));
+
+ final handler = MessagesDebouncer(
+ handler: MessagesHandler(playgroundController: controller),
+ );
+ MessagesListener(handler: handler);
Review Comment:
Added a doc comment on `MessageListener`. As for referencing it later, we should wait until we need this.
Anyway, this is a moved code and not a new one.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043288518
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -16,70 +16,38 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
-import 'package:flutter_gen/gen_l10n/app_localizations.dart';
-import 'package:playground/constants/links.dart';
-import 'package:playground/constants/sizes.dart';
-import 'package:playground/modules/examples/components/examples_components.dart';
-import 'package:playground/modules/examples/components/outside_click_handler.dart';
-import 'package:playground/modules/examples/models/popover_state.dart';
-import 'package:playground/pages/playground/states/example_selector_state.dart';
-import 'package:playground/utils/dropdown_utils.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
-import 'package:url_launcher/url_launcher.dart';
-const int kAnimationDurationInMilliseconds = 80;
-const Offset kAnimationBeginOffset = Offset(0.0, -0.02);
-const Offset kAnimationEndOffset = Offset(0.0, 0.0);
-const double kAdditionalDyAlignment = 50.0;
+import '../../constants/sizes.dart';
+import '../../pages/standalone_playground/notifiers/example_selector_state.dart';
+import '../../utils/dropdown_utils.dart';
+import 'components/outside_click_handler.dart';
+import 'examples_dropdown_content.dart';
+import 'models/popover_state.dart';
+
const double kLgContainerHeight = 490.0;
const double kLgContainerWidth = 400.0;
class ExampleSelector extends StatefulWidget {
- final void Function() changeSelectorVisibility;
final bool isSelectorOpened;
+ final PlaygroundController playgroundController;
const ExampleSelector({
- Key? key,
- required this.changeSelectorVisibility,
required this.isSelectorOpened,
Review Comment:
There should just be no `isSelectorOpened` anywhere. https://github.com/apache/beam/issues/24371
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043293802
##########
playground/frontend/lib/pages/standalone_playground/screen.dart:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../../components/logo/logo_component.dart';
+import '../../constants/sizes.dart';
+import '../../modules/actions/components/new_example_action.dart';
+import '../../modules/actions/components/reset_action.dart';
+import '../../modules/analytics/analytics_service.dart';
+import '../../modules/editor/components/pipeline_options_dropdown/pipeline_options_dropdown.dart';
+import '../../modules/examples/example_selector.dart';
+import '../../modules/sdk/components/sdk_selector.dart';
+import '../../modules/shortcuts/components/shortcuts_manager.dart';
+import '../../modules/shortcuts/constants/global_shortcuts.dart';
+import 'state.dart';
+import 'widgets/close_listener_nonweb.dart'
+ if (dart.library.html) 'widgets/close_listener.dart';
+import 'widgets/more_actions.dart';
+import 'widgets/playground_page_body.dart';
+import 'widgets/playground_page_footer.dart';
+import 'widgets/playground_page_providers.dart';
+
+class StandalonePlaygroundScreen extends StatelessWidget {
+ final StandalonePlaygroundNotifier notifier;
+
+ const StandalonePlaygroundScreen(this.notifier);
+
+ @override
+ Widget build(BuildContext context) {
+ final snippetController =
+ notifier.playgroundController.snippetEditingController;
+
+ //print('Building PlaygroundPage $n');
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1043292258
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -118,20 +85,29 @@ class _ExampleSelectorState extends State<ExampleSelector>
);
}
- OverlayEntry createExamplesDropdown() {
- Offset dropdownOffset = findDropdownOffset(key: selectorKey);
+ Future<void> _loadCatalogIfNot(PlaygroundController controller) async {
+ try {
+ await controller.exampleCache.loadAllPrecompiledObjectsIfNot();
+ } on Exception catch (ex) {
+ PlaygroundComponents.toastNotifier.addException(ex);
+ }
+ }
+
+ OverlayEntry _createExamplesDropdown() {
+ Offset dropdownOffset = findDropdownOffset(key: _selectorKey);
return OverlayEntry(
builder: (context) {
return ChangeNotifierProvider<PopoverState>(
create: (context) => PopoverState(false),
builder: (context, state) {
- return Consumer<PlaygroundController>(
- builder: (context, playgroundController, child) => Stack(
+ return ChangeNotifierProvider<PlaygroundController>.value(
+ value: widget.playgroundController,
Review Comment:
There is no more `PlaygroundController` above in the tree.
This is what I mean when stressing compile-time guarantee.
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031616631
##########
playground/frontend/playground_components/lib/src/widgets/toasts/toast_listener.dart:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+
+import 'package:flutter/material.dart';
+import 'package:fluttertoast/fluttertoast.dart';
Review Comment:
`+`
--
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 #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1031581763
##########
playground/frontend/playground_components/lib/src/cache/example_cache.dart:
##########
@@ -175,55 +191,61 @@ class ExampleCache extends ChangeNotifier {
);
}
- Future<void> _loadCategories() async {
- final result = await _exampleRepository.getListOfExamples(
+ Future<void> _loadAllPrecompiledObjects() async {
+ final result = await _exampleRepository.getPrecompiledObjects(
const GetPrecompiledObjectsRequest(
Review Comment:
Not this time, this PR is large already.
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1044117027
##########
playground/frontend/playground_components/lib/src/widgets/toasts/toast.dart:
##########
@@ -66,27 +74,32 @@ class BaseNotification extends StatelessWidget {
);
}
- Widget _renderNotificationContent(BuildContext context) {
+ Widget _content(BuildContext context) {
final textTheme = Theme.of(context).textTheme.bodyText1;
Review Comment:
<img width="1085" alt="Π‘Π½ΠΈΠΌΠΎΠΊ ΡΠΊΡΠ°Π½Π° 2022-12-09 Π² 10 03 26" src="https://user-images.githubusercontent.com/17158960/206635458-643aa4ec-38f9-40a5-b0c7-673371ae418c.png">
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1044151719
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_button.dart:
##########
@@ -18,16 +18,21 @@
import 'package:flutter/material.dart';
import 'package:flutter_gen/gen_l10n/app_localizations.dart';
-import 'package:playground/components/dropdown_button/dropdown_button.dart';
-import 'package:playground/modules/editor/components/share_dropdown/share_dropdown_body.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../../components/dropdown_button/dropdown_button.dart';
+import 'share_dropdown_body.dart';
+
const _kShareDropdownHeight = 140.0;
const _kShareDropdownWidth = 460.0;
const _kButtonColorOpacity = 0.2;
class ShareButton extends StatelessWidget {
- const ShareButton({super.key});
+ final PlaygroundController playgroundController;
Review Comment:
I think, that compile-time guarantee of presence instance class will never be done in flutter, because, widget tree can be changes at runtime.
For example, look at this code:
```
class Blue42 extends StatefulWidget {
final Widget child;
const Blue42({super.key, required this.child});
@override
_Blue42State createState() => _Blue42State();
}
class _Blue42State extends State<Blue42> {
@override
Widget build(BuildContext context) {
final isOddSecond = DateTime.now().second.isOdd;
if (isOddSecond) {
return Provider.value(value: Colors.blue, child: widget.child);
} else {
return Provider.value(value: 42, child: widget.child);
}
}
}
```
You will never be sure, What exactly exists in the tree - `Colors.blue` or `42`. It's absolutely synthetic example, but it demonstrates that it's impossible to guarantee at compile-time that tree contains specific object. I'm sure, that it's not a reason for ridding of provider opportunities because of obtaining compile time guarantee. Eventually even Flutter use this mechanism every time when you use `Theme.of` or `MediaQuery.of`, so you trust flutter that these objects exists in the tree. We can trust ourselves too, so I don't think it will be a problem if we will use provider for objects which have to exist in a whole application lifecycle.
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1041764844
##########
playground/frontend/lib/controllers/factories.dart:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+
+import 'package:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../constants/params.dart';
+import '../modules/messages/handlers/messages_debouncer.dart';
+import '../modules/messages/handlers/messages_handler.dart';
+import '../modules/messages/listeners/messages_listener.dart';
+
+PlaygroundController createPlaygroundController(
+ ExamplesLoadingDescriptor descriptor,
+) {
+ final exampleCache = ExampleCache(
+ exampleRepository: GetIt.instance.get<ExampleRepository>(),
+ );
+
+ final controller = PlaygroundController(
+ examplesLoader: ExamplesLoader(),
+ exampleCache: exampleCache,
+ codeRepository: GetIt.instance.get<CodeRepository>(),
+ );
+
+ unawaited(_loadExamples(controller, descriptor));
+
+ final handler = MessagesDebouncer(
+ handler: MessagesHandler(playgroundController: controller),
+ );
+ MessagesListener(handler: handler);
Review Comment:
`MessageListener` created here, but there aren't any way to get it's instance after. For example, to change handler. I don't think that necessary to change this now, just point to possible refactoring. Also I think, that this interface is unclear, because:
1. It's hard to find, when it's required
2. It's hard to understand. What especially messages mean? I already worked with it, so I know. But when I investigated this I have to google this lib at pub.dev to understand.
##########
playground/frontend/lib/controllers/factories.dart:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+
+import 'package:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../constants/params.dart';
+import '../modules/messages/handlers/messages_debouncer.dart';
+import '../modules/messages/handlers/messages_handler.dart';
+import '../modules/messages/listeners/messages_listener.dart';
+
+PlaygroundController createPlaygroundController(
+ ExamplesLoadingDescriptor descriptor,
+) {
+ final exampleCache = ExampleCache(
+ exampleRepository: GetIt.instance.get<ExampleRepository>(),
+ );
+
+ final controller = PlaygroundController(
+ examplesLoader: ExamplesLoader(),
+ exampleCache: exampleCache,
+ codeRepository: GetIt.instance.get<CodeRepository>(),
+ );
+
+ unawaited(_loadExamples(controller, descriptor));
+
+ final handler = MessagesDebouncer(
+ handler: MessagesHandler(playgroundController: controller),
+ );
+ MessagesListener(handler: handler);
+
+ return controller;
+}
+
+Future<void> _loadExamples(
Review Comment:
This method manipulates with `PlaygroundController`. Suppose will be better to move this logic inside it.
##########
playground/frontend/lib/modules/examples/components/example_list/example_list.dart:
##########
@@ -17,24 +17,27 @@
*/
import 'package:flutter/material.dart';
-import 'package:playground/modules/examples/components/examples_components.dart';
-import 'package:playground/pages/playground/states/example_selector_state.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
-class ExampleList extends StatelessWidget {
- final ScrollController controller;
- final AnimationController animationController;
- final OverlayEntry? dropdown;
+import '../../../../pages/standalone_playground/notifiers/example_selector_state.dart';
+import '../examples_components.dart';
+
+class ExampleList extends StatefulWidget {
+ final VoidCallback onSelected;
final ExampleBase selectedExample;
const ExampleList({
- Key? key,
- required this.controller,
+ required this.onSelected,
required this.selectedExample,
- required this.animationController,
- required this.dropdown,
- }) : super(key: key);
+ });
+
+ @override
+ State<ExampleList> createState() => _ExampleListState();
+}
+
+class _ExampleListState extends State<ExampleList> {
+ final _scrollController = ScrollController();
@override
Widget build(BuildContext context) {
Review Comment:
backgroundColor is deprecated at line 47. Use colorScheme.background instead
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -16,70 +16,38 @@
* limitations under the License.
*/
+import 'dart:async';
+
import 'package:flutter/material.dart';
-import 'package:flutter_gen/gen_l10n/app_localizations.dart';
-import 'package:playground/constants/links.dart';
-import 'package:playground/constants/sizes.dart';
-import 'package:playground/modules/examples/components/examples_components.dart';
-import 'package:playground/modules/examples/components/outside_click_handler.dart';
-import 'package:playground/modules/examples/models/popover_state.dart';
-import 'package:playground/pages/playground/states/example_selector_state.dart';
-import 'package:playground/utils/dropdown_utils.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
-import 'package:url_launcher/url_launcher.dart';
-const int kAnimationDurationInMilliseconds = 80;
-const Offset kAnimationBeginOffset = Offset(0.0, -0.02);
-const Offset kAnimationEndOffset = Offset(0.0, 0.0);
-const double kAdditionalDyAlignment = 50.0;
+import '../../constants/sizes.dart';
+import '../../pages/standalone_playground/notifiers/example_selector_state.dart';
+import '../../utils/dropdown_utils.dart';
+import 'components/outside_click_handler.dart';
+import 'examples_dropdown_content.dart';
+import 'models/popover_state.dart';
+
const double kLgContainerHeight = 490.0;
const double kLgContainerWidth = 400.0;
class ExampleSelector extends StatefulWidget {
- final void Function() changeSelectorVisibility;
final bool isSelectorOpened;
+ final PlaygroundController playgroundController;
const ExampleSelector({
- Key? key,
- required this.changeSelectorVisibility,
required this.isSelectorOpened,
Review Comment:
`isSelectorOpened` is inner `PlaygroundController` value. Suppose we can rid of this param
##########
playground/frontend/lib/pages/standalone_playground/screen.dart:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../../components/logo/logo_component.dart';
+import '../../constants/sizes.dart';
+import '../../modules/actions/components/new_example_action.dart';
+import '../../modules/actions/components/reset_action.dart';
+import '../../modules/analytics/analytics_service.dart';
+import '../../modules/editor/components/pipeline_options_dropdown/pipeline_options_dropdown.dart';
+import '../../modules/examples/example_selector.dart';
+import '../../modules/sdk/components/sdk_selector.dart';
+import '../../modules/shortcuts/components/shortcuts_manager.dart';
+import '../../modules/shortcuts/constants/global_shortcuts.dart';
+import 'state.dart';
+import 'widgets/close_listener_nonweb.dart'
+ if (dart.library.html) 'widgets/close_listener.dart';
+import 'widgets/more_actions.dart';
+import 'widgets/playground_page_body.dart';
+import 'widgets/playground_page_footer.dart';
+import 'widgets/playground_page_providers.dart';
+
+class StandalonePlaygroundScreen extends StatelessWidget {
+ final StandalonePlaygroundNotifier notifier;
+
+ const StandalonePlaygroundScreen(this.notifier);
+
+ @override
+ Widget build(BuildContext context) {
+ final snippetController =
+ notifier.playgroundController.snippetEditingController;
+
+ //print('Building PlaygroundPage $n');
Review Comment:
redundant comment?
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -118,20 +85,29 @@ class _ExampleSelectorState extends State<ExampleSelector>
);
}
- OverlayEntry createExamplesDropdown() {
- Offset dropdownOffset = findDropdownOffset(key: selectorKey);
+ Future<void> _loadCatalogIfNot(PlaygroundController controller) async {
+ try {
+ await controller.exampleCache.loadAllPrecompiledObjectsIfNot();
+ } on Exception catch (ex) {
+ PlaygroundComponents.toastNotifier.addException(ex);
+ }
+ }
+
+ OverlayEntry _createExamplesDropdown() {
+ Offset dropdownOffset = findDropdownOffset(key: _selectorKey);
return OverlayEntry(
builder: (context) {
return ChangeNotifierProvider<PopoverState>(
create: (context) => PopoverState(false),
builder: (context, state) {
- return Consumer<PlaygroundController>(
- builder: (context, playgroundController, child) => Stack(
+ return ChangeNotifierProvider<PlaygroundController>.value(
+ value: widget.playgroundController,
Review Comment:
`context.read<PlaygroundController>()`?
##########
playground/frontend/playground_components/lib/src/cache/example_cache.dart:
##########
@@ -19,11 +19,16 @@
import 'dart:async';
import 'package:collection/collection.dart';
-import 'package:flutter/material.dart';
+import 'package:flutter/foundation.dart';
+import '../exceptions/catalog_loading_exception.dart';
+import '../exceptions/example_loading_exception.dart';
+import '../exceptions/snippet_saving_exception.dart';
import '../models/category_with_examples.dart';
import '../models/example.dart';
import '../models/example_base.dart';
+import '../models/example_loading_descriptors/user_shared_example_loading_descriptor.dart';
Review Comment:
unused import
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -89,27 +57,26 @@ class _ExampleSelectorState extends State<ExampleSelector>
color: Theme.of(context).dividerColor,
borderRadius: BorderRadius.circular(kSmBorderRadius),
),
- child: Consumer<PlaygroundController>(
Review Comment:
Why do you prefer pass `PlaygroundController` in constructor instead of using `Consumer` here?
##########
playground/frontend/lib/locator.dart:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:app_state/app_state.dart';
+import 'package:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
+
+import 'config.g.dart';
+import 'pages/loading/page.dart';
+import 'router/page_factory.dart';
+import 'router/route_information_parser.dart';
+
+Future<void> initializeServiceLocator() async {
Review Comment:
Also think that will be better to make this method as a static method of a class, or move to main, because it is not used else where
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -89,27 +57,26 @@ class _ExampleSelectorState extends State<ExampleSelector>
color: Theme.of(context).dividerColor,
borderRadius: BorderRadius.circular(kSmBorderRadius),
),
- child: Consumer<PlaygroundController>(
- builder: (context, state, child) => TextButton(
- key: selectorKey,
+ child: ChangeNotifierProvider<PlaygroundController>.value(
+ value: widget.playgroundController,
+ builder: (context, child) => TextButton(
+ key: _selectorKey,
onPressed: () {
if (widget.isSelectorOpened) {
- animationController.reverse();
- examplesDropdown?.remove();
+ _overlayEntry?.remove();
+ widget.playgroundController.exampleCache.setSelectorOpened(false);
} else {
- animationController.forward();
- examplesDropdown = createExamplesDropdown();
- Overlay.of(context)?.insert(examplesDropdown!);
+ unawaited(_loadCatalogIfNot(widget.playgroundController));
+ _overlayEntry = _createExamplesDropdown();
+ Overlay.of(context)?.insert(_overlayEntry!);
Review Comment:
unnecessary `?`
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -89,27 +57,26 @@ class _ExampleSelectorState extends State<ExampleSelector>
color: Theme.of(context).dividerColor,
borderRadius: BorderRadius.circular(kSmBorderRadius),
),
- child: Consumer<PlaygroundController>(
- builder: (context, state, child) => TextButton(
- key: selectorKey,
+ child: ChangeNotifierProvider<PlaygroundController>.value(
Review Comment:
I think there are a lot of cases, when `PlaygroundController` could be changed. And `ExampleSelector` and `ShareButtonBody` shouldn't know about all of then. I think it will be better to split `PlaygroundController` in the future
##########
playground/frontend/lib/controllers/factories.dart:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+
+import 'package:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../constants/params.dart';
+import '../modules/messages/handlers/messages_debouncer.dart';
+import '../modules/messages/handlers/messages_handler.dart';
+import '../modules/messages/listeners/messages_listener.dart';
+
+PlaygroundController createPlaygroundController(
Review Comment:
May be will be better to create ordinary `PlaygroundControllerBuilder` with static method `build`? It will be more clear for users, that playground controller created in separate place
##########
playground/frontend/lib/pages/standalone_playground/widgets/editor_textarea_wrapper.dart:
##########
@@ -83,7 +85,9 @@ class CodeTextAreaWrapper extends StatelessWidget {
],
Semantics(
container: true,
- child: const ShareButton(),
+ child: ShareButton(
+ playgroundController: controller,
Review Comment:
also consider `context.read`
##########
playground/frontend/lib/modules/examples/examples_dropdown_content.dart:
##########
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:flutter_gen/gen_l10n/app_localizations.dart';
+import 'package:playground/constants/links.dart';
+import 'package:playground/constants/sizes.dart';
+import 'package:playground/modules/examples/components/example_list/example_list.dart';
+import 'package:playground/modules/examples/components/filter/filter.dart';
+import 'package:playground/modules/examples/components/search_field/search_field.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:url_launcher/url_launcher.dart';
+
+class ExamplesDropdownContent extends StatefulWidget {
+ final VoidCallback onSelected;
+ final PlaygroundController playgroundController;
+
+ const ExamplesDropdownContent({
+ required this.onSelected,
+ required this.playgroundController,
+ });
+
+ @override
+ State<ExamplesDropdownContent> createState() =>
+ _ExamplesDropdownContentState();
+}
+
+class _ExamplesDropdownContentState extends State<ExamplesDropdownContent> {
+ final _searchController = TextEditingController();
+
+ @override
+ Widget build(BuildContext context) {
+ final exampleCache = widget.playgroundController.exampleCache;
Review Comment:
`context.read<PlaygroundController>()`?
##########
playground/frontend/lib/pages/standalone_playground/screen.dart:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../../components/logo/logo_component.dart';
+import '../../constants/sizes.dart';
+import '../../modules/actions/components/new_example_action.dart';
+import '../../modules/actions/components/reset_action.dart';
+import '../../modules/analytics/analytics_service.dart';
+import '../../modules/editor/components/pipeline_options_dropdown/pipeline_options_dropdown.dart';
+import '../../modules/examples/example_selector.dart';
+import '../../modules/sdk/components/sdk_selector.dart';
+import '../../modules/shortcuts/components/shortcuts_manager.dart';
+import '../../modules/shortcuts/constants/global_shortcuts.dart';
+import 'state.dart';
+import 'widgets/close_listener_nonweb.dart'
Review Comment:
May be will be better to manage this logic inside `CloseListener`?
##########
playground/frontend/playground_components/lib/src/controllers/playground_controller.dart:
##########
@@ -137,8 +140,38 @@ class PlaygroundController with ChangeNotifier {
selectedExample?.type != ExampleType.test &&
[Sdk.java, Sdk.python].contains(sdk);
+ /// If no SDK is selected, sets it to [sdk] and creates an empty state for it.
+ void setEmptyIfNoSdk(Sdk sdk) {
Review Comment:
All users of this method set `defaultSdk` if their own value is null. Suggest to move this logic here
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_button.dart:
##########
@@ -18,16 +18,21 @@
import 'package:flutter/material.dart';
import 'package:flutter_gen/gen_l10n/app_localizations.dart';
-import 'package:playground/components/dropdown_button/dropdown_button.dart';
-import 'package:playground/modules/editor/components/share_dropdown/share_dropdown_body.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../../components/dropdown_button/dropdown_button.dart';
+import 'share_dropdown_body.dart';
+
const _kShareDropdownHeight = 140.0;
const _kShareDropdownWidth = 460.0;
const _kButtonColorOpacity = 0.2;
class ShareButton extends StatelessWidget {
- const ShareButton({super.key});
+ final PlaygroundController playgroundController;
Review Comment:
I think it is not necessary to pass `playgroundController` here. You already use `Consumer` at `ShareTabs`. Suggest to remove `ChangeNotifierProvider` from `ShareDropdownBody` and pass it inside `Consumer` at `ShareTabs`
##########
playground/frontend/playground_components/lib/src/widgets/toasts/toast.dart:
##########
@@ -19,44 +19,52 @@
import 'package:flutter/material.dart';
import 'package:flutter_svg/flutter_svg.dart';
-import '../constants/sizes.dart';
+import '../../assets/assets.gen.dart';
+import '../../constants/colors.dart';
+import '../../constants/sizes.dart';
+import '../../models/toast.dart';
+import '../../models/toast_type.dart';
+import '../../playground_components.dart';
-const kNotificationBorderWidth = 4.0;
-const kMaxTextWidth = 300.0;
+const _borderWidth = 4.0;
+const _textWidth = 300.0;
-class BaseNotification extends StatelessWidget {
- final String title;
- final String notification;
- final Color color;
- final String asset;
+const _colors = UnmodifiableToastTypeMap(
+ error: BeamNotificationColors.error,
+ info: BeamNotificationColors.info,
+);
- const BaseNotification({
- super.key,
- required this.title,
- required this.notification,
- required this.color,
- required this.asset,
- });
+final _iconNames = UnmodifiableToastTypeMap(
+ error: Assets.notificationIcons.error,
+ info: Assets.notificationIcons.info,
+);
+
+class ToastWidget extends StatelessWidget {
+ final Toast toast;
+
+ const ToastWidget(this.toast);
@override
Widget build(BuildContext context) {
- return Stack(
- children: [
- _renderLeftBorder(context),
- _renderNotificationContent(context),
- ],
+ return Card(
+ child: Stack(
+ children: [
+ _leftBorder(context),
Review Comment:
May be change on stateless widgets?
##########
playground/frontend/lib/pages/embedded_playground/screen.dart:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../standalone_playground/widgets/playground_page_providers.dart';
+import 'state.dart';
+import 'widgets/embedded_actions.dart';
+import 'widgets/embedded_appbar_title.dart';
+import 'widgets/embedded_editor.dart';
+import 'widgets/embedded_split_view.dart';
+
+const kActionsWidth = 300.0;
+const kActionsHeight = 40.0;
+
+class EmbeddedPlaygroundScreen extends StatelessWidget {
+ final EmbeddedPlaygroundNotifier notifier;
+
+ const EmbeddedPlaygroundScreen(this.notifier);
+
+ @override
+ Widget build(BuildContext context) {
+ return PlaygroundPageProviders(
+ playgroundController: notifier.playgroundController,
+ child: ToastListenerWidget(
+ child: Scaffold(
+ appBar: AppBar(
+ automaticallyImplyLeading: false,
+ title: const EmbeddedAppBarTitle(),
+ actions: const [EmbeddedActions()],
+ ),
+ body: EmbeddedSplitView(
+ first: EmbeddedEditor(isEditable: notifier.isEditable),
+ second: Container(
+ color: Theme.of(context).backgroundColor,
Review Comment:
deprecated `backgroundColor`
##########
playground/frontend/playground_components/lib/src/widgets/toasts/toast.dart:
##########
@@ -66,27 +74,32 @@ class BaseNotification extends StatelessWidget {
);
}
- Widget _renderNotificationContent(BuildContext context) {
+ Widget _content(BuildContext context) {
final textTheme = Theme.of(context).textTheme.bodyText1;
Review Comment:
deprecated
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033130147
##########
playground/frontend/lib/modules/examples/example_selector.dart:
##########
@@ -50,36 +48,9 @@ class ExampleSelector extends StatefulWidget {
State<ExampleSelector> createState() => _ExampleSelectorState();
}
-class _ExampleSelectorState extends State<ExampleSelector>
- with TickerProviderStateMixin {
- final GlobalKey selectorKey = LabeledGlobalKey('ExampleSelector');
- late OverlayEntry? examplesDropdown;
- late AnimationController animationController;
- late Animation<Offset> offsetAnimation;
-
- final TextEditingController textController = TextEditingController();
- final ScrollController scrollController = ScrollController();
-
- @override
- void initState() {
- super.initState();
- animationController = AnimationController(
Review Comment:
Another dropdowns also uses this animation, but all of them uses separate widget for this: `AppDropdownButton`. So if you remove animation from this widget now, it breaks UX. Suggest to return these 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
[GitHub] [beam] alexeyinkin commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033223213
##########
playground/frontend/playground_components/test/src/controllers/example_loaders/common.dart:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:playground_components/playground_components.dart';
+
+import 'package:playground_components/src/controllers/example_loaders/example_loader.dart';
+import 'package:playground_components/src/controllers/example_loaders/example_loader_factory.dart';
+
+class TestExampleLoadingDescriptor extends ExampleLoadingDescriptor {
+ final Sdk? sdk;
+ final bool ok;
Review Comment:
`+`
--
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] Malarg commented on a diff in pull request #24338: Show error messages when Playground requests fail (#24033)
Posted by GitBox <gi...@apache.org>.
Malarg commented on code in PR #24338:
URL: https://github.com/apache/beam/pull/24338#discussion_r1033142284
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/examples_loader.dart:
##########
@@ -48,58 +50,98 @@ class ExamplesLoader {
_playgroundController = value;
}
+ /// Loads examples from [descriptor]'s immediate list.
+ ///
+ /// Sets empty editor for SDKs of failed examples.
Future<void> load(ExamplesLoadingDescriptor descriptor) async {
if (_descriptor == descriptor) {
return;
}
_descriptor = descriptor;
- await Future.wait(
- descriptor.descriptors.map(
- (one) => loadOne(group: descriptor, one: one),
- ),
- );
+ final loaders = descriptor.descriptors.map(_createLoader).whereNotNull();
+
+ try {
+ final loadFutures = loaders.map(_loadOne);
+ await Future.wait(loadFutures);
+ } on Exception catch (ex) {
+ _emptyMissing(loaders);
+ throw ExampleLoadingException(ex);
+ }
final sdk = descriptor.initialSdk;
if (sdk != null) {
_playgroundController!.setSdk(sdk);
}
}
- Future<void> loadDefaultIfAny(Sdk sdk) async {
- final group = _descriptor;
- final one = group?.lazyLoadDescriptors[sdk]?.firstOrNull;
+ ExampleLoader? _createLoader(ExampleLoadingDescriptor descriptor) {
+ final loader = defaultFactory.create(
+ descriptor: descriptor,
+ exampleCache: _playgroundController!.exampleCache,
+ );
- if (group == null || one == null) {
+ if (loader == null) {
+ // TODO: Log.
+ print('Cannot create example loader for $descriptor');
+ return null;
+ }
+
+ return loader;
+ }
+
+ void _emptyMissing(Iterable<ExampleLoader> loaders) {
+ loaders.forEach(_emptyIfMissing);
+ }
+
+ Future<void> _emptyIfMissing(ExampleLoader loader) async {
+ final sdk = loader.sdk;
+
+ if (sdk == null) {
return;
}
- return loadOne(
- group: group,
- one: one,
+ _playgroundController!.setEmptyIfNotExists(
+ sdk,
+ setCurrentSdk: _shouldSetCurrentSdk(sdk),
);
}
- Future<void> loadOne({
- required ExamplesLoadingDescriptor group,
- required ExampleLoadingDescriptor one,
- }) async {
- final loader = defaultFactory.create(
- descriptor: one,
- exampleCache: _playgroundController!.exampleCache,
- );
+ Future<void> loadDefaultIfAny(Sdk sdk) async {
+ final group = _descriptor;
Review Comment:
I don't see any reasons to add `!` here. This method already contain null check:
```
Future<void> loadDefaultIfAny(Sdk sdk) async {
final one = _descriptor?.lazyLoadDescriptors[sdk]?.firstOrNull;
if (_descriptor == null || one == null) {
return;
}
final loader = _createLoader(one);
if (loader == null) {
return;
}
await _loadOne(loader);
}
```
--
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