Prevent crashes on range errors when selecting device #129290
Conversation
github-actions
bot
added
the
tool
|
|
||
| Device? chosenDevice; | ||
| while (chosenDevice == null) { | ||
| final String userInputString = await _readUserInput(devices.length); | ||
| if (userInputString.toLowerCase() == 'q') { | ||
| throwToolExit(''); | ||
| } | ||
| final int deviceIndex = int.parse(userInputString) - 1; | ||
| if (deviceIndex > -1 && deviceIndex < devices.length) { | ||
| chosenDevice = devices[deviceIndex]; | ||
| } | ||
| } | ||
| return devices[int.parse(userInput) - 1]; | ||
| return chosenDevice; |
There was a problem hiding this comment.
@vashworth hey I noticed that we have two different places we are implementing logic for selecting one of the available devices... this PR has fixes for both places but wondering if we should simplify this and have a function that is used by both TargetDevices and TargetDevicesWithExtendedWirelessDeviceDiscovery?
Seems like it was a deliberate decision to have these two classes separate though so just wanted to check in with you
There was a problem hiding this comment.
Yeah, although they're similar, they're separate because TargetDevices knows in advance exactly what numbers are valid options, whereas TargetDevicesWithExtendedWirelessDeviceDiscovery does not since options get added asynchronously.
If you look into the TargetDevices._readUserInput function, it uses terminal.promptForCharInput, which handles the while loop and validates it's an acceptable input, so you shouldn't need to change this.
Since TargetDevicesWithExtendedWirelessDeviceDiscovery does not know in advance what options are valid, it can't use terminal.promptForCharInput, so it accepts any number as input and then validates it's a valid option.
There was a problem hiding this comment.
Ah good call, I will revert the changes made to TargetDevices then and find a test that uses TargetDevicesWithExtendedWirelessDeviceDiscovery. I think the test I tried to piggy back on was using TargetDevices method for selecting a device
`globals.terminal.promptForCharInput` method already checks that the input is valid
| List<String> input = <String>[]; | ||
|
|
||
| @override | ||
| Future<String> readUserInput() async { | ||
| return input; | ||
| // If only one value is provided for the input, continue | ||
| // to return that one input value without popping | ||
| // | ||
| // If more than one input values are provided, we are simulating | ||
| // the user selecting more than one option for a device, so we will pop | ||
| // them out from the front | ||
| if (input.length > 1) { | ||
| return input.removeAt(0); | ||
| } | ||
|
|
||
| return input[0]; |
There was a problem hiding this comment.
hey @vashworth, i reworked this method so that we provide lists of inputs for selecting a device.
Thoughts? The while loop in the TargetDeviceSelection.userSelectDevice() method makes it a bit difficult to mock multiple inputs over a test
| // Having the `0` first is an invalid choice for a device, the second | ||
| // item in the list, `1` is a valid option though which will return a | ||
| // valid device | ||
| targetDevices.deviceSelection.input = <String>['0', '1']; |
There was a problem hiding this comment.
Could you add another input here that's out of range on the top end too (so 2 in this case)?
eliasyishak
added
the
autosubmit
flutter/flutter@c40baf4...042c036 2023-06-22 [email protected] Remove unnecessary variable `_hasPrimaryFocus` (flutter/flutter#129066) 2023-06-22 [email protected] Fix Material 3 Scrollable `TabBar` (flutter/flutter#125974) 2023-06-22 [email protected] Fix: Closing bottom sheet and removing FAB cause assertion failure (flutter/flutter#128566) 2023-06-22 [email protected] Add `InputDecorationTheme.merge` (flutter/flutter#129011) 2023-06-22 [email protected] Roll Packages from 9af50d4 to 95bc1c6 (6 revisions) (flutter/flutter#129351) 2023-06-22 [email protected] Prevent crashes on range errors when selecting device (flutter/flutter#129290) 2023-06-22 [email protected] Revert "Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision)" (flutter/flutter#129353) 2023-06-22 [email protected] Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision) (flutter/flutter#129339) 2023-06-22 [email protected] Roll Flutter Engine from d9530e2b87de to 703c9a14ac7f (1 revision) (flutter/flutter#129337) 2023-06-22 [email protected] Roll Flutter Engine from c6251a69a09a to d9530e2b87de (1 revision) (flutter/flutter#129334) 2023-06-22 [email protected] Roll Flutter Engine from 08aaa88bf67f to c6251a69a09a (10 revisions) (flutter/flutter#129331) 2023-06-22 [email protected] Manual roll of packages to 9af50d4 (flutter/flutter#129328) 2023-06-21 [email protected] Roll Flutter Engine from 090fae83548a to 08aaa88bf67f (3 revisions) (flutter/flutter#129306) 2023-06-21 [email protected] [framework,web] add FlutterTimeline and semantics benchmarks that use it (flutter/flutter#128366) 2023-06-21 [email protected] Roll pub packages (flutter/flutter#128966) 2023-06-21 [email protected] Remove incorrect non-nullable assumption from `ShapeDecoration.lerp` (flutter/flutter#129298) 2023-06-21 [email protected] Gracefully handle negative position in getWordAtOffset (flutter/flutter#128464) 2023-06-21 [email protected] [flutter_tools] add a gradle error handler for could not open cache directory (flutter/flutter#129222) 2023-06-21 [email protected] Roll Flutter Engine from f973fb4636d3 to 090fae83548a (5 revisions) (flutter/flutter#129293) 2023-06-21 [email protected] Selection area right click behavior should match native (flutter/flutter#128224) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
2 participants
Prevent the cli from crashing when a user selects a number that is not valid for
flutter rundevice selectionFixes issue:
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.