Add SurfaceController #516
Conversation
|
/gemini summarize |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and positive refactoring by adding the SurfaceController. This encapsulates surface-specific state and logic, simplifying the GenUiManager and making the overall architecture cleaner and more scalable. The changes are consistently applied across the examples and tests. I've provided a few suggestions to further improve maintainability by removing some dead code, simplifying repetitive checks, fixing a test, and making listener management more explicit.
| if (message is SurfaceUpdate && message.surfaceId != surfaceId) { | ||
| throw ArgumentError( | ||
| 'Mismatched surfaceId in message: ' | ||
| 'expected $surfaceId, got ${message.surfaceId}', | ||
| ); | ||
| } | ||
| if (message is BeginRendering && message.surfaceId != surfaceId) { | ||
| throw ArgumentError( | ||
| 'Mismatched surfaceId in message: ' | ||
| 'expected $surfaceId, got ${message.surfaceId}', | ||
| ); | ||
| } | ||
| if (message is DataModelUpdate && message.surfaceId != surfaceId) { | ||
| throw ArgumentError( | ||
| 'Mismatched surfaceId in message: ' | ||
| 'expected $surfaceId, got ${message.surfaceId}', | ||
| ); | ||
| } |
There was a problem hiding this comment.
These checks for surfaceId are repetitive. You can make this code more concise and maintainable by using a switch expression to extract the surfaceId from the message and then performing a single check. This will also make it easier to add new message types in the future.
final String messageSurfaceId = switch (message) {
SurfaceUpdate(:final surfaceId) => surfaceId,
BeginRendering(:final surfaceId) => surfaceId,
DataModelUpdate(:final surfaceId) => surfaceId,
SurfaceDeletion(:final surfaceId) => surfaceId,
};
if (messageSurfaceId != surfaceId) {
throw ArgumentError(
'Mismatched surfaceId in message: '
'expected $surfaceId, got $messageSurfaceId',
);
}| // Listen for updates to this specific controller to manage history. | ||
| // We don't store a reference to the listener, so it can't be removed | ||
| // later. However, since the lifecycle of the controller is managed by | ||
| // the GenUiManager and tied to the conversation, this should not | ||
| // result in a memory leak in practice as the controller itself will be | ||
| // disposed. | ||
| controller.uiDefinitionNotifier.addListener(() { | ||
| _handleDefinitionUpdate(controller); | ||
| }); |
There was a problem hiding this comment.
The long comment explaining the listener lifecycle suggests the implementation could be clearer. While the current logic is sound, explicitly managing listeners makes the code more robust and self-documenting.
You can achieve this by storing listeners in a map and removing them when a surface is removed. This removes the need for the long explanatory comment.
For example, you could add a _surfaceListeners map to your class:
final _surfaceListeners = <String, VoidCallback>{};Then, in _handleSurfaceUpdate:
// When adding a listener for SurfaceAdded:
final listener = () => _handleDefinitionUpdate(controller);
_surfaceListeners[controller.surfaceId] = listener;
controller.uiDefinitionNotifier.addListener(listener);
// When handling SurfaceRemoved:
final listener = _surfaceListeners.remove(controller.surfaceId);
if (listener != null) {
controller.uiDefinitionNotifier.removeListener(listener);
}This makes the listener management explicit and easier to follow.
| // After dispose, trying to add a listener should throw an error. | ||
| // The test runner is catching this error before the try/catch block can, | ||
| // so this part of the test is commented out. The error message proves | ||
| // that dispose() is working as intended. | ||
| // try { | ||
| // notifier.addListener(() {}); | ||
| // fail('Should have thrown an error'); | ||
| // } catch (e) { | ||
| // expect(e, isA<Error>()); | ||
| // expect( | ||
| // e.toString(), | ||
| // contains('A ValueNotifier<UiDefinition?> was used after being disposed.'), | ||
| // ); | ||
| // } | ||
| }); |
There was a problem hiding this comment.
This test case for dispose is commented out. It can be fixed and enabled by using expect(..., throwsA(...)) to verify that using the notifier after disposal throws an error. This is a more standard way to test for exceptions/errors in Flutter tests than a try-catch block.
| // After dispose, trying to add a listener should throw an error. | |
| // The test runner is catching this error before the try/catch block can, | |
| // so this part of the test is commented out. The error message proves | |
| // that dispose() is working as intended. | |
| // try { | |
| // notifier.addListener(() {}); | |
| // fail('Should have thrown an error'); | |
| // } catch (e) { | |
| // expect(e, isA<Error>()); | |
| // expect( | |
| // e.toString(), | |
| // contains('A ValueNotifier<UiDefinition?> was used after being disposed.'), | |
| // ); | |
| // } | |
| }); | |
| // After dispose, trying to add a listener should throw an error. | |
| expect(() => notifier.addListener(() {}), throwsA(isA<FlutterError>())); |
1 participant
Summary of Changes
This pull request introduces a new
SurfaceControllerclass to centralize the management of individual UI surfaces within thegenuipackage. This refactoring aims to improve modularity, encapsulation, and maintainability by moving surface-specific state and logic out of theGenUiManagerand into dedicated controllers. TheGenUiManagernow acts as a coordinator for theseSurfaceControllerinstances, and theGenUiSurfacewidget directly consumes aSurfaceController, streamlining the UI rendering process. This change impacts several example applications and tests, which have been updated to align with the new architecture.Highlights
SurfaceController, has been added to centralize the management of individual UI surfaces, encapsulatingUiDefinitionandDataModel.GenUiManagerhas been refactored to delegate surface-specific state management toSurfaceControllerinstances, acting as a coordinator for their lifecycle.GenUiSurfacewidget now directly accepts aSurfaceControllerinstead of separatehostandsurfaceIdparameters, simplifying its API and improving encapsulation.GenUiConversationclass has been updated to utilizeSurfaceControllerfor managing UI surfaces, including handlingonSurfaceAdded,onSurfaceDeleted, andonSurfaceUpdatedcallbacks withSurfaceControllerobjects.SurfaceController-based API, ensuring consistency and correctness across the codebase.Changelog
GenUiSurfaceto usecontrollerinstead ofsurfaceIdandhost._handleSurfaceAddedto acceptSurfaceController.MessageViewto receiveSurfaceController.MessageViewto acceptmessageControllerandsurfaceControllerdirectly.GenUiSurfaceusage to pass thecontroller.AiUiMessagecase to retrieve and passSurfaceControllertoGenUiSurface.userPromptBuilder.GenUiSurfaceto usecontroller.ValueListenableBuilderto listen tocontroller.uiDefinitionNotifier.GenUiSurfaceto usecontroller.ValueListenableBuilderto listen tocontroller.uiDefinitionNotifier.GenUiSurfaceto usecontroller.ValueListenableBuilderto listen tocontroller.uiDefinitionNotifier.GenUiSurfaceto usecontroller.ValueListenableBuilderto listen tocontroller.uiDefinitionNotifier.SurfaceControllerwithGenUiSurface._MyHomePageStateto useSurfaceControllerinonSurfaceAddedandGenUiSurface.SurfaceControllerwithGenUiSurface.src/core/surface_controller.dart.surface_controller.dart.onSurfaceAdded,onSurfaceDeleted,onSurfaceUpdatedcallbacks to useSurfaceController._handleSurfaceUpdateand_handleDefinitionUpdateto manageAiUiMessagehistory based onSurfaceController's notifier.surface()method withgetSurfaceController().GenUiUpdateto useSurfaceController.SurfaceUpdatedevent class.GenUiHostinterface implementation._surfacesmap with_surfaceControllersmap.getSurfaceControllerto create and manageSurfaceControllerinstances.SurfaceControllerinstances.surface_controller.dart.GenUiSurfaceconstructor to take acontrollerparameter.buildand_dispatchEventmethods to usewidget.controller.SurfaceControllerclass to manageUiDefinitionandDataModelfor a single UI surface.GenUiSurfaceto useSurfaceController.SurfaceControllerwithGenUiSurface.SurfaceControllerwithGenUiSurface.SurfaceControllerwithGenUiSurface.dataModelviaSurfaceController.SurfaceControllerwithGenUiSurface.SurfaceControllerwithGenUiSurface.dataModelviaSurfaceController.SurfaceControllerwithGenUiSurface.SurfaceControllerwithGenUiSurface.SurfaceControllerwithGenUiSurface.SurfaceControllerwithGenUiSurface.SurfaceControllerwithGenUiSurface.dataModelviaSurfaceController.SurfaceControllerwithGenUiSurface.SurfaceControllerwithGenUiSurface.dataModelviaSurfaceController.SurfaceControllerwithGenUiSurface.SurfaceControllerwithGenUiSurface.dataModelviaSurfaceController.FlutterErrorimport.getSurfaceController.SurfaceUpdatedandsurface()methods.handleMessageanddisposetests to reflectSurfaceControllerchanges.SurfaceController's disposal and initial state.SurfaceControllerwithGenUiSurface.dart:async.expectLaterforSurfaceAddedto checkcontroller.surfaceIdandcontroller.uiDefinitionNotifier.value.SurfaceUpdatedtest to use aCompleterandaddListeneronuiDefinitionNotifier._ChatScreenStateto handleSurfaceAddedandSurfaceRemovedevents usingSurfaceController.SurfaceUpdatedevent handling.GenUiSurfaceto usecontroller.