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