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 2023/01/04 09:59:36 UTC
[GitHub] [beam] Malarg commented on a diff in pull request #24865: Multifile examples on frontend (#24859)
Malarg commented on code in PR #24865:
URL: https://github.com/apache/beam/pull/24865#discussion_r1061182400
##########
playground/frontend/lib/pages/standalone_playground/widgets/editor_textarea_wrapper.dart:
##########
@@ -47,60 +46,34 @@ class CodeTextAreaWrapper extends StatelessWidget {
return const LoadingIndicator();
}
- return Column(
- children: [
- Expanded(
- child: Stack(
- children: [
- Positioned.fill(
- child: SnippetEditor(
- controller: snippetController,
- isEditable: true,
- ),
+ return SnippetEditor(
+ controller: snippetController,
+ isEditable: true,
+ actionsWidget: Row(
Review Comment:
Suggest to extract as a stateless widget
##########
playground/frontend/playground_components/lib/src/controllers/example_loaders/http_example_loader.dart:
##########
@@ -48,9 +49,11 @@ class HttpExampleLoader extends ExampleLoader {
return Example(
name: descriptor.uri.path.split('/').lastOrNull ?? 'HTTP Example',
+ files: [
Review Comment:
http loader does not support multifile?
##########
playground/frontend/playground_components/lib/src/controllers/playground_controller.dart:
##########
@@ -400,22 +393,19 @@ class PlaygroundController with ChangeNotifier {
}
Future<UserSharedExampleLoadingDescriptor> saveSnippet() async {
- final controller = requireSnippetEditingController();
- final code = controller.codeController.fullText;
- final name = 'examples.userSharedName'.tr();
+ final snippetController = requireSnippetEditingController();
+ final files = snippetController.getFiles();
final snippetId = await exampleCache.saveSnippet(
- files: [
- SharedFile(code: code, isMain: true, name: name),
- ],
- sdk: controller.sdk,
- pipelineOptions: controller.pipelineOptions,
+ files: files,
Review Comment:
Sort.
##########
playground/frontend/playground_components/lib/src/controllers/snippet_editing_controller.dart:
##########
@@ -156,19 +85,32 @@ class SnippetEditingController extends ChangeNotifier {
bool get isChanged => _isChanged;
void _updateIsChanged() {
- _isChanged = _isCodeChanged() || _arePipelineOptionsChanged();
+ _isChanged = _calculateIsChanged();
}
- bool _isCodeChanged() {
- return _selectedExample?.source != codeController.fullText;
+ bool _calculateIsChanged() {
+ for (final controller in fileControllers) {
+ if (controller.isChanged) {
+ return true;
+ }
+ }
+
+ if (_arePipelineOptionsChanged()) {
+ return true;
+ }
+
+ return false;
}
Review Comment:
```suggestion
bool _calculateIsChanged() {
return _isAnyFileControllerChanged() || _arePipelineOptionsChanged();
}
```
##########
playground/frontend/playground_components/lib/src/controllers/snippet_editing_controller.dart:
##########
@@ -186,39 +128,84 @@ class SnippetEditingController extends ChangeNotifier {
return ContentExampleLoadingDescriptor(
complexity: example.complexity,
- content: codeController.fullText,
+ files: getFiles(),
name: example.name,
sdk: sdk,
);
}
- void setSource(String source) {
- codeController.readOnlySectionNames = const {};
- codeController.visibleSectionNames = const {};
+ void _replaceFileControllers(
Review Comment:
Pretty big method. Could be splitted on `_dismantleOldControllers` and `_buildNewControllers`
##########
playground/frontend/playground_components/lib/src/controllers/snippet_editing_controller.dart:
##########
@@ -16,54 +16,33 @@
* limitations under the License.
*/
-import 'dart:math';
-
+import 'package:collection/collection.dart';
import 'package:flutter/widgets.dart';
-import 'package:flutter_code_editor/flutter_code_editor.dart';
-import 'package:get_it/get_it.dart';
import '../models/example.dart';
import '../models/example_loading_descriptors/content_example_loading_descriptor.dart';
import '../models/example_loading_descriptors/empty_example_loading_descriptor.dart';
import '../models/example_loading_descriptors/example_loading_descriptor.dart';
import '../models/example_view_options.dart';
import '../models/sdk.dart';
-import '../services/symbols/symbols_notifier.dart';
+import '../models/snippet_file.dart';
+import 'snippet_file_editing_controller.dart';
/// The main state object for a single [sdk].
class SnippetEditingController extends ChangeNotifier {
+ final List<SnippetFileEditingController> fileControllers = [];
final Sdk sdk;
- final CodeController codeController;
- final _symbolsNotifier = GetIt.instance.get<SymbolsNotifier>();
+
Example? _selectedExample;
Review Comment:
Sort?
##########
playground/frontend/playground_components/lib/src/controllers/snippet_editing_controller.dart:
##########
@@ -186,39 +128,84 @@ class SnippetEditingController extends ChangeNotifier {
return ContentExampleLoadingDescriptor(
complexity: example.complexity,
- content: codeController.fullText,
+ files: getFiles(),
name: example.name,
sdk: sdk,
);
}
- void setSource(String source) {
- codeController.readOnlySectionNames = const {};
- codeController.visibleSectionNames = const {};
+ void _replaceFileControllers(
+ Iterable<SnippetFile> files,
+ ExampleViewOptions viewOptions,
+ ) {
+ for (final oldController in fileControllers) {
+ oldController.removeListener(_onFileControllerChanged);
+ }
+ final newControllers = <SnippetFileEditingController>[];
+
+ for (final file in files) {
+ final controller = SnippetFileEditingController(
+ contextLine1Based: file.isMain ? _selectedExample?.contextLine : null,
+ savedFile: file,
+ sdk: sdk,
+ viewOptions: viewOptions,
+ );
+
+ newControllers.add(controller);
+ controller.addListener(_onFileControllerChanged);
+ }
- codeController.fullText = source;
- codeController.historyController.deleteHistory();
- }
+ for (final oldController in fileControllers) {
+ oldController.dispose();
+ }
- void _onSymbolsNotifierChanged() {
- final mode = sdk.highlightMode;
- if (mode == null) {
- return;
+ fileControllers.clear();
+ fileControllers.addAll(newControllers);
+
+ _fileControllersByName.clear();
+ for (final controller in newControllers) {
+ _fileControllersByName[controller.savedFile.name] = controller;
}
- final dictionary = _symbolsNotifier.getDictionary(mode);
- if (dictionary == null) {
- return;
+ _activeFileController =
+ fileControllers.firstWhereOrNull((c) => c.savedFile.isMain);
+ }
+
+ void _onFileControllerChanged() {
+ if (!_isChanged) {
+ if (_isAnyFileControllerChanged()) {
+ _isChanged = true;
+ notifyListeners();
+ }
+ } else {
+ _updateIsChanged();
+ if (!_isChanged) {
+ notifyListeners();
+ }
}
+ }
- codeController.autocompleter.setCustomWords(dictionary.symbols);
+ bool _isAnyFileControllerChanged() {
+ return fileControllers.any((c) => c.isChanged);
}
- @override
- void dispose() {
- _symbolsNotifier.removeListener(
- _onSymbolsNotifierChanged,
- );
- super.dispose();
+ SnippetFileEditingController? get activeFileController =>
+ _activeFileController;
+
+ SnippetFileEditingController? getFileControllerByName(String name) {
+ return _fileControllersByName[name];
+ }
+
+ void activateFileControllerByName(String name) {
+ final newController = getFileControllerByName(name);
Review Comment:
Are there situation possible if newController is null here?
##########
playground/frontend/playground_components/lib/src/repositories/example_client/grpc_example_client.dart:
##########
@@ -364,7 +369,7 @@ class GrpcExampleClient implements ExampleClient {
grpc.SnippetFile()
..name = item.name
..isMain = true
- ..content = item.code,
+ ..content = item.content,
Review Comment:
```
grpc.SnippetFile(
content: item.content,
isMain: true,
name: item.name,
),
```
##########
playground/frontend/playground_components/lib/src/controllers/snippet_editing_controller.dart:
##########
@@ -186,39 +128,84 @@ class SnippetEditingController extends ChangeNotifier {
return ContentExampleLoadingDescriptor(
complexity: example.complexity,
- content: codeController.fullText,
+ files: getFiles(),
name: example.name,
sdk: sdk,
);
}
- void setSource(String source) {
- codeController.readOnlySectionNames = const {};
- codeController.visibleSectionNames = const {};
+ void _replaceFileControllers(
+ Iterable<SnippetFile> files,
+ ExampleViewOptions viewOptions,
+ ) {
+ for (final oldController in fileControllers) {
+ oldController.removeListener(_onFileControllerChanged);
+ }
+ final newControllers = <SnippetFileEditingController>[];
+
+ for (final file in files) {
+ final controller = SnippetFileEditingController(
+ contextLine1Based: file.isMain ? _selectedExample?.contextLine : null,
+ savedFile: file,
+ sdk: sdk,
+ viewOptions: viewOptions,
+ );
+
+ newControllers.add(controller);
+ controller.addListener(_onFileControllerChanged);
+ }
- codeController.fullText = source;
- codeController.historyController.deleteHistory();
- }
+ for (final oldController in fileControllers) {
+ oldController.dispose();
+ }
- void _onSymbolsNotifierChanged() {
- final mode = sdk.highlightMode;
- if (mode == null) {
- return;
+ fileControllers.clear();
+ fileControllers.addAll(newControllers);
+
+ _fileControllersByName.clear();
+ for (final controller in newControllers) {
+ _fileControllersByName[controller.savedFile.name] = controller;
}
- final dictionary = _symbolsNotifier.getDictionary(mode);
- if (dictionary == null) {
- return;
+ _activeFileController =
+ fileControllers.firstWhereOrNull((c) => c.savedFile.isMain);
+ }
+
+ void _onFileControllerChanged() {
+ if (!_isChanged) {
+ if (_isAnyFileControllerChanged()) {
+ _isChanged = true;
+ notifyListeners();
+ }
+ } else {
+ _updateIsChanged();
+ if (!_isChanged) {
+ notifyListeners();
+ }
}
+ }
- codeController.autocompleter.setCustomWords(dictionary.symbols);
+ bool _isAnyFileControllerChanged() {
Review Comment:
`_areFileControllersChanged`
##########
playground/frontend/lib/modules/examples/components/example_list/example_item_actions.dart:
##########
@@ -65,3 +59,21 @@ class ExampleItemActions extends StatelessWidget {
Provider.of<PopoverState>(context, listen: false).setOpen(isOpen);
}
}
+
+/// A wrapper of a standard size for icons in the example list.
+class _Icon extends StatelessWidget {
+ const _Icon(this.child);
Review Comment:
rename to `icon` and make parameter named?
##########
playground/frontend/playground_components/lib/src/controllers/snippet_editing_controller.dart:
##########
@@ -156,19 +85,32 @@ class SnippetEditingController extends ChangeNotifier {
bool get isChanged => _isChanged;
void _updateIsChanged() {
- _isChanged = _isCodeChanged() || _arePipelineOptionsChanged();
+ _isChanged = _calculateIsChanged();
}
- bool _isCodeChanged() {
- return _selectedExample?.source != codeController.fullText;
+ bool _calculateIsChanged() {
+ for (final controller in fileControllers) {
+ if (controller.isChanged) {
+ return true;
+ }
+ }
+
+ if (_arePipelineOptionsChanged()) {
+ return true;
+ }
+
+ return false;
}
bool _arePipelineOptionsChanged() {
return _pipelineOptions != (_selectedExample?.pipelineOptions ?? '');
}
void reset() {
- codeController.text = _selectedExample?.source ?? '';
+ for (final controller in fileControllers) {
Review Comment:
tried to change this on `forEach`, but we have a linter rule, forbidding this. To me, `forEach` method less verbose here. What do you think?
##########
playground/frontend/lib/modules/examples/components/example_list/example_item_actions.dart:
##########
@@ -17,12 +17,15 @@
*/
import 'package:flutter/material.dart';
-import 'package:playground/modules/examples/components/description_popover/description_popover_button.dart';
-import 'package:playground/modules/examples/components/multifile_popover/multifile_popover_button.dart';
-import 'package:playground/modules/examples/models/popover_state.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
+import '../../models/popover_state.dart';
+import '../description_popover/description_popover_button.dart';
+import '../multi_file_icon.dart';
+
+const double _iconSize = 30;
Review Comment:
1. Move to `_Icon`? Looks like it doesn't required in the whole file.
2. May be to use `kIconSizeLg` instead of this variable? Especially `DescriptionPopoverButton` uses it
##########
playground/frontend/playground_components/lib/src/controllers/playground_controller.dart:
##########
@@ -400,22 +393,19 @@ class PlaygroundController with ChangeNotifier {
}
Future<UserSharedExampleLoadingDescriptor> saveSnippet() async {
- final controller = requireSnippetEditingController();
- final code = controller.codeController.fullText;
- final name = 'examples.userSharedName'.tr();
+ final snippetController = requireSnippetEditingController();
+ final files = snippetController.getFiles();
final snippetId = await exampleCache.saveSnippet(
- files: [
- SharedFile(code: code, isMain: true, name: name),
- ],
- sdk: controller.sdk,
- pipelineOptions: controller.pipelineOptions,
+ files: files,
+ sdk: snippetController.sdk,
+ pipelineOptions: snippetController.pipelineOptions,
);
final sharedExample = Example(
- source: code,
- name: name,
- sdk: controller.sdk,
+ files: files,
Review Comment:
Sort
##########
playground/frontend/lib/modules/examples/components/example_list/example_item_actions.dart:
##########
@@ -65,3 +59,21 @@ class ExampleItemActions extends StatelessWidget {
Provider.of<PopoverState>(context, listen: false).setOpen(isOpen);
}
}
+
+/// A wrapper of a standard size for icons in the example list.
+class _Icon extends StatelessWidget {
Review Comment:
`_IconFrame` or `_IconWrapper`?
##########
playground/frontend/playground_components/lib/src/controllers/snippet_file_editing_controller.dart:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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:math';
+
+import 'package:flutter/widgets.dart';
+import 'package:flutter_code_editor/flutter_code_editor.dart';
+import 'package:get_it/get_it.dart';
+
+import '../models/example_view_options.dart';
+import '../models/sdk.dart';
+import '../models/snippet_file.dart';
+import '../services/symbols/symbols_notifier.dart';
+
+/// The main state object for a file in a snippet.
+class SnippetFileEditingController extends ChangeNotifier {
+ final CodeController codeController;
+ final SnippetFile savedFile;
+ final Sdk sdk;
+
+ bool _isChanged = false;
+
+ final _symbolsNotifier = GetIt.instance.get<SymbolsNotifier>();
+
+ SnippetFileEditingController({
+ required this.savedFile,
+ required this.sdk,
+ required ExampleViewOptions viewOptions,
+ int? contextLine1Based,
+ }) : codeController = CodeController(
+ language: sdk.highlightMode,
+ namedSectionParser: const BracketsStartEndNamedSectionParser(),
+ text: savedFile.content,
+ ) {
+ _applyViewOptions(viewOptions);
+ if (contextLine1Based != null) {
Review Comment:
why this logic should be here? Would be awesome if comment will be added 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