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

Expression evaluation autocomplete overlay is positioned over the last . in the expression #3449

Merged
merged 5 commits into from
Oct 14, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
16 changes: 15 additions & 1 deletion packages/devtools_app/lib/src/debugger/evaluate.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import '../globals.dart';
import '../notifications.dart';
import '../theme.dart';
import '../ui/search.dart';
import '../ui/utils.dart';
import '../utils.dart';
import 'debugger_controller.dart';

Expand Down Expand Up @@ -135,14 +136,27 @@ class _ExpressionEvalFieldState extends State<ExpressionEvalField>
shouldRequestFocus: false,
supportClearField: true,
onSelection: _onSelection,
tracking: true,
decoration: const InputDecoration(
contentPadding: EdgeInsets.all(denseSpacing),
border: OutlineInputBorder(),
focusedBorder: OutlineInputBorder(borderSide: BorderSide.none),
enabledBorder: OutlineInputBorder(borderSide: BorderSide.none),
labelText: 'Eval',
),
determineOverlayXPosition:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: how about the name
overlayXPositionBuilder
or similar for consistency with other similar APIs in Flutter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, switched to overlayXPositionBuilder

(String inputValue, TextStyle inputStyle) {
// X-coordinate is equivalent to the width of the input text
// up to the last "." or the insertion point (cursor):
final textSegment = inputValue.contains('.')
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: slightly cleaner is:

final indexOfDot = inputValue.lastIndexOf('.')
final textSegment = indexOfDot != -1 ? inputValue.substring(0, indexOfDot + 1) : inputValue;

A little better as it doesn't have two different expressions with the same goal (contains and lastIndexOf) and is a little faster due to less dupe work not that it matters in this particular case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good idea! Switched to that

? inputValue.substring(0, inputValue.lastIndexOf('.') + 1)
: inputValue;
return calculateTextSpanWidth(
TextSpan(
text: textSegment,
style: inputStyle,
),
);
},
),
),
),
Expand Down
34 changes: 16 additions & 18 deletions packages/devtools_app/lib/src/ui/search.dart
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,11 @@ typedef ClearSearchField = Function(
bool force,
});

/// Provided by clients to specify where the autocomplete overlay should be
/// positioned relative to the input text.
typedef DetermineOverlayXPosition = double Function(
Copy link
Contributor

Choose a reason for hiding this comment

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

also rename this to match the new name. Consider not using a typedef unless you think it adds value. I think the typedef is only used once so probably would be reasonable to remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, renamed. Still using the typedef since it's being used twice

String inputValue, TextStyle inputStyle);
Copy link
Member

Choose a reason for hiding this comment

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

nit: trailing comma

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, done!


mixin SearchFieldMixin<T extends StatefulWidget> on State<T> {
TextEditingController searchTextFieldController;
FocusNode _searchFieldFocusNode;
Expand Down Expand Up @@ -601,9 +606,10 @@ mixin SearchFieldMixin<T extends StatefulWidget> on State<T> {
/// [searchFieldKey]
/// [searchFieldEnabled]
/// [onSelection]
/// [onHilightDropdown] use to override default highlghter.
/// [onHighlightDropdown] use to override default highlghter.
/// [decoration]
/// [tracking] if true displays pop-up to the right of the TextField's caret.
/// [determineOverlayXPosition] callback function to determine where the
/// autocomplete overlay should be positioned relative to the input text.
/// [supportClearField] if true clear TextField content if pop-up not visible. If
/// pop-up is visible close the pop-up on first ESCAPE.
/// [keyEventsToPropogate] a set of key events that should be propogated to
Expand All @@ -617,7 +623,7 @@ mixin SearchFieldMixin<T extends StatefulWidget> on State<T> {
HighlightAutoComplete onHighlightDropdown,
InputDecoration decoration,
String label,
bool tracking = false,
DetermineOverlayXPosition determineOverlayXPosition,
bool supportClearField = false,
Set<LogicalKeyboardKey> keyEventsToPropogate = const {},
VoidCallback onClose,
Expand All @@ -633,7 +639,7 @@ mixin SearchFieldMixin<T extends StatefulWidget> on State<T> {
searchTextFieldController: searchTextFieldController,
decoration: decoration,
label: label,
tracking: tracking,
determineOverlayXPosition: determineOverlayXPosition,
onClose: onClose,
);

Expand Down Expand Up @@ -718,6 +724,7 @@ class _SearchField extends StatelessWidget {
this.tracking = false,
this.decoration,
this.onClose,
this.determineOverlayXPosition,
});

final SearchControllerMixin controller;
Expand All @@ -731,30 +738,21 @@ class _SearchField extends StatelessWidget {
final bool tracking;
final InputDecoration decoration;
final VoidCallback onClose;
final DetermineOverlayXPosition determineOverlayXPosition;

@override
Widget build(BuildContext context) {
final textStyle = Theme.of(context).textTheme.subtitle1;
final searchField = TextField(
key: searchFieldKey,
autofocus: true,
enabled: searchFieldEnabled,
focusNode: searchFieldFocusNode,
controller: searchTextFieldController,
style: textStyle,
onChanged: (value) {
if (tracking) {
// Use a TextPainter to calculate the width of the newly entered text.
// TODO(terry): The TextPainter's TextStyle is default (same as this
// TextField) consider explicitly using a TextStyle of
// this TextField if the TextField needs styling.
final painter = TextPainter(
textDirection: TextDirection.ltr,
text: TextSpan(text: value),
);
painter.layout();

// X coordinate of the pop-up, immediately to the right of the insertion
// point (caret).
controller.xPosition = painter.width;
if (determineOverlayXPosition != null) {
controller.xPosition = determineOverlayXPosition(value, textStyle);
}
controller.search = value;
},
Expand Down