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

[custom-devices] add screenshotting support #80675

Merged

Conversation

ardera
Copy link
Contributor

@ardera ardera commented Apr 18, 2021

This implements #79678.

I tested it on my device and the flutter SDK part of this works. However it's still not possible to take screenshots of the app, at least when using flutter-pi because the device-part of screenshotting support is missing. I.e. there's currently no ready-to-use command to take screenshots of the Raspberry Pi when using KMS directly without X11 / Wayland. You can get a fbdev screenshot (and that's what I tested) and that works, but only shows the console. So in the future I'll probably add some kind of support inside flutter-pi to take screenshots.

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 and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • 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 feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 18, 2021
@google-cla google-cla bot added the cla: yes label Apr 18, 2021
@jonahwilliams jonahwilliams self-requested a review April 19, 2021 17:36
@jonahwilliams
Copy link
Member

This would use a platform specific screenshot mechanism right?

Context is that I also have #80616 as a WIP which will allow falling back to a "rasterizer" screenshot. That should allow all platforms to have some level of screenshot support.

@ardera
Copy link
Contributor Author

ardera commented Apr 19, 2021

This would use a platform specific screenshot mechanism right?

Context is that I also have #80616 as a WIP which will allow falling back to a "rasterizer" screenshot. That should allow all platforms to have some level of screenshot support.

Yep exactly. Just adds a new command to the config that will take the screenshot and store it in some path. If the command isn't present, the CustomDevice will report that screenshotting isn't supported.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Overall LGTM, except for comment on the regex replace of output

@ardera ardera force-pushed the feature/custom-device-screenshotting branch from c4107b8 to 62cd5b8 Compare April 25, 2021 01:25
@ardera
Copy link
Contributor Author

ardera commented Apr 25, 2021

how am I getting this allowlist error? The problem with it started happening way after the base master commit I'm using and the bundle builder refactor (which used the same upstream commit as base) tested fine. I'll try rebasing onto latest master tomorrow

@jonahwilliams
Copy link
Member

There was a bug in the test script, you'll need to pull lastest master to get the fix

@jonahwilliams
Copy link
Member

@ardera do you need any help getting synced to a good flutter hash?

@ardera ardera force-pushed the feature/custom-device-screenshotting branch from 62cd5b8 to 75d6f5d Compare April 29, 2021 21:40
@ardera
Copy link
Contributor Author

ardera commented Apr 29, 2021

@ardera do you need any help getting synced to a good flutter hash?

Thanks but I just didn't have time on sunday and wanted to do it later instead 😄 probably one reason why you guys don't give time estimates ;)

@jonahwilliams
Copy link
Member

Fair enough 😄

@ardera
Copy link
Contributor Author

ardera commented Apr 29, 2021

So there's one test failing, but I'm not sure it's related. Seems like it times out after 25s, but some other tests take long as well (the Test is taking a long time message is printed out a few other times)

04:38 +9 ~14: test\integration.shard\overall_experience_test.dart: flutter error messages include a DevTools link                                                                                      
Test is taking a long time.
Expected state transitions:
    0 NOW WAITING FOR> /^An Observatory debugger and profiler on Flutter test device is available at: /
    1                  /^The Flutter DevTools debugger and profiler on Flutter test device is available at: /
    2                  /^Performing hot reload\.\.\./
    3                  /^Reloaded 0 libraries in [0-9]+ms./
So far nothing has been logged; use debug:true to print all output.
(streaming all logs from this point on...)
stdout: Syncing files to device Flutter test device...                     265ms
stdout: 
stdout: Flutter run key commands.
stdout: r Hot reload. 
stdout: R Hot restart.
stdout: h List all available interactive commands.
stdout: d Detach (terminate "flutter run" but leave application running).
stdout: c Clear the screen
stdout: q Quit (terminate the application on the device).
stdout: 
stdout:  Running with sound null safety 
stdout: 
stdout: An Observatory debugger and profiler on Flutter test device is available at: http://127.0.0.1:53624/49ye2qa6uGk=/
(matched /^An Observatory debugger and profiler on Flutter test device is available at: /)
(process is not quitting, trying to send a "q" just in case that helps)
(a functional test should never reach this point)
stdout: 
stdout: Application finished.
(process terminated with exit code 0)
The subprocess terminated before all the expected transitions had been matched.
The transition that we were hoping to see next but that we never saw was:
    1 NOW WAITING FOR> /^The Flutter DevTools debugger and profiler on Flutter test device is available at: /

04:50 +9 ~14 -1: test\integration.shard\overall_experience_test.dart: flutter error messages include a DevTools link [E]                                                                               
  Missed some expected transitions.
  test\integration.shard\overall_experience_test.dart 275:5  runFlutter

@jonahwilliams
Copy link
Member

That is unfortunately another broken test, we just landed 1b4f448 to work around it

…st true

- don't trim the screenshot output in the SDK, let the command do it
- change the default screenshot command to use 'tr' to remove whitespaces, tabs and newlines
- remove some logging in takeScreenshot since processUtils.run will do the necessary logging itself
@ardera ardera force-pushed the feature/custom-device-screenshotting branch from 75d6f5d to 408e7a9 Compare April 29, 2021 23:06
@ardera
Copy link
Contributor Author

ardera commented Apr 29, 2021

Okay, rebased onto that now

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@fluttergithubbot fluttergithubbot merged commit 82830fa into flutter:master Apr 30, 2021
@ardera ardera deleted the feature/custom-device-screenshotting branch July 17, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants