Skip to content

[Property Editor] Handle deprecated widget arguments #9062

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.

Sign up for GitHub

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

Merged
elliette merged 3 commits into from
Mar 25, 2025
Merged

[Property Editor] Handle deprecated widget arguments #9062

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03eeec8
Exclude deprecated arguments
elliette Mar 24, 2025
a4088af
Add test cases
elliette Mar 24, 2025
cea99d7
Add filter condition inline
elliette Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/devtools_app/lib/src/shared/editor/api_classes.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ abstract class Field {
static const hasArgument = 'hasArgument';
static const id = 'id';
static const isDarkMode = 'isDarkMode';
static const isDeprecated = 'isDeprecated';
static const isEditable = 'isEditable';
static const isNullable = 'isNullable';
static const isRequired = 'isRequired';
Expand Down Expand Up @@ -524,6 +525,7 @@ class EditableArgument with Serializable {
required this.isNullable,
required this.isRequired,
required this.isEditable,
required this.isDeprecated,
required this.hasDefault,
this.options,
this.value,
Expand All @@ -540,6 +542,7 @@ class EditableArgument with Serializable {
isNullable: (map[Field.isNullable] as bool?) ?? false,
isRequired: (map[Field.isRequired] as bool?) ?? false,
isEditable: (map[Field.isEditable] as bool?) ?? true,
isDeprecated: (map[Field.isDeprecated] as bool?) ?? false,
hasDefault: map.containsKey(Field.defaultValue),
options: (map[Field.options] as List<Object?>?)?.cast<String>(),
value: map[Field.value],
Expand Down Expand Up @@ -575,6 +578,9 @@ class EditableArgument with Serializable {
/// where previous positional parameters have no argument.
final bool isEditable;

/// Whether the argument is deprecated.
final bool isDeprecated;

/// A list of values that could be provided for this argument.
///
/// This will only be included if the parameter's [type] is "enum".
Expand Down Expand Up @@ -613,6 +619,7 @@ class EditableArgument with Serializable {
Field.isNullable: isNullable,
Field.isRequired: isRequired,
Field.isEditable: isEditable,
Field.isDeprecated: isDeprecated,
Field.options: options,
Field.displayValue: displayValue,
Field.errorText: errorText,
Expand Down
1 change: 1 addition & 0 deletions ...ools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_controller.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ class PropertyEditorController extends DisposableController
(result?.args ?? <EditableArgument>[])
.map(argToProperty)
.nonNulls
.where(notDeprecatedWithNoValue)
.toList();
final name = result?.name;
_editableWidgetData.value = (
Expand Down
4 changes: 4 additions & 0 deletions .../devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_types.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ class EditableProperty extends EditableArgument {
isNullable: argument.isNullable,
isRequired: argument.isRequired,
isEditable: argument.isEditable,
isDeprecated: argument.isDeprecated,
options: argument.options,
displayValue: argument.displayValue,
errorText: argument.errorText,
Expand Down Expand Up @@ -219,6 +220,9 @@ EditableProperty? argToProperty(EditableArgument argument) {
}
}

bool notDeprecatedWithNoValue(EditableArgument argument) =>
Copy link
Copy Markdown
Member

@kenzieschmoll kenzieschmoll Mar 24, 2025

Choose a reason for hiding this comment

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

nit: consider making this function phrased in the positive deprecatedAndNotSet and then negate it where you call the method. I think this would make it easier to read rather than having to mentally parse the negatives.

Copy link
Copy Markdown
Member

@kenzieschmoll kenzieschmoll Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It actually seems like the case we want to check for is deprecatedAndSet right? and then we only include properties that are either not deprecated or deprecatedAndSet?

Copy link
Copy Markdown
Member Author

@elliette elliette Mar 24, 2025

Choose a reason for hiding this comment

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

Haha yeah, I removed this method and just added the filter condition inline to where we actually filter out the deprecated properties. I think with a comment this is more clear than trying to come up with a descriptive method name:

// Filter out any deprecated properties that aren't set.
.where((property) => !property.isDeprecated || property.hasArgument)

!(argument.isDeprecated && !argument.hasArgument);

/// The following types should match those returned by the Analysis Server. See:
/// https://github.com/dart-lang/sdk/blob/154b473cdb65c2686bb44fedec03ba2deddb80fd/pkg/analysis_server/lib/src/lsp/handlers/custom/editable_arguments/handler_editable_arguments.dart#L182
const stringType = 'string';
Expand Down
18 changes: 17 additions & 1 deletion ...s/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_view.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ class _PropertyLabels extends StatelessWidget {
Widget build(BuildContext context) {
final colorScheme = Theme.of(context).colorScheme;
final isSet = property.hasArgument;
final isDeprecated = property.isDeprecated;
final isDefault = property.isDefault;

return LayoutBuilder(
Expand All @@ -214,7 +215,22 @@ class _PropertyLabels extends StatelessWidget {
textColor: colorScheme.onPrimary,
),
),
if (isDefault)
// We exclude deprecated properties that are not set, so this label
// is always displayed under the "set" label.
if (isDeprecated)
Padding(
padding: _labelPadding(isTopLabel: !isSet),
child: RoundedLabel(
labelText: _maybeTruncateLabel('deprecated', width: width),
tooltipText: 'Property argument is deprecated.',
fontSize: smallFontSize,
backgroundColor: colorScheme.error,
textColor: colorScheme.onError,
),
),
// We only have space for two labels, so the deprecated label takes
// precedence over the default label.
if (isDefault && !isDeprecated)
Padding(
padding: _labelPadding(isTopLabel: !isSet),
child: RoundedLabel(
Expand Down
109 changes: 108 additions & 1 deletion ...ages/devtools_app/test/standalone_ui/ide_shared/property_editor/property_editor_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ void main() {
(document: textDocument2, position: activeCursorPosition2): result2,
(document: textDocument1, position: activeCursorPosition3): resultWithText,
(document: textDocument1, position: activeCursorPosition4): resultWithTitle,
(document: textDocument1, position: activeCursorPosition5):
deprecatedResult,
};

late MockEditorClient mockEditorClient;
Expand Down Expand Up @@ -145,6 +147,27 @@ void main() {
});
});

testWidgets('verify editable arguments when some are deprecated', (
tester,
) async {
await tester.runAsync(() async {
// Load the property editor.
await tester.pumpWidget(wrap(propertyEditor));
final editableArgsFuture = waitForEditableArgs();

// Send an active location changed event.
eventController.add(activeLocationChangedEvent5);

// Wait for the expected editable args.
final editableArgs = await editableArgsFuture;
verifyEditableArgs(
actual: editableArgs,
// Only deprecated properties with set arguments should be included.
expected: [deprecatedPropertyWithArg],
);
});
});

testWidgets('verify editable arguments update when widget changes', (
tester,
) async {
Expand Down Expand Up @@ -329,6 +352,30 @@ void main() {
});
});

group('inputs for deprecated arguments', () {
testWidgets('inputs are expected for deprecated arguments', (tester) async {
// Load the property editor.
await tester.pumpWidget(wrap(propertyEditor));

// Change the editable args.
controller.initForTestsOnly(editableArgsResult: deprecatedResult);
await tester.pumpAndSettle();

final deprecatedWithArgInput = _findDropdownButtonFormField(
'deprecatedWithArg',
);

// Verify the inputs are expected.
expect(deprecatedWithArgInput, findsOneWidget);

// Verify the labels and required are expected.
_labelsAndRequiredTextAreExpected(
deprecatedWithArgInput,
inputExpectations: deprecatedWithArgInputExpectations,
);
});
});

group('filtering editable arguments', () {
testWidgets('can filter by name', (tester) async {
// Load the property editor.
Expand Down Expand Up @@ -884,8 +931,17 @@ void _labelsAndRequiredTextAreExpected(
shouldBeSet ? findsOneWidget : findsNothing,
reason: 'Expected to find ${shouldBeSet ? 'a' : 'no'} "set" badge.',
);
// Check for the existence/non-existence of the "deprecated" badge.
final shouldBeDeprecated = inputExpectations['isDeprecated'] == true;
expect(
_labelForInput(inputFinder, matching: 'deprecated'),
shouldBeDeprecated ? findsOneWidget : findsNothing,
reason:
'Expected to find ${shouldBeDeprecated ? 'a' : 'no'} "deprecated" badge.',
);
// Check for the existence/non-existence of the "default" badge.
final shouldBeDefault = inputExpectations['isDefault'] == true;
final shouldBeDefault =
inputExpectations['isDefault'] == true && !shouldBeDeprecated;
expect(
_labelForInput(inputFinder, matching: 'default'),
shouldBeDefault ? findsOneWidget : findsNothing,
Expand Down Expand Up @@ -1058,6 +1114,18 @@ final activeLocationChangedEvent4 = ActiveLocationChangedEvent(
textDocument: textDocument1,
);

// Location position 5
final activeCursorPosition5 = CursorPosition(character: 81, line: 19);
final anchorCursorPosition5 = CursorPosition(character: 113, line: 12);
final editorSelection5 = EditorSelection(
active: activeCursorPosition5,
anchor: anchorCursorPosition5,
);
final activeLocationChangedEvent5 = ActiveLocationChangedEvent(
selections: [editorSelection5],
textDocument: textDocument1,
);

final notADartDocument = TextDocument(
uriAsString: '/my/fake/other.js',
version: 1,
Expand Down Expand Up @@ -1122,6 +1190,7 @@ final heightInputExpectations = {
'isSet': false,
'isRequired': false,
'isDefault': true,
'isDeprecated': false,
};
final result1 = EditableArgumentsResult(
name: widgetName,
Expand All @@ -1143,6 +1212,7 @@ final softWrapInputExpectations = {
'isSet': false,
'isRequired': false,
'isDefault': true,
'isDeprecated': false,
};
final alignProperty = EditableArgument.fromJson({
'name': 'align',
Expand All @@ -1169,12 +1239,49 @@ final alignInputExpectations = {
'isSet': true,
'isRequired': false,
'isDefault': false,
'isDeprecated': false,
};
final result2 = EditableArgumentsResult(
name: widgetName,
args: [softWrapProperty, alignProperty],
);

// Result for test cases of deprecated properties
final deprecatedPropertyNoArg = EditableArgument.fromJson({
'name': 'deprecatedNoArg',
'type': 'bool',
'isNullable': false,
'defaultValue': false,
'hasArgument': false,
'isEditable': true,
'isRequired': false,
'isDeprecated': true,
});

final deprecatedPropertyWithArg = EditableArgument.fromJson({
'name': 'deprecatedWithArg',
'type': 'bool',
'value': false,
'isNullable': false,
'defaultValue': true,
'hasArgument': true,
'isEditable': true,
'isRequired': false,
'isDeprecated': true,
});

final deprecatedWithArgInputExpectations = {
'isSet': true,
'isRequired': false,
'isDefault': false,
'isDeprecated': true,
};

final deprecatedResult = EditableArgumentsResult(
name: widgetName,
args: [deprecatedPropertyNoArg, deprecatedPropertyWithArg],
);

// Example results for documentation test cases.
final resultWithWidgetNameAndDocs = result1;
final resultWithWidgetNameNoDocs = result2;
Expand Down
Loading