-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added takeScreenshot Action #1775
base: main
Are you sure you want to change the base?
Conversation
@mehsaandev no reviewer? |
also which ticket does this address? add the proposed yaml to that ticket. Let's get things done properly please |
@snehmehta PTAL |
@@ -109,6 +109,7 @@ dependencies: | |||
web_socket_client: ^0.1.0 | |||
app_links: ^6.3.2 | |||
share_plus: ^10.0.3 | |||
screenshot: ^3.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look at the source code, i don't think this package add that much value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am getting ScreenshotController from this package.
throw LanguageError("Widget not found: '$widgetId'"); | ||
} | ||
|
||
final widget = Screenshot( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this will work as you are taking a live widget that putting it in a separate render tree? have you tested it in android, ios and web with many different types of widgets? what if the widget is a form with values entered by the user? if it is limited to readonly widgets, make that clear.
child: widget, | ||
), | ||
), | ||
delay: const Duration(milliseconds: 1000), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why delay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ADD comments in the code!
final widget = Screenshot( | ||
controller: screenshotController, | ||
child: DataScopeWidget( | ||
scopeManager: scopeManager, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test the case where the takescreenshot action is being invoked from the bottom sheet or a dialog and the widgetId is of a widget that is on the main screen. scopeManager may have issues in that case so please check that case.
event: EnsembleEvent(initiator, data: {'error': e.toString()}), | ||
); | ||
} | ||
rethrow; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you actually tested it? rethrowing the error may cause issues. TEST the error case please
@@ -1165,6 +1167,8 @@ abstract class EnsembleAction { | |||
return FilePickerAction.fromYaml(payload: payload); | |||
} else if (actionType == ActionType.openUrl) { | |||
return OpenUrlAction.fromYaml(payload: payload); | |||
} else if (actionType == ActionType.takeScreenshot) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please test to make sure that this action can be invoked from javascript as ensemble.takeScreenshot
You have only added the yaml support and not js support. ALWAYS test the js as well and add that as kitchen sink example and documentation
No description provided.