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/04 13:14:54 UTC
[GitHub] [beam] alexeyinkin commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
alexeyinkin commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1013884620
##########
learning/tour-of-beam/frontend/lib/models/content_tree.dart:
##########
@@ -18,15 +18,25 @@
import '../repositories/models/get_content_tree_response.dart';
import 'module.dart';
+import 'node.dart';
+import 'parent_node.dart';
-class ContentTreeModel {
+class ContentTreeModel extends ParentNodeModel {
final String sdkId;
Review Comment:
`String get sdkId => id;` ?
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -17,33 +17,82 @@
*/
import 'package:flutter/widgets.dart';
+import 'package:get_it/get_it.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../cache/content_tree.dart';
+import '../../../models/group.dart';
import '../../../models/node.dart';
+import '../../../models/unit.dart';
class ContentTreeController extends ChangeNotifier {
String _sdkId;
List<String> _treeIds;
NodeModel? _currentNode;
+ final _contentTreeCache = GetIt.instance.get<ContentTreeCache>();
+ final expandedIds = <String>{};
ContentTreeController({
required String initialSdkId,
List<String> initialTreeIds = const [],
}) : _sdkId = initialSdkId,
- _treeIds = initialTreeIds;
+ _treeIds = initialTreeIds {
+ expandedIds.addAll(initialTreeIds);
+ _contentTreeCache.addListener(_onContentTreeCacheChange);
+ _onContentTreeCacheChange();
+ }
Sdk get sdk => Sdk.parseOrCreate(_sdkId);
String get sdkId => _sdkId;
List<String> get treeIds => _treeIds;
NodeModel? get currentNode => _currentNode;
- void onNodeTap(NodeModel node) {
+ void openNode(NodeModel node) {
+ if (!expandedIds.contains(node.id)) {
+ expandedIds.add(node.id);
+ }
+
if (node == _currentNode) {
return;
}
- _currentNode = node;
- // TODO(alexeyinkin): Set _treeIds from node.
+ if (node is GroupModel) {
+ openNode(node.nodes.first);
+ } else if (node is UnitModel) {
+ _currentNode = node;
+ }
+
+ if (_currentNode != null) {
+ _treeIds = _getNodeAncestors(_currentNode!, [_currentNode!.id]);
+ }
+ notifyListeners();
+ }
+
+ List<String> _getNodeAncestors(NodeModel node, List<String> ancestors) {
+ if (node.parent != null) {
+ ancestors.add(node.parent!.id);
+ return _getNodeAncestors(node.parent!, ancestors);
Review Comment:
`[...ancestorIds, node.parent!.id]`
We should avoid adding to passed lists. The reasons are:
1. The list can be used in the calling code.
2. The list may be unmodifiable.
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -17,33 +17,82 @@
*/
import 'package:flutter/widgets.dart';
+import 'package:get_it/get_it.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../cache/content_tree.dart';
+import '../../../models/group.dart';
import '../../../models/node.dart';
+import '../../../models/unit.dart';
class ContentTreeController extends ChangeNotifier {
String _sdkId;
List<String> _treeIds;
NodeModel? _currentNode;
+ final _contentTreeCache = GetIt.instance.get<ContentTreeCache>();
+ final expandedIds = <String>{};
ContentTreeController({
required String initialSdkId,
List<String> initialTreeIds = const [],
}) : _sdkId = initialSdkId,
- _treeIds = initialTreeIds;
+ _treeIds = initialTreeIds {
+ expandedIds.addAll(initialTreeIds);
+ _contentTreeCache.addListener(_onContentTreeCacheChange);
+ _onContentTreeCacheChange();
+ }
Sdk get sdk => Sdk.parseOrCreate(_sdkId);
String get sdkId => _sdkId;
List<String> get treeIds => _treeIds;
NodeModel? get currentNode => _currentNode;
- void onNodeTap(NodeModel node) {
+ void openNode(NodeModel node) {
+ if (!expandedIds.contains(node.id)) {
+ expandedIds.add(node.id);
+ }
+
if (node == _currentNode) {
return;
}
- _currentNode = node;
- // TODO(alexeyinkin): Set _treeIds from node.
+ if (node is GroupModel) {
+ openNode(node.nodes.first);
+ } else if (node is UnitModel) {
+ _currentNode = node;
+ }
+
+ if (_currentNode != null) {
+ _treeIds = _getNodeAncestors(_currentNode!, [_currentNode!.id]);
+ }
+ notifyListeners();
+ }
+
+ List<String> _getNodeAncestors(NodeModel node, List<String> ancestors) {
Review Comment:
`ancestorIds`
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/unit.dart:
##########
@@ -33,16 +33,48 @@ class UnitWidget extends StatelessWidget {
required this.contentTreeController,
});
+ @override
+ State<UnitWidget> createState() => _UnitWidgetState();
+}
+
+class _UnitWidgetState extends State<UnitWidget> {
+ @override
+ void initState() {
+ super.initState();
+ widget.contentTreeController.addListener(_rebuild);
+ }
+
+ @override
+ void dispose() {
+ widget.contentTreeController.removeListener(_rebuild);
+ super.dispose();
+ }
+
+ void _rebuild() {
+ setState(() {});
Review Comment:
Use `AnimatedBuilder` instead.
##########
playground/frontend/playground_components/lib/src/constants/colors.dart:
##########
@@ -36,45 +36,51 @@ class BeamColors {
class BeamGraphColors {
static const node = BeamColors.grey3;
- static const border = Color(0xFF45454E);
+ static const border = Color(0xff45454E);
Review Comment:
Is this deliberate? Why using lowercase for alpha and uppercase for others?
##########
learning/tour-of-beam/frontend/pubspec.lock:
##########
@@ -278,7 +278,7 @@ packages:
name: flutter_code_editor
url: "https://pub.dartlang.org"
source: hosted
- version: "0.1.1"
+ version: "0.1.3"
Review Comment:
It's already `0.1.3` in `master`. This will likely cause a merge conflict.
##########
playground/frontend/playground_components/lib/src/theme/theme.dart:
##########
@@ -32,6 +32,9 @@ class BeamThemeExtension extends ThemeExtension<BeamThemeExtension> {
final Color primaryBackgroundTextColor;
final Color lightGreyBackgroundTextColor;
final Color secondaryBackgroundColor;
+ // TODO(nausharipov): simplify new color addition
+ final Color selectedProgressColor;
+ final Color unselectedProgressColor;
Review Comment:
Can these use a single color with some transparency?
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/group.dart:
##########
@@ -34,22 +34,44 @@ class GroupWidget extends StatelessWidget {
required this.contentTreeController,
});
+ @override
+ State<GroupWidget> createState() => _GroupWidgetState();
+}
+
+class _GroupWidgetState extends State<GroupWidget> {
@override
Widget build(BuildContext context) {
+ final isExpanded =
+ widget.contentTreeController.expandedIds.contains(widget.group.id);
+
return ExpansionTileWrapper(
ExpansionTile(
+ key: Key('${widget.group.id}$isExpanded'),
+ initiallyExpanded: isExpanded,
tilePadding: EdgeInsets.zero,
+ onExpansionChanged: (expand) {
+ /// Since expandedIds is also used for expansion, it needs to be
+ /// updated to prevent the tile to stay collapsed on title tap.
+ if (!expand) {
+ widget.contentTreeController.expandedIds.remove(widget.group.id);
+ setState(() {});
+ }
+ },
title: GroupTitleWidget(
- group: group,
- onTap: () => contentTreeController.onNodeTap(group),
+ group: widget.group,
+ onTap: () {
+ widget.contentTreeController.openNode(widget.group);
+ // Couldn't make it rebuild reliably with AnimatedBuilder
Review Comment:
Put such comments in a form of TODO.
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -17,33 +17,82 @@
*/
import 'package:flutter/widgets.dart';
+import 'package:get_it/get_it.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../cache/content_tree.dart';
+import '../../../models/group.dart';
import '../../../models/node.dart';
+import '../../../models/unit.dart';
class ContentTreeController extends ChangeNotifier {
String _sdkId;
List<String> _treeIds;
NodeModel? _currentNode;
+ final _contentTreeCache = GetIt.instance.get<ContentTreeCache>();
+ final expandedIds = <String>{};
ContentTreeController({
required String initialSdkId,
List<String> initialTreeIds = const [],
}) : _sdkId = initialSdkId,
- _treeIds = initialTreeIds;
+ _treeIds = initialTreeIds {
+ expandedIds.addAll(initialTreeIds);
+ _contentTreeCache.addListener(_onContentTreeCacheChange);
+ _onContentTreeCacheChange();
+ }
Sdk get sdk => Sdk.parseOrCreate(_sdkId);
String get sdkId => _sdkId;
List<String> get treeIds => _treeIds;
NodeModel? get currentNode => _currentNode;
- void onNodeTap(NodeModel node) {
+ void openNode(NodeModel node) {
+ if (!expandedIds.contains(node.id)) {
+ expandedIds.add(node.id);
+ }
+
if (node == _currentNode) {
return;
}
- _currentNode = node;
- // TODO(alexeyinkin): Set _treeIds from node.
+ if (node is GroupModel) {
+ openNode(node.nodes.first);
+ } else if (node is UnitModel) {
+ _currentNode = node;
+ }
+
+ if (_currentNode != null) {
+ _treeIds = _getNodeAncestors(_currentNode!, [_currentNode!.id]);
+ }
+ notifyListeners();
+ }
+
+ List<String> _getNodeAncestors(NodeModel node, List<String> ancestors) {
+ if (node.parent != null) {
+ ancestors.add(node.parent!.id);
+ return _getNodeAncestors(node.parent!, ancestors);
+ } else {
Review Comment:
No `else` after `return`.
--
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