[ermine] Close view if underlying component exits
This change responds to the session's ViewController closing its
bound handle to dismiss the associated story and its view.
Test: Added a unittest for close on ViewController. All tests pass.
Bug: 47792
Change-Id: Ic0d4ee1369b063850284c13dcb49042cc470d215
Reviewed-on: https://fuchsia-review.googlesource.com/c/experiences/+/427246
Reviewed-by: Darren Chan <chandarren@google.com>
Reviewed-by: Charles Whitten <cwhitten@google.com>
Testability-Review: Sanjay Chouksey <sanjayc@google.com>
Commit-Queue: Sanjay Chouksey <sanjayc@google.com>
diff --git a/session_shells/ermine/session/src/element_repository/event_handler.rs b/session_shells/ermine/session/src/element_repository/event_handler.rs
index 46de872..7c1b021 100644
--- a/session_shells/ermine/session/src/element_repository/event_handler.rs
+++ b/session_shells/ermine/session/src/element_repository/event_handler.rs
@@ -13,7 +13,6 @@
},
fidl_fuchsia_ui_app::ViewProviderMarker,
fuchsia_async as fasync, fuchsia_scenic as scenic,
- fuchsia_syslog::fx_log_info,
fuchsia_zircon as zx,
fuchsia_zircon::AsHandleRef,
futures::{select, stream::StreamExt, FutureExt, TryStreamExt},
@@ -125,7 +124,14 @@
// that there was a crash. The current contract is that if the
// view controller binding closes without a dismiss then the
// presenter should treat this as a crash and respond accordingly.
- fx_log_info!("An element closed unexpectedly: {:?}", element);
+ if let Some(proxy) = view_controller_proxy {
+ // We want to allow the presenter the ability to dismiss
+ // the view so we tell it to dismiss and then wait for
+ // the view controller stream to close.
+ let _ = proxy.dismiss();
+ //TODO(47925) introdue a timeout here
+ wait_for_view_controller_close(proxy).await;
+ }
},
_ = wait_for_optional_view_controller_close(view_controller_proxy.clone()).fuse() => {
// signals that the presenter would like to close the element.
diff --git a/session_shells/ermine/shell/lib/src/models/app_model.dart b/session_shells/ermine/shell/lib/src/models/app_model.dart
index cac9ae5..a447a40 100644
--- a/session_shells/ermine/shell/lib/src/models/app_model.dart
+++ b/session_shells/ermine/shell/lib/src/models/app_model.dart
@@ -132,7 +132,10 @@
@visibleForTesting
void advertise() {
// Expose the presenter service to the environment.
- _presenterService = PresenterService(clustersModel.presentStory);
+ _presenterService = PresenterService(
+ onPresent: clustersModel.presentStory,
+ onDismiss: clustersModel.dismissStory,
+ );
_startupContext.outgoing
.addPublicService(_presenterService.bind, PresenterService.serviceName);
}
diff --git a/session_shells/ermine/shell/lib/src/models/cluster_model.dart b/session_shells/ermine/shell/lib/src/models/cluster_model.dart
index d21bf34..424366d 100644
--- a/session_shells/ermine/shell/lib/src/models/cluster_model.dart
+++ b/session_shells/ermine/shell/lib/src/models/cluster_model.dart
@@ -7,6 +7,7 @@
import 'package:flutter/rendering.dart';
import 'package:fidl_fuchsia_ui_views/fidl_async.dart';
+import 'package:fuchsia_scenic_flutter/child_view_connection.dart';
import 'package:tiler/tiler.dart' show TilerModel, TileModel;
import '../utils/presenter.dart';
@@ -79,7 +80,7 @@
@override
void presentStory(
- ViewHolderToken token,
+ ChildViewConnection connection,
ViewRef viewRef,
ViewControllerImpl viewController,
String id,
@@ -97,7 +98,16 @@
_addErmineStory(story);
}
- story.presentView(token, viewRef, viewController);
+ story.presentView(connection, viewRef, viewController);
+ }
+
+ @override
+ void dismissStory(ViewControllerImpl viewController) {
+ // Find the story with the supplied viewController.
+ final story =
+ stories.where((story) => story.viewController == viewController).first;
+ assert(story != null);
+ story.delete();
}
/// Creates and adds a [Story] to the current cluster given it's [StoryInfo],
diff --git a/session_shells/ermine/shell/lib/src/models/ermine_shell.dart b/session_shells/ermine/shell/lib/src/models/ermine_shell.dart
index aad7c6f..63ee39d 100644
--- a/session_shells/ermine/shell/lib/src/models/ermine_shell.dart
+++ b/session_shells/ermine/shell/lib/src/models/ermine_shell.dart
@@ -3,6 +3,7 @@
// found in the LICENSE file.
import 'package:fidl_fuchsia_ui_views/fidl_async.dart';
+import 'package:fuchsia_scenic_flutter/child_view_connection.dart';
import '../utils/presenter.dart';
import '../utils/suggestion.dart';
@@ -29,8 +30,8 @@
/// it has been launched. The story may have been proposed by ermine or
/// an external source.
void presentStory(
- /// The view holder token used to connect the view to the process
- ViewHolderToken token,
+ /// The [ChildViewConnection] used to connect the view to the process.
+ ChildViewConnection connection,
/// The [ViewRef] of the view used as a handle for focusing.
ViewRef viewRef,
@@ -43,4 +44,7 @@
/// story was launched from ermine.
String id,
);
+
+ /// Called when a story is dismissed by the session instead of the user.
+ void dismissStory(ViewControllerImpl viewController);
}
diff --git a/session_shells/ermine/shell/lib/src/models/ermine_story.dart b/session_shells/ermine/shell/lib/src/models/ermine_story.dart
index 3a4a634..3830c78 100644
--- a/session_shells/ermine/shell/lib/src/models/ermine_story.dart
+++ b/session_shells/ermine/shell/lib/src/models/ermine_story.dart
@@ -25,7 +25,6 @@
// An optional view controller which allows the story to communicate with the
// process.
- @visibleForTesting
ViewControllerImpl viewController;
ViewRef viewRef;
@@ -134,12 +133,8 @@
}
void presentView(
- ViewHolderToken viewHolderToken, ViewRef viewRef, ViewControllerImpl vc) {
- childViewConnectionNotifier.value = ChildViewConnection(
- viewHolderToken,
- onAvailable: (_) {},
- onUnavailable: (_) {},
- );
+ ChildViewConnection connection, ViewRef viewRef, ViewControllerImpl vc) {
+ childViewConnectionNotifier.value = connection;
this.viewRef = viewRef;
viewController = vc;
viewController?.didPresent();
diff --git a/session_shells/ermine/shell/lib/src/utils/presenter.dart b/session_shells/ermine/shell/lib/src/utils/presenter.dart
index c948582..f388ad0 100644
--- a/session_shells/ermine/shell/lib/src/utils/presenter.dart
+++ b/session_shells/ermine/shell/lib/src/utils/presenter.dart
@@ -7,7 +7,7 @@
import 'package:fidl_fuchsia_session/fidl_async.dart' as fidl;
import 'package:fidl_fuchsia_ui_views/fidl_async.dart';
import 'package:fidl/fidl.dart';
-import 'package:fuchsia_logger/logger.dart';
+import 'package:fuchsia_scenic_flutter/child_view_connection.dart';
import 'suggestion.dart';
@@ -17,16 +17,24 @@
/// requesting source did not provide a channel. The methods will still be safe
/// to call even if it is not bound.
typedef PresentViewCallback = void Function(
- ViewHolderToken, ViewRef, ViewControllerImpl, String);
+ ChildViewConnection, ViewRef, ViewControllerImpl, String);
+
+/// A callback which is invoked when the element is dismissed by the session.
+///
+/// This callback is the result of the session getting notified of a component
+/// exiting by itself or a crash. It is NOT due to the user closing it from
+/// the shell.
+typedef DismissViewCallback = void Function(ViewControllerImpl);
/// A service which implements the fuchsia.session.GraphicalPresenter protocol.
class PresenterService extends fidl.GraphicalPresenter {
static const String serviceName = fidl.GraphicalPresenter.$serviceName;
final List<fidl.GraphicalPresenterBinding> _bindings = [];
- final PresentViewCallback _onPresent;
+ final PresentViewCallback onPresent;
+ final DismissViewCallback onDismiss;
- PresenterService(this._onPresent);
+ PresenterService({this.onPresent, this.onDismiss});
/// Binds the request to this model.
void bind(InterfaceRequest<fidl.GraphicalPresenter> request) {
@@ -42,7 +50,8 @@
@override
Future<void> presentView(fidl.ViewSpec viewSpec,
InterfaceRequest<fidl.ViewController> viewControllerRequest) async {
- final viewController = ViewControllerImpl()..bind(viewControllerRequest);
+ final viewController = ViewControllerImpl(onDismiss)
+ ..bind(viewControllerRequest);
// Check to see if we have an id that we included in the annotation.
final idAnnotation = viewSpec.annotations?.customAnnotations
@@ -50,8 +59,13 @@
final viewHolderToken = viewSpec.viewHolderToken;
if (viewHolderToken != null) {
- _onPresent(
+ final connection = ChildViewConnection(
viewHolderToken,
+ onAvailable: (_) {},
+ onUnavailable: (_) {},
+ );
+ onPresent(
+ connection,
viewSpec.viewRef,
viewController,
idAnnotation?.value?.text ?? '',
@@ -66,14 +80,13 @@
final _binding = fidl.ViewControllerBinding();
final StreamController<void> _onPresentedStreamController =
StreamController.broadcast();
+ DismissViewCallback onDismiss;
+
+ ViewControllerImpl(this.onDismiss);
void bind(InterfaceRequest<fidl.ViewController> interfaceRequest) {
if (interfaceRequest != null && interfaceRequest.channel != null) {
_binding.bind(this, interfaceRequest);
- _binding.whenClosed.then((_) {
- //TODO(47793) Need to watch our binding for unexpected closures and notify the user
- log.info('binding closed unexpectedly');
- });
}
}
@@ -91,9 +104,7 @@
@override
Future<void> dismiss() async {
- // TODO(47792): A call to dismiss indicates that the view should go away we need to
- // allow the user of this class to asynchronously dismiss the view before closing
- // the channel.
+ onDismiss?.call(this);
close();
}
diff --git a/session_shells/ermine/shell/meta/ermine.cmx b/session_shells/ermine/shell/meta/ermine.cmx
index de9f7c8..c1224c6 100644
--- a/session_shells/ermine/shell/meta/ermine.cmx
+++ b/session_shells/ermine/shell/meta/ermine.cmx
@@ -26,11 +26,12 @@
"fuchsia.settings.Intl",
"fuchsia.sys.Environment",
"fuchsia.sys.Launcher",
- "fuchsia.ui.focus.FocusChainListenerRegistry",
"fuchsia.ui.brightness.Control",
+ "fuchsia.ui.focus.FocusChainListenerRegistry",
"fuchsia.ui.input.ImeService",
- "fuchsia.ui.policy.Presenter",
+ "fuchsia.ui.input.InputDeviceRegistry",
"fuchsia.ui.policy.Presentation",
+ "fuchsia.ui.policy.Presenter",
"fuchsia.ui.scenic.Scenic",
"fuchsia.ui.shortcut.Registry"
]
diff --git a/session_shells/ermine/shell/test/app_model_test.dart b/session_shells/ermine/shell/test/app_model_test.dart
index 3a1bddb..36342e3 100644
--- a/session_shells/ermine/shell/test/app_model_test.dart
+++ b/session_shells/ermine/shell/test/app_model_test.dart
@@ -216,6 +216,7 @@
test('Should update peekNotifier when fullscreen', () async {
final story = MockErmineStory();
when(clustersModel.fullscreenStory).thenReturn(story);
+ when(clustersModel.hasStories).thenReturn(true);
expect(appModel.isFullscreen, true);
diff --git a/session_shells/ermine/shell/test/clusters_model_test.dart b/session_shells/ermine/shell/test/clusters_model_test.dart
index eee312b..d1261f6 100644
--- a/session_shells/ermine/shell/test/clusters_model_test.dart
+++ b/session_shells/ermine/shell/test/clusters_model_test.dart
@@ -3,6 +3,7 @@
// found in the LICENSE file.
import 'package:fidl_fuchsia_ui_views/fidl_async.dart';
+import 'package:fuchsia_scenic_flutter/child_view_connection.dart';
import 'package:mockito/mockito.dart';
import 'package:test/test.dart';
import 'package:zircon/zircon.dart';
@@ -32,9 +33,9 @@
when(eventPair.isValid).thenReturn(false);
clustersModel.presentStory(
- ViewHolderToken(value: eventPair),
+ MockChildViewConnection(),
ViewRef(reference: eventPair),
- ViewControllerImpl(),
+ ViewControllerImpl((_) {}),
'id',
);
@@ -45,6 +46,25 @@
expect(clustersModel.getStory(id), isNotNull);
});
+ test('Dismissed stories should be deleted', () {
+ final eventPair = MockEventPair();
+ when(eventPair.isValid).thenReturn(false);
+
+ final viewController = ViewControllerImpl((_) {});
+ clustersModel.presentStory(
+ MockChildViewConnection(),
+ ViewRef(reference: eventPair),
+ viewController,
+ 'id',
+ );
+
+ final story = clustersModel.focusedStory;
+ expect(story.viewController, viewController);
+
+ clustersModel.dismissStory(viewController);
+ expect(clustersModel.hasStories, false);
+ });
+
test('Story maximize/minimize should toggle fullscreen', () {
// Create a story.
final suggestion = Suggestion(id: 'id', url: 'url', title: 'title');
@@ -112,4 +132,6 @@
});
}
+class MockChildViewConnection extends Mock implements ChildViewConnection {}
+
class MockEventPair extends Mock implements EventPair {}
