Clarify TextAffinity docs #7238

justinmc · 2018-12-17T23:07:03Z

Clarifies the docs for flutter/flutter#25229.

A bug and some incorrect tests were found in the Flutter framework due to a misuse of TextAffinity, so this tries to make the details more clear.

justinmc · 2018-12-17T23:07:53Z

@Hixie I mainly wrote these docs based on the explanation you gave me last week, so I thought I'd tag you here to make sure I got it right.

HansMuller

This is a difficult topic. Hall of mirrors.

HansMuller · 2018-12-19T17:10:57Z

lib/ui/text.dart

+/// will be displayed as LTR and the uppercase letters RTL: "helloHELLO".  The
+/// string would appear as "helloOLLEH". A text position of 6 would be
+/// ambiguous, since it could be represented as being to the right of the "o"
+/// or to the right of the "H".


It would be helpful if this paragraph concluded by saying that if TextAffinity was upstream text position 6 ( think you mean 5) would mean for a text cursor.

HansMuller · 2018-12-19T17:19:08Z

lib/ui/text.dart

 enum TextAffinity {
  /// The position has affinity for the upstream side of the text position.
  ///
-  /// For example, if the offset of the text position is a line break, the


We really need to provide an intuitive definition of upstream here. If it can be explained in terms of characters being entered with a text field then perhaps upstream is towards the older characters, and downstream is towards the newer ones.

Since BIDI text is where this gets pretty complicated it might be worth stuffing a helloHELLO example in here and in the downstream doc.

It would also be worth reiterating what "text position" means. As I understand it, a text position indicates a location in between two characters in the internal representation of the text - not the displayed version. So offset 5 in "helloHELLO" is in between the o and the H. But in the rendered version of the text, "helloOLLEH", offset 5 is either to the right of the "o" for upstream affinity, or to the right of the H for downstream.

That's what I'm assuming is correct anwyay ...

HansMuller · 2018-12-19T17:20:40Z

lib/ui/text.dart

  /// position represents the start of the second line.
+  ///
+  /// As another example, in the bidirectional text example above, a text
+  /// position of 6 would appear just to the right of the "H".
  downstream,
 }

 /// A visual position in a string of text.


We really need to make it clear that this is a position in a string, not a position in the string when it's displayed. And it corresponds to be location in between characters.

justinmc · 2018-12-19T22:57:02Z

@HansMuller Updated with your suggestions and ready for re-review. I tried to be super clear and give fundamental definitions and separate examples. Let me know if this one makes sense.

HansMuller

This is much improved!

LGTM with a few additional suggestions about wording.

HansMuller · 2018-12-20T20:05:17Z

lib/ui/text.dart

 ///
-/// Must be specified along with [TextAffinity] in order to disambiguate certain
-/// situations. See the docs for TextAffinity for further details.
+/// An offset into a string in code is ambiguous with its corresponding position


Here and elsewhere: you may be working too hard to distinguish a string and a string rendering. For example in this case you could just say: The location of an offset in a rendered string is ambiguous in two cases.

I made a suggestion about wording for a few of the cases. Might be worth reconsidering the wording where "string in code" appears.

HansMuller · 2018-12-20T20:05:29Z

lib/ui/text.dart

@@ -884,21 +904,21 @@ class TextPosition {
  }) : assert(offset != null),
       assert(affinity != null);

-  /// The index of the character that immediately follows the position.
+  /// The index of the character that immediately follows the position in the
+  /// string represenation of the text.


representation

HansMuller · 2018-12-20T20:07:43Z

lib/ui/text.dart

@@ -833,47 +833,67 @@ class TextBox {
  String toString() => 'TextBox.fromLTRBD(${left.toStringAsFixed(1)}, ${top.toStringAsFixed(1)}, ${right.toStringAsFixed(1)}, ${bottom.toStringAsFixed(1)}, $direction)';
 }

-/// Whether a [TextPosition] is visually upstream or downstream of its offset.
+/// Disambiguates cases where an offset into a string in code could represent


Disambiguates cases where a string offset could match two locations in the rendered string.

HansMuller · 2018-12-20T20:08:13Z

lib/ui/text.dart

-/// string would appear as "helloOLLEH". A text position of 6 would be
-/// ambiguous, since it could be represented as being to the right of the "o"
-/// or to the right of the "H".
+/// LTR and RTL text. Consider the following string representation in code,


Consdier the following string,

justinmc · 2018-12-20T22:00:36Z

@HansMuller Just made those fixes, I think this is much better. Thanks for the help. Let me know if you see anything else before I merge.

cbracken · 2018-12-20T22:52:09Z

lib/ui/text.dart

@@ -833,28 +833,66 @@ class TextBox {
  String toString() => 'TextBox.fromLTRBD(${left.toStringAsFixed(1)}, ${top.toStringAsFixed(1)}, ${right.toStringAsFixed(1)}, ${bottom.toStringAsFixed(1)}, $direction)';
 }

-/// Whether a [TextPosition] is visually upstream or downstream of its offset.
+/// Disambiguates cases where a string offset could match two locations in the
+/// rendered string.


It seems useful to preserve the reference to TextPosition in the doc comment to help with discoverability.

cbracken · 2018-12-20T22:53:39Z

lib/ui/text.dart

@@ -833,28 +833,66 @@ class TextBox {
  String toString() => 'TextBox.fromLTRBD(${left.toStringAsFixed(1)}, ${top.toStringAsFixed(1)}, ${right.toStringAsFixed(1)}, ${bottom.toStringAsFixed(1)}, $direction)';
 }

-/// Whether a [TextPosition] is visually upstream or downstream of its offset.
+/// Disambiguates cases where a string offset could match two locations in the
+/// rendered string.


nit: Generally prefer to document types using a noun-phrase as the summary sentence.

https://www.dartlang.org/guides/language/effective-dart/documentation#prefer-starting-library-or-type-comments-with-noun-phrases

justinmc · 2018-12-20T23:31:32Z

@cbracken Thanks for the tips, I opened a quick PR to improve that a bit more: #7274

flutter/engine@6a90418...54a3577 git log 6a90418..54a3577 --no-merges --oneline 54a3577 Roll src/third_party/skia b03e024a4034..bdfe3a3ee9c3 (14 commits) (flutter/engine#7277) ead76a4 Remove unused native function dumpCopmpilationTrace(). (flutter/engine#7276) eaffc46 [License] Enable avoid_positional_boolean_parameters lint (flutter/engine#7275) b07753b [License] Sync analysis_options.yaml from framework (flutter/engine#7273) 233c100 [License] Enable always_require_non_null_named_parameters f4b348f [License] Enable flutter_style_todos lint 48be433 [License] Enable prefer_is_empty lint bd648ba [License] Enable prefer_const_constructors lint 2b207a5 [License] Enable unnecessary_parenthesis lint 97ae271 [License] Enable prefer_initializing_formals lint c8f83fb [License] Enable prefer_collection_literals lint d120e15 [License] Enable prefer_void_to_null lint e2ab378 [License] Enable directives_ordering lint 5f2e6cd [License] Enable unnecessary_overrides lint bd955d7 [License] Enable avoid_function_literals_in_foreach_calls lint 392df65 [License] Enable prefer_const_declarations lint 06e3591 [License] Enable prefer_final_fields lint 32423ef [License] Enable prefer_asserts_in_initializer_lists lint 4703a7f [License] Enable prefer_equal_for_default_values lint 1d7285b [License] Enable empty_catches lint 68fadca [License] Enable unnecessary_this lint c6f5e67 [License] Enable prefer_single_quotes lint cc51731 Clarify TextAffinity docs (flutter/engine#7238) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

Hixie · 2019-01-06T23:14:48Z

lib/ui/text.dart

+/// characters. For newline characters, the position is fully specified by the
+/// offset alone, and there is no ambiguity.
+///
+/// TextAffinity also affects bidirectional text at the interface between LTR


nit: [TextAffinity]

Hixie · 2019-01-06T23:15:03Z

lib/ui/text.dart

+/// and RTL text. Consider the following string, where the lowercase letters
+/// will be displayed as LTR and the uppercase letters RTL: "helloHELLO".  When
+/// rendered, the string would appear visually as "helloOLLEH".  An offset of 5
+/// would be ambiguous without a corresponding TextAffinity.  Looking at the


(and so on below)

Hixie · 2019-01-06T23:15:32Z

lib/ui/text.dart

 enum TextAffinity {
-  /// The position has affinity for the upstream side of the text position.
+  /// The position has affinity for the upstream side of the text position, or
+  /// in the direction of the beginning of the string.


s/or/i.e./

"or" makes it sound like it could be either, as opposed to it being the same thing

Hixie · 2019-01-06T23:16:10Z

lib/ui/text.dart

  ///
-  /// For example, if the offset of the text position is a line break, the
-  /// position represents the end of the first line.
+  /// In the bidirectional text example above, an offset of 5 with TextAffinity


"above" doesn't make sense here, this text might not be rendered near the class text.

Hixie · 2019-01-06T23:20:53Z

lib/ui/text.dart

+/// A position in a string of text. A TextPosition can be used to locate a
+/// position in a string in code (using the [offset] property), and it can also
+/// be used to locate the same position visually in a rendered string of text
+/// (using [offset] and, when needed to resolve ambiguity, [affinity]).


first paragraph should be short (split it after the first sentence)

justinmc · 2019-01-07T01:20:43Z

@Hixie Thanks for the comments, I'll make these fixes in another PR.

Clarify TextAffinity docs

28b6fce

justinmc self-assigned this Dec 17, 2018

justinmc requested review from Hixie and HansMuller December 17, 2018 23:07

googlebot added the cla: yes label Dec 17, 2018

HansMuller reviewed Dec 19, 2018

View reviewed changes

Clarify TextPosition and the definition of upstream/downstream

13e27c1

HansMuller approved these changes Dec 20, 2018

View reviewed changes

Docs fixes from code review, less redundant with 'string in code'

ae28dfb

justinmc merged commit cc51731 into flutter:master Dec 20, 2018

justinmc deleted the text-affinity-docs branch December 20, 2018 22:14

cbracken reviewed Dec 20, 2018

View reviewed changes

justinmc mentioned this pull request Dec 20, 2018

TextAffinity Docs Improvement #7274

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 21, 2018

cc51731 Clarify TextAffinity docs (flutter/engine#7238)

b6297e1

engine-flutter-autoroll mentioned this pull request Dec 21, 2018

Roll engine 6a9041853c4b..54a3577c0139 (23 commits) flutter/flutter#25649

Merged

Hixie reviewed Jan 6, 2019

View reviewed changes

justinmc mentioned this pull request Jan 7, 2019

Improve TextAffinity Docs #7387

Merged

Clarify TextAffinity docs #7238

Clarify TextAffinity docs #7238

Uh oh!

Conversation

justinmc commented Dec 17, 2018

Uh oh!

justinmc commented Dec 17, 2018

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Dec 19, 2018

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Dec 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Dec 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Jan 7, 2019

Uh oh!

Uh oh!