Skip to content
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

inspector add screenshot #4274

Closed
wants to merge 1 commit into from
Closed

Conversation

mengdouer
Copy link

@mengdouer mengdouer commented Jul 21, 2022

This pr is a screenshot of the current widget that can be displayed on the inspect screen when you double-click its widget. Below is its demo. Inside our company, this feature is very helpful in debugging.
It's still in the draft stage, not sure how you feel about this change, any thoughts on this can be discussed here.

2022-07-21.18.31.11.mov

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking

for advice on the #hackers-new channel on Discord.

@kenzieschmoll
Copy link
Member

Thanks for the PR!

@jacob314 @InMatrix - is this something we've considered adding to the inspector in the past? Any thoughts on this UX?

return SynchronousFuture<EasyImageProvider>(this);
}

@override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why are we overriding == and hashcode?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The implementation of this component is in another function big picture detection, and may refer to the implementation of other providers. Here is directly reused. It may not be needed here. i will delete it.

if (useDaemonApi) {
return invokeServiceMethodDaemonParams('screenshot', {
'id': selection.id,
'width': 480,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pass in width and height as named parameters to this method.
I would also recommend using sizes based on the size available in the UI rather than forcing to 480 and 640. Smaller screenshots are significantly faster to send from the device and to encode.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes you are right i will revise it.

return Text("no data");
}
return Container(
width: 480,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make these const and adjust based on the size of the window. If DevTools is displayed in a small embedded window, you probably want a smaller screenshot.

@InMatrix
Copy link

@jacob314 @InMatrix - is this something we've considered adding to the inspector in the past? Any thoughts on this UX?

We've considered this kind of widget screenshot in the HotUI experiment: https://docs.google.com/document/d/1ZaHqKnON8-WEhke3Y6FpHeuB5BNlxDQj1cCYB1SoI_g/edit#heading=h.pub7jnop54q0

About the specific UI, I wonder if we can solve the same problem by hooking up widget selection in the tree view to the on-device widget highlighter. Essentially, when you select a node in the tree, the corresponding widget will get highlighted on the device/simulator automatically, allowing you to identify the widget. @zzm990321 Is this what you use the widget screenshots for? It would be helpful if you can elaborate on your use case a bit more.

@jacob314
Copy link
Contributor

I'd added that API to Flutter for use cases similar to this but never had time to implement in DevTools.
Some things to consider:

  1. Should polish to handle widgets with transparent backgrounds better. For example, see what it does when selecting a text widget.
  2. Consider showing the screenshot without a click when viewing the layout explorer view.
  3. Consider alternate UX of showing a screenshot of the full app in the inspector and highlighting the bounding box for the selected widget.

@mengdouer
Copy link
Author

@jacob314 @InMatrix - is this something we've considered adding to the inspector in the past? Any thoughts on this UX?

We've considered this kind of widget screenshot in the HotUI experiment: https://docs.google.com/document/d/1ZaHqKnON8-WEhke3Y6FpHeuB5BNlxDQj1cCYB1SoI_g/edit#heading=h.pub7jnop54q0

About the specific UI, I wonder if we can solve the same problem by hooking up widget selection in the tree view to the on-device widget highlighter. Essentially, when you select a node in the tree, the corresponding widget will get highlighted on the device/simulator automatically, allowing you to identify the widget. @zzm990321 Is this what you use the widget screenshots for? It would be helpful if you can elaborate on your use case a bit more.

Thank you for your reply. I think it's good to display it on the device too, but viewing it on devtools makes it clear that only a widget is viewed,it will not be affected by other displays and this image can be saved.

@mengdouer
Copy link
Author

I'd added that API to Flutter for use cases similar to this but never had time to implement in DevTools. Some things to consider:

  1. Should polish to handle widgets with transparent backgrounds better. For example, see what it does when selecting a text widget.
  2. Consider showing the screenshot without a click when viewing the layout explorer view.
  3. Consider alternate UX of showing a screenshot of the full app in the inspector and highlighting the bounding box for the selected widget.

Sincerely thank you for your advice.

  1. Yes, the transparent component display is not good, I will optimize it later.
  2. Can we add a tab to the right of the Details Trees so that when the component is selected we can view the screenshot without additional clicks.

image

3. Explicitly wanting to see a screenshot of a single component I think is sometimes useful too. I was also able to display both images to devtools. I think it would still be helpful to show it on devtools. Of course, I still need to refer to your comments.

@InMatrix @jacob314 Thanks for the suggestion. After confirming that the code can be merged, I will fix code problem uniformly

@InMatrix
Copy link

Can we add a tab to the right of the Details Trees so that when the component is selected we can view the screenshot without additional clicks.

This sounds reasonable. Could you prototype it and post a video?

Also, how performant is taking screenshots for individual widgets? For example, when the user quickly cycle through a bunch of widgets in the tree view, how long does it take for each screenshot to load?

@mengdouer
Copy link
Author

Can we add a tab to the right of the Details Trees so that when the component is selected we can view the screenshot without additional clicks.

This sounds reasonable. Could you prototype it and post a video?

Also, how performant is taking screenshots for individual widgets? For example, when the user quickly cycle through a bunch of widgets in the tree view, how long does it take for each screenshot to load?

OK I'll implement a prototype later. In terms of performance, there is no test yet. I think @jacob314 may know more about it, and we can listen to his opinions and I will do some tests later too

@Hixie
Copy link

Hixie commented Oct 18, 2022

@mengdouer Is this PR still on your radar?

@Hixie
Copy link

Hixie commented Feb 14, 2023

@mengdouer thanks for your contribution!
I'm going to close this PR since it appears work here has stalled, but please do not hesitate to reopen it if you would like to resume work. You may also wish to file a bug with this proposal and link to this PR from the bug in case someone else would like to take over!

@Hixie Hixie closed this Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants