-
Notifications
You must be signed in to change notification settings - Fork 340
Add coldboot option to AVD emulators #3381
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
Conversation
ff3a965 to
231549f
Compare
DanTup
left a comment
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.
This is great, thanks! I've added some minor comments, but otherwise this looks good to me :-)
src/shared/vscode/device_manager.ts
Outdated
| } | ||
|
|
||
| type PickableDevice = vs.QuickPickItem & { device: f.Device | PlatformEnabler | Emulator | EmulatorCreator }; | ||
| type PickableDevice = vs.QuickPickItem & { device: f.Device | PlatformEnabler | Emulator | EmulatorCreator } & { coldBoot?: boolean | undefined | null }; |
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.
Do we need both undefined and null here? If so, it should be clear what each means (perhaps in a comment).
nits: I'd also use either ? or undefined but not both, and probably just include coldBoot in the same definition as the device field.
src/shared/vscode/device_manager.ts
Outdated
| coldBoot: true, | ||
| description: e.description, | ||
| device: e.device, | ||
| label: e.label + " (cold boot)", |
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.
Looking at the screenshot, I think it might look nicer if we put (cold boot) after android (I think in the description here?) rather than the label - what do you think?
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.
Yes, looks much better if set in the description field. I will change that.
231549f to
5b31948
Compare
src/shared/vscode/device_manager.ts
Outdated
| import { LogCategory } from "../enums"; | ||
| import * as f from "../flutter/daemon_interfaces"; | ||
| import { CustomEmulator, CustomEmulatorDefinition, Emulator, EmulatorCreator, IFlutterDaemon, Logger, PlatformEnabler } from "../interfaces"; | ||
| import { CustomEmulatorDefinition, Emulator, EmulatorCreator, IFlutterDaemon, Logger, PlatformEnabler } from "../interfaces"; |
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.
Strange, how did that change happen... I will fix that later today
DanTup
left a comment
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.
Looks good to me once the compile issue is fixed - thanks!
5b31948 to
8eb032d
Compare
|
@DanTup , I have no idea why the one workflow failed. Maybe some random failure? Could you might hit rerun? |
It seems like a VS Code flake to me. There are a lot of moving parts in these tests so it's not unusual for random failures. The change all looks good to me - thanks very much for the contribution! :) |

This pull request adds a cold boot option to each android based emulator.
The command palette looks as follows:

closes #3366