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

Conversation

elliette
Copy link
Member

  • Previously we had a boolean parameter tracking which if true would display the autocomplete overlay over the cursor
  • Instead, clients now provide a callback function that determines the X-position of the overlay. For the case of the expression evaluation autocomplete, this callback returns the position of the last . in the expression (if there is a .).

Demo:

expression evaluation 2

This PR is part of the work towards #3095

@elliette elliette changed the title Expression evaluation autocomplete overlay should be position over the last . in the expression Expression evaluation autocomplete overlay is positioned over the last . in the expression Oct 13, 2021
/// Provided by clients to specify where the autocomplete overlay should be
/// positioned relative to the input text.
typedef DetermineOverlayXPosition = double Function(
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!

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

@@ -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

Copy link
Contributor

@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

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

lgtm

@elliette elliette merged commit 1ece492 into flutter:master Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants