alexeyinkin commented on code in PR #24456:
URL: https://github.com/apache/beam/pull/24456#discussion_r1037014802
##########
playground/frontend/playground_components/lib/src/controllers/playground_controller.dart:
##########
@@ -138,7 +139,8 @@ class PlaygroundController with ChangeNotifier {
[Sdk.java, Sdk.python].contains(sdk);
void setExample(
- Example example, {
+ Example example,
+ ExampleLoadingDescriptor descriptor, {
Review Comment:
This calls for a named argument to me.
##########
playground/frontend/playground_components/lib/src/cache/example_cache.dart:
##########
@@ -104,15 +105,17 @@ class ExampleCache extends ChangeNotifier {
);
}
- Future<Example> loadSharedExample(String id) async {
+ Future<Example> loadSharedExample(
+ UserSharedExampleLoadingDescriptor descriptor,
+ ) async {
final result = await _exampleRepository.getSnippet(
- GetSnippetRequest(id: id),
+ GetSnippetRequest(id: descriptor.snippetId),
);
return Example(
sdk: result.sdk,
name: result.files.first.name,
- path: id,
+ path: descriptor.snippetId,
Review Comment:
Unnecessary?
##########
playground/frontend/playground_components/lib/src/models/example_loading_descriptors/content_example_loading_descriptor.dart:
##########
@@ -76,7 +76,7 @@ class ContentExampleLoadingDescriptor extends
ExampleLoadingDescriptor {
}
@override
- List<Object> get props => [content, sdk.id];
+ List<Object> get props => [content, sdk.id, complexity?.name ?? '', name ??
''];
Review Comment:
Turn to `List<Object?>`
##########
playground/frontend/playground_components/lib/src/models/example_loading_descriptors/example_loading_descriptor.dart:
##########
@@ -28,4 +29,6 @@ abstract class ExampleLoadingDescriptor with EquatableMixin {
final ExampleViewOptions viewOptions;
Map<String, dynamic> toJson() => throw UnimplementedError();
+
+ bool get canBePassedInUrl => this is! ContentExampleLoadingDescriptor;
Review Comment:
This is an unnecessary knowledge for the base class and an extra reason for
it to change.
Either make the getter abstract or default to something.
##########
playground/frontend/test/pages/playground/states/example_selector_state_test.mocks.dart:
##########
@@ -1,7 +1,8 @@
-// Mocks generated by Mockito 5.2.0 from annotations
+// Mocks generated by Mockito 5.3.2 from annotations
Review Comment:
If mockito is upgraded, this should also be in pubspec.lock, otherwise
someone will overwrite this with the old version and can face potential merge
conflicts.
##########
playground/frontend/playground_components/lib/src/controllers/snippet_editing_controller.dart:
##########
@@ -45,16 +47,26 @@ class SnippetEditingController extends ChangeNotifier {
_onSymbolsNotifierChanged();
}
- set selectedExample(Example? value) {
- _selectedExample = value;
+ void configure({
+ required Example? example,
+ ExampleLoadingDescriptor? descriptor,
+ String? pipelineOptions,
+ }) {
+ _descriptor = descriptor ?? _descriptor;
+ _selectedExample = example ?? _selectedExample;
+
+ if (example != null && descriptor == null) {
+ _descriptor = null;
+ }
Review Comment:
This adds complexity with conditions.
I suggest adding `{required ExampleLoadingDescriptor descriptor}` and always
overwrite both.
Mixing `pipelineOptions` here is extra confusing because `Example` itself
contains `pipelineOptions`. So if we set both, what is the reason to prefer one
`pipelineOptions` over another?
##########
playground/frontend/playground_components/test/src/common/descriptors.dart:
##########
@@ -0,0 +1,29 @@
+import 'package:playground_components/playground_components.dart';
Review Comment:
rat
##########
playground/frontend/lib/pages/embedded_playground/components/embedded_actions.dart:
##########
@@ -58,16 +59,29 @@ class EmbeddedActions extends StatelessWidget {
void _openStandalonePlayground(PlaygroundController controller) {
// The empty list forces the parsing of EmptyExampleLoadingDescriptor
// and prevents the glimpse of the default catalog example.
+
+ final descriptor = controller.getLoadingDescriptor();
+ final descriptors = descriptor.descriptors;
+ final urlDescriptors = descriptors.where((d) => d.canBePassedInUrl);
+ final jsDescriptors = descriptors.where((d) => !d.canBePassedInUrl);
+
+ final json = jsonEncode(
+ urlDescriptors.map((d) => d.toJson()).toList(growable: false),
+ );
final window = html.window.open(
- '/?$kExamplesParam=[]&$kSdkParam=${controller.sdk?.id}',
+ '/?$kExamplesParam=$json&$kSdkParam=${controller.sdk?.id}',
'',
);
- javaScriptPostMessageRepeated(
- window,
- SetContentMessage(
- descriptor: controller.getLoadingDescriptor(),
- ),
- );
+ if (jsDescriptors.isNotEmpty) {
+ javaScriptPostMessageRepeated(
+ window,
+ SetContentMessage(
+ descriptor: descriptor.copyWith(
Review Comment:
I don't think we should copy it here.
1. `descriptor` may contain `initialSdk`. At best it is redundant since we
pass it in the URL. At worst it can switch the SDK if the user changed it after
loading from URL and before the message is received.
2. `descriptor` may contain `lazyLoadDescriptors`. They have no effect in
this message currently but may get in the way if we ever stop ignoring them.
--
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]
