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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
