close
Skip to content

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

Merged
elliette merged 5 commits into
flutter:masterfrom
elliette:autocomplete
Oct 14, 2021
Merged

Expression evaluation autocomplete overlay is positioned over the last . in the expression#3449
elliette merged 5 commits into
flutter:masterfrom
elliette:autocomplete

Conversation

@elliette

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, done!

enabledBorder: OutlineInputBorder(borderSide: BorderSide.none),
labelText: 'Eval',
),
determineOverlayXPosition:

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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


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

Copy link
Copy Markdown
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
Copy Markdown
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

@jacob314 jacob314 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@jacob314 jacob314 left a comment

Copy link
Copy Markdown
Contributor

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