Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10592011:
Add BiDirectional Text formatting utilites to the i18n library. (Closed)
|
Created:
8 years, 6 months ago by Emily Fortuna Modified:
8 years, 5 months ago Reviewers:
Alan Knight CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd BiDirectional Text formatting utilites to the i18n library.
Committed: https://code.google.com/p/dart/source/detail?r=9138
Patch Set 1 : #
Total comments: 70
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 8 (0 generated)
This library implements all the methods available in the Python version, and also a subset of the ones available in the Closure BiDi files. Let me know if some methods still seem extraneous or are conspicuously missing. Thanks!
I haven't looked at the tests yet, but in the interests of providing some actual feedback... https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... File lib/i18n/bidi_formatter.dart (right): https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:16: * Go to https://sites.google.com/a/google.com/bidi-howto/ to learn more about This site isn't accessible outside Google. Shouldn't things listed as resources here be publicly accessible? https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:96: * directionality, we wrap it with a `span' tag and adds a `dir' attribute Grammar nitpicking on comments, but "we" and "adds" don't agree. Also the quotes around span and dir have directionality - that is, the leading one is a backquote, which looks odd, at least in this font, not sure if we have a convention for whether to use word-processor style quotes or programming style. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:104: * appended (LRM or RLM). If [isHtml] is false, we HTML-escape the [text]. The comment says that the unicode bidi characters should be used only where markup cannot be used, but we have an option on this to always append one at the end. That seems very odd. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:109: var result = ''; Is there a point initializing this to a value that we will only ever throw away? https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:140: * [isHtml]. What's the point of having a parameter that's ignored? Is there something that expects to call these very symmetrically? I could see maybe if you were calling it reflectively you want the same arguments, but we don't even have that yet. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:143: TextDirection direction]) { I found the name unicodeWrap initially confusing. It sort of sounds like wrapping the string in Unicode, whatever that would mean. It might make it clearer, as well as making the two methods a bit more clearly symmetric if they were something like wrapWithSpan and wrapWithFormatCharacters? Or some other name for the second one, I'm not quite sure what to call those things. But if the current names are consistent with other libraries people might have already used, that might outweigh this. So I'm waffling, basically. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:151: return result.concat(_resetDirIfNeeded(text, direction, isHtml, resetDir)); If we're always ending with PDF, wouldn't do that do the reset automatically? Or maybe I don't understand what PDF does, but if it doesn't reset the direction, what does it do? https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:167: */ Maybe it's just my ignorance of the usage of these things, but it would be nice if the comments gave an indication of what things might be used for. The wrap calls are pretty obvious. This one seems less clear - why do I want the marker for the direction that I'm already going? https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:189: * Returns a unicode BiDi mark matching the context [direction]. When you say "the context" that makes me think you mean something in the environment. Wouldn't it be better here to just say "matching [direction]" ? Except I think that's not really what it means. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:193: * empty string. [isHtml] is true if [text] is HTML or HTML-escaped. I find the name resetDirIfNeeded much clearer than the explanation. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:195: String _resetDirIfNeeded(final String text, TextDirection direction, Why is the text final here, but nowhere else? What do we get by declaring it that way? https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:198: if (resetDir && We've defined the class TextDirection. Couldn't we simplify a bunch of this if we defined methods it that would return the Unicode Bidi indicator characters for the corresponding and the opposite directions, and maybe equality. e.g. if (contextDirection != direction) return contextDirection.unicodeOpposite(); https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:219: bool _requiresDirectionChange(TextDirection direction) { This seems like it could be a method on TextDirection. e.g. contextDirection.requiresChangeTo(direction) https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... File lib/i18n/bidi_utils.dart (right): https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:30: static final UNKNOWN = const TextDirection._('UNKNOWN', 'ltr'); I presume the 'ltr' on UNKNOWN means that we assume left to right in a span if we don't know, but that should probably be spelled out. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:33: final String value; Probably more informative to enumerate the values. e.g. Indicates the direction of the text. One of: 'LTR', 'RTL', 'UNKNOWN'. Do we need to say that it's a String, or would that be pulled out automatically from the definition? https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:37: */ Same as above. Also specifying is spelled wrong. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:100: return const RegExp('^[^$_RTL_CHARS]*[$_LTR_CHARS]').hasMatch( That comes out really nicely. I like the use of string interpolation to re-use partial regexes and make the whole thing comprehensible. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:107: */ I worry a bit with these about people calling many of these types of methods and repeatedly stripping the HTML (and then complaining that it's slow :-) It feels like there might reasonably be an object in amongst all these methods, that if you wanted to do something complicated to a string you would create one and it would remember those bits of state. And maybe it's the formatter object, but it's not clear. And hard to know without understanding more about how these methods would be used in practice. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:178: @'($|-|_)', ignoreCase : true).hasMatch(languageString); Is a regular expression, especially a 3-line one, really the right solution for this? Or wouldn't we be better off using even a simple regex that splits on - or _ to divide it into fields and then have logic based on those field values. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:188: */ How badly do we need these? The other things don't use it, it's trying to parse and modify HTML, which is scary, and comments like "(tested on FF and IE)" sound rather scary. Is that true for this code, or was that copied from the Closure comment? https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:234: html).iterator(); This code seems confusing. Why are we asking for allMatches and only using the first. Shouldn't we ask for firstMatch? https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:257: if (useRtl) { Is a regex really necessary here? Seems like string starts with one of '([{<' and ends with the corresponding close. Maybe the regex is the easiest way to write that, but I don't know that it's the most efficient, and certainly not the clearest. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:258: return _guardBracketHelper(str, regexp, '<span dir=rtl>', '</span>'); probably worth a statement to set a string to 'rtl' or 'ltr' as appropriate and then have only the one call with an interpolated value. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:409: str = str.replaceAll(new RegExp(regex1), '${keys["one"]}\u05f4'); Does this actually work? With a hard-coded "one"? What if there were two occurences? This looks like another one of those things I wouldn't implement with a regex, particularly if I had to implement regex replacement in order to do it. It seems like looping over the string appending to a buffer and looking for a quote where the previous character was Hebrew, then appending one extra, would be at least as simple. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:422: */ Do we really need to do this? Especially for CSS, this seems like it would want to be done on the server, or ahead of time. Telling developers to write two versions of the CSS (or Jacob thinks there's a preprocessor or compiler that would do it for you properly) seems better. And then if we did the previous method without a regex we could also take out the polyfill backreferenceHelper. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:445: static bool detectRtlDirectionality(String str, [bool isHtml]) { Wouldn't this be better called e.g. isRTL?
ptal https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... File lib/i18n/bidi_formatter.dart (right): https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:96: * directionality, we wrap it with a `span' tag and adds a `dir' attribute On 2012/06/22 00:16:53, Alan Knight wrote: > Grammar nitpicking on comments, but "we" and "adds" don't agree. Also the quotes > around span and dir have directionality - that is, the leading one is a > backquote, which looks odd, at least in this font, not sure if we have a > convention for whether to use word-processor style quotes or programming style. The backticks are for marking code sections in markdown: http://daringfireball.net/projects/markdown/syntax#code https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:104: * appended (LRM or RLM). If [isHtml] is false, we HTML-escape the [text]. On 2012/06/22 00:16:53, Alan Knight wrote: > The comment says that the unicode bidi characters should be used only where > markup cannot be used, but we have an option on this to always append one at the > end. That seems very odd. This is what the closure and python libraries do. We can get rid of it, but not knowing all the use cases for this I'm not certain if that would be problematic for applications using our library, particularly if it's expected to behave the same way as in the libraries in other languages. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:109: var result = ''; On 2012/06/22 00:16:53, Alan Knight wrote: > Is there a point initializing this to a value that we will only ever throw away? Result has to be in scope to return it at line 121. It's not in scope if you define it at lines 117 and 119. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:140: * [isHtml]. On 2012/06/22 00:16:53, Alan Knight wrote: > What's the point of having a parameter that's ignored? Is there something that > expects to call these very symmetrically? I could see maybe if you were calling > it reflectively you want the same arguments, but we don't even have that yet. isHtml is needed in the call to estimateDirection, so the parameter is not actually ignored. This replicates the closure library's behavior. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:143: TextDirection direction]) { On 2012/06/22 00:16:53, Alan Knight wrote: > I found the name unicodeWrap initially confusing. It sort of sounds like > wrapping the string in Unicode, whatever that would mean. It might make it > clearer, as well as making the two methods a bit more clearly symmetric if they > were something like wrapWithSpan and wrapWithFormatCharacters? Or some other > name for the second one, I'm not quite sure what to call those things. But if > the current names are consistent with other libraries people might have already > used, that might outweigh this. So I'm waffling, basically. These names were taken directly from closure. I've changed them now, though. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:151: return result.concat(_resetDirIfNeeded(text, direction, isHtml, resetDir)); On 2012/06/22 00:16:53, Alan Knight wrote: > If we're always ending with PDF, wouldn't do that do the reset automatically? Or > maybe I don't understand what PDF does, but if it doesn't reset the direction, > what does it do? It's needed to mark the end of the text that is being set in a particular direction. See http://www.w3.org/TR/html4/struct/dirlang.html#h-8.2.3 I suspect they're explicitly marking the change in direction to be very clear with formatting and not leave layout to chance in the world of many browsers. This replicates the behavior in Closure. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:167: */ On 2012/06/22 00:16:53, Alan Knight wrote: > Maybe it's just my ignorance of the usage of these things, but it would be nice > if the comments gave an indication of what things might be used for. The wrap > calls are pretty obvious. This one seems less clear - why do I want the marker > for the direction that I'm already going? This one, like the others, I took from the library this is based off of. We can certainly take it out. I was erring on the side of providing functions that I was less sure about the usage of since I don't have a lot of experience with writing products that regularly use bidi myself. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:189: * Returns a unicode BiDi mark matching the context [direction]. On 2012/06/22 00:16:53, Alan Knight wrote: > When you say "the context" that makes me think you mean something in the > environment. Wouldn't it be better here to just say "matching [direction]" ? > Except I think that's not really what it means. rephrased. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:193: * empty string. [isHtml] is true if [text] is HTML or HTML-escaped. On 2012/06/22 00:16:53, Alan Knight wrote: > I find the name resetDirIfNeeded much clearer than the explanation. Good thing I changed the name differently from the original closure version then. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:195: String _resetDirIfNeeded(final String text, TextDirection direction, On 2012/06/22 00:16:53, Alan Knight wrote: > Why is the text final here, but nowhere else? What do we get by declaring it > that way? That was for an optimization that is no longer needed. removed. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:198: if (resetDir && On 2012/06/22 00:16:53, Alan Knight wrote: > We've defined the class TextDirection. Couldn't we simplify a bunch of this if > we defined methods it that would return the Unicode Bidi indicator characters > for the corresponding and the opposite directions, and maybe equality. e.g. > if (contextDirection != direction) return contextDirection.unicodeOpposite(); This function requires knowledge of the last strongly directional text in "text", so although we could subsitute isDirectionChange in for the contextDirection and direction checks, we'd still have to add back in the one check with context Direction to make sure that the context is LTR but the text ends with RTL. Adding unicode characters to the TextDirection class feels like we're muddying the distinctions between the classes for a class that's just represents text direction, not expose characters to help you do formatting. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:219: bool _requiresDirectionChange(TextDirection direction) { On 2012/06/22 00:16:53, Alan Knight wrote: > This seems like it could be a method on TextDirection. e.g. > contextDirection.requiresChangeTo(direction) Done. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... File lib/i18n/bidi_utils.dart (right): https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:30: static final UNKNOWN = const TextDirection._('UNKNOWN', 'ltr'); On 2012/06/22 00:16:53, Alan Knight wrote: > I presume the 'ltr' on UNKNOWN means that we assume left to right in a span if > we don't know, but that should probably be spelled out. Done. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:33: final String value; On 2012/06/22 00:16:53, Alan Knight wrote: > Probably more informative to enumerate the values. e.g. Indicates the direction > of the text. One of: 'LTR', 'RTL', 'UNKNOWN'. Do we need to say that it's a > String, or would that be pulled out automatically from the definition? Done. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:37: */ On 2012/06/22 00:16:53, Alan Knight wrote: > Same as above. Also specifying is spelled wrong. Done. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:100: return const RegExp('^[^$_RTL_CHARS]*[$_LTR_CHARS]').hasMatch( On 2012/06/22 00:16:53, Alan Knight wrote: > That comes out really nicely. I like the use of string interpolation to re-use > partial regexes and make the whole thing comprehensible. Done. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:107: */ On 2012/06/22 00:16:53, Alan Knight wrote: > I worry a bit with these about people calling many of these types of methods and > repeatedly stripping the HTML (and then complaining that it's slow :-) It feels > like there might reasonably be an object in amongst all these methods, that if > you wanted to do something complicated to a string you would create one and it > would remember those bits of state. And maybe it's the formatter object, but > it's not clear. And hard to know without understanding more about how these > methods would be used in practice. As mentioned at the top of this file, bidi_formatter should be the primary location for users to access to format text. The methods here are mainly to provide supplemental information if you're doing something odd with the layout. They shouldn't be called frequently. I started re-writing these methods so that they were no longer static, and you could construct a BidiUtil object for a particular string, but the frequency that these are used combined with the then unusual way to determine properties about strings for these methods versus the rest, it seemed more trouble than it's worth. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:178: @'($|-|_)', ignoreCase : true).hasMatch(languageString); On 2012/06/22 00:16:53, Alan Knight wrote: > Is a regular expression, especially a 3-line one, really the right solution for > this? Or wouldn't we be better off using even a simple regex that splits on - or > _ to divide it into fields and then have logic based on those field values. The language code specification system is fairly complex: http://www.rfc-editor.org/rfc/bcp/bcp47.txt There is an order that the codes can be provided in, but specific codes for specific languages can be omitted because they're the default AND the different subtags can have different lengths, so it's not easy to determine what subtag distinguishes what without parsing the entire string, and therefore making this function vastly more complicated. We can revisit this if we start needing to do fancy things with language codes and have a language code class, but for now, it seems to be more trouble than it's worth. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:188: */ On 2012/06/22 00:16:53, Alan Knight wrote: > How badly do we need these? The other things don't use it, it's trying to parse > and modify HTML, which is scary, and comments like "(tested on FF and IE)" sound > rather scary. Is that true for this code, or was that copied from the Closure > comment? Since this is being developed not on a Windows machine, I can't test it on IE, but since I'm replicating the closure behavior here exactly, I preserved the comment intact from the closure library. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:234: html).iterator(); On 2012/06/22 00:16:53, Alan Knight wrote: > This code seems confusing. Why are we asking for allMatches and only using the > first. Shouldn't we ask for firstMatch? Fixed. That was from a refactor that apparently didn't get fully completed. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:257: if (useRtl) { On 2012/06/22 00:16:53, Alan Knight wrote: > Is a regex really necessary here? Seems like string starts with one of '([{<' > and ends with the corresponding close. Maybe the regex is the easiest way to > write that, but I don't know that it's the most efficient, and certainly not the > clearest. Renamed to make clearer? https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:258: return _guardBracketHelper(str, regexp, '<span dir=rtl>', '</span>'); On 2012/06/22 00:16:53, Alan Knight wrote: > probably worth a statement to set a string to 'rtl' or 'ltr' as appropriate and > then have only the one call with an interpolated value. Done. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:409: str = str.replaceAll(new RegExp(regex1), '${keys["one"]}\u05f4'); On 2012/06/22 00:16:53, Alan Knight wrote: > Does this actually work? With a hard-coded "one"? What if there were two > occurences? > > This looks like another one of those things I wouldn't implement with a regex, > particularly if I had to implement regex replacement in order to do it. It seems > like looping over the string appending to a buffer and looking for a quote where > the previous character was Hebrew, then appending one extra, would be at least > as simple. Removed regular expression version. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:422: */ On 2012/06/22 00:16:53, Alan Knight wrote: > Do we really need to do this? Especially for CSS, this seems like it would want > to be done on the server, or ahead of time. Telling developers to write two > versions of the CSS (or Jacob thinks there's a preprocessor or compiler that > would do it for you properly) seems better. And then if we did the previous > method without a regex we could also take out the polyfill backreferenceHelper. Done. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:445: static bool detectRtlDirectionality(String str, [bool isHtml]) { On 2012/06/22 00:16:53, Alan Knight wrote: > Wouldn't this be better called e.g. isRTL? isRTL is reserved for knowing for certain that the direction is RTL. This is an estimation. isRTL can be found in bidi_formatter.dart
Also looked at the tests, which look good. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... File lib/i18n/bidi_formatter.dart (right): https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:16: * Go to https://sites.google.com/a/google.com/bidi-howto/ to learn more about On 2012/06/22 00:16:53, Alan Knight wrote: > This site isn't accessible outside Google. Shouldn't things listed as resources > here be publicly accessible? Do we do anything about this? https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:96: * directionality, we wrap it with a `span' tag and adds a `dir' attribute On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > Grammar nitpicking on comments, but "we" and "adds" don't agree. Also the > quotes > > around span and dir have directionality - that is, the leading one is a > > backquote, which looks odd, at least in this font, not sure if we have a > > convention for whether to use word-processor style quotes or programming > style. > > The backticks are for marking code sections in markdown: > http://daringfireball.net/projects/markdown/syntax#code Ah, ok. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:104: * appended (LRM or RLM). If [isHtml] is false, we HTML-escape the [text]. On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > The comment says that the unicode bidi characters should be used only where > > markup cannot be used, but we have an option on this to always append one at > the > > end. That seems very odd. > > This is what the closure and python libraries do. We can get rid of it, but not > knowing all the use cases for this I'm not certain if that would be problematic > for applications using our library, particularly if it's expected to behave the > same way as in the libraries in other languages. OK. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:109: var result = ''; On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > Is there a point initializing this to a value that we will only ever throw > away? > > Result has to be in scope to return it at line 121. It's not in scope if you > define it at lines 117 and 119. Yes, it needs to be declared there, but there seems little point in initializing it rather than leaving it null. There's no scenario where that initial value can escape, or where it would be a valid return value. Not exactly a major point, though. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:140: * [isHtml]. On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > What's the point of having a parameter that's ignored? Is there something that > > expects to call these very symmetrically? I could see maybe if you were > calling > > it reflectively you want the same arguments, but we don't even have that yet. > > isHtml is needed in the call to estimateDirection, so the parameter is not > actually ignored. This replicates the closure library's behavior. But it sounds like this is only applicable to plain text, so the value of isHtml can be replaced with a literal false (or left out) on the call to estimateDirection. All it does there is control whether the input is stripped of html, which should never apply if called from here. Unless there's a use case in which you put in HTML and return plain text, which seems very odd. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:143: TextDirection direction]) { On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > I found the name unicodeWrap initially confusing. It sort of sounds like > > wrapping the string in Unicode, whatever that would mean. It might make it > > clearer, as well as making the two methods a bit more clearly symmetric if > they > > were something like wrapWithSpan and wrapWithFormatCharacters? Or some other > > name for the second one, I'm not quite sure what to call those things. But if > > the current names are consistent with other libraries people might have > already > > used, that might outweigh this. So I'm waffling, basically. > > These names were taken directly from closure. I've changed them now, though. OK. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:151: return result.concat(_resetDirIfNeeded(text, direction, isHtml, resetDir)); On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > If we're always ending with PDF, wouldn't do that do the reset automatically? > Or > > maybe I don't understand what PDF does, but if it doesn't reset the direction, > > what does it do? > > It's needed to mark the end of the text that is being set in a particular > direction. See http://www.w3.org/TR/html4/struct/dirlang.html#h-8.2.3 I suspect > they're explicitly marking the change in direction to be very clear with > formatting and not leave layout to chance in the world of many browsers. This > replicates the behavior in Closure. OK, I think I understand that. It's ending the text, and the reset is adding another explicit override to a particular direction. Seems very messy, but that's probably part of why they recommend the HTML form. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:167: */ On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > Maybe it's just my ignorance of the usage of these things, but it would be > nice > > if the comments gave an indication of what things might be used for. The wrap > > calls are pretty obvious. This one seems less clear - why do I want the marker > > for the direction that I'm already going? > > This one, like the others, I took from the library this is based off of. We can > certainly take it out. I was erring on the side of providing functions that I > was less sure about the usage of since I don't have a lot of experience with > writing products that regularly use bidi myself. Yeah, I worry about us just reproducing a bunch of messy stuff that we don't understand. I think I'm inclined to err on the side of leaving things out and letting people complain about their absence, at which point we should be able to get a use case for why they want it. The triple value logic is particularly odd. It doesn't actually return the Unicode Bidi mark, it returns either that or an empty string. This seems to be treating a null context direction as meaning UNKNOWN, even though we have a representation for that and probably ought to initialize it that way rather than having the possibility for it to be null. And if we had the methods on TextDirection to return the markers, then this becomes close to a no-op. return contextDirection.unicodeMark; One more line if we want to handle the null case. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:198: if (resetDir && On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > We've defined the class TextDirection. Couldn't we simplify a bunch of this if > > we defined methods it that would return the Unicode Bidi indicator characters > > for the corresponding and the opposite directions, and maybe equality. e.g. > > if (contextDirection != direction) return > contextDirection.unicodeOpposite(); > > This function requires knowledge of the last strongly directional text in > "text", so although we could subsitute isDirectionChange in for the > contextDirection and direction checks, we'd still have to add back in the one > check with context Direction to make sure that the context is LTR but the text > ends with RTL. Adding unicode characters to the TextDirection class feels like > we're muddying the distinctions between the classes for a class that's just > represents text direction, not expose characters to help you do formatting. So we can go from an 8 clause if statement to 3 (and 2 if we also factor out the obvious guard clause) if (!resetDir) return ''; if ((contextDirection != textDirection) || (contextDirection != endDirection(text,isHTML)) return contextDirection.unicodeOpposite(); } else { return contextDirection.unicodeMarker(); } That seems like a significant improvement. As far as putting unicode characters onto text direction, it already has HTML constants, so that doesn't seem like a stretch. Or you could store the constants elsewhere and just use functions to ask for unicodeOpposite(contextDirection) though I think I'd rather have them accessible from the object. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... File lib/i18n/bidi_utils.dart (right): https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:107: */ On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > I worry a bit with these about people calling many of these types of methods > and > > repeatedly stripping the HTML (and then complaining that it's slow :-) It > feels > > like there might reasonably be an object in amongst all these methods, that if > > you wanted to do something complicated to a string you would create one and it > > would remember those bits of state. And maybe it's the formatter object, but > > it's not clear. And hard to know without understanding more about how these > > methods would be used in practice. > > As mentioned at the top of this file, bidi_formatter should be the primary > location for users to access to format text. The methods here are mainly to > provide supplemental information if you're doing something odd with the layout. > They shouldn't be called frequently. I started re-writing these methods so that > they were no longer static, and you could construct a BidiUtil object for a > particular string, but the frequency that these are used combined with the then > unusual way to determine properties about strings for these methods versus the > rest, it seemed more trouble than it's worth. OK https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:178: @'($|-|_)', ignoreCase : true).hasMatch(languageString); On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > Is a regular expression, especially a 3-line one, really the right solution > for > > this? Or wouldn't we be better off using even a simple regex that splits on - > or > > _ to divide it into fields and then have logic based on those field values. > > The language code specification system is fairly complex: > http://www.rfc-editor.org/rfc/bcp/bcp47.txt There is an order that the codes can > be provided in, but specific codes for specific languages can be omitted because > they're the default AND the different subtags can have different lengths, so > it's not easy to determine what subtag distinguishes what without parsing the > entire string, and therefore making this function vastly more complicated. We > can revisit this if we start needing to do fancy things with language codes and > have a language code class, but for now, it seems to be more trouble than it's > worth. Grumble, grumble, ok. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:188: */ On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > How badly do we need these? The other things don't use it, it's trying to > parse > > and modify HTML, which is scary, and comments like "(tested on FF and IE)" > sound > > rather scary. Is that true for this code, or was that copied from the Closure > > comment? > > Since this is being developed not on a Windows machine, I can't test it on IE, > but since I'm replicating the closure behavior here exactly, I preserved the > comment intact from the closure library. Maybe we should change it to something like "should work on FF and IE". And maybe we should mention Chrome. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:257: if (useRtl) { On 2012/06/25 20:25:35, Emily Fortuna wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > Is a regex really necessary here? Seems like string starts with one of '([{<' > > and ends with the corresponding close. Maybe the regex is the easiest way to > > write that, but I don't know that it's the most efficient, and certainly not > the > > clearest. > > Renamed to make clearer? Not a fan of all these regexes, but it'll do.
https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... File lib/i18n/bidi_formatter.dart (right): https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:16: * Go to https://sites.google.com/a/google.com/bidi-howto/ to learn more about On 2012/06/25 23:29:56, Alan Knight wrote: > On 2012/06/22 00:16:53, Alan Knight wrote: > > This site isn't accessible outside Google. Shouldn't things listed as > resources > > here be publicly accessible? > > Do we do anything about this? You should look at the new version of these files that I uploaded with my last email. I just took the url out. The URL is technically available to anyone on the internet, they just don't all have permission to view it (you get a google docs error). https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:109: var result = ''; On 2012/06/25 23:29:56, Alan Knight wrote: > On 2012/06/25 20:25:35, Emily Fortuna wrote: > > On 2012/06/22 00:16:53, Alan Knight wrote: > > > Is there a point initializing this to a value that we will only ever throw > > away? > > > > Result has to be in scope to return it at line 121. It's not in scope if you > > define it at lines 117 and 119. > > Yes, it needs to be declared there, but there seems little point in initializing > it rather than leaving it null. There's no scenario where that initial value can > escape, or where it would be a valid return value. Not exactly a major point, > though. Done. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:140: * [isHtml]. On 2012/06/25 23:29:56, Alan Knight wrote: > On 2012/06/25 20:25:35, Emily Fortuna wrote: > > On 2012/06/22 00:16:53, Alan Knight wrote: > > > What's the point of having a parameter that's ignored? Is there something > that > > > expects to call these very symmetrically? I could see maybe if you were > > calling > > > it reflectively you want the same arguments, but we don't even have that > yet. > > > > isHtml is needed in the call to estimateDirection, so the parameter is not > > actually ignored. This replicates the closure library's behavior. > > But it sounds like this is only applicable to plain text, so the value of isHtml > can be replaced with a literal false (or left out) on the call to > estimateDirection. All it does there is control whether the input is stripped of > html, which should never apply if called from here. Unless there's a use case in > which you put in HTML and return plain text, which seems very odd. There's the (admittedly) obscure case the comments mention in lines 137-138 on this particular version of the file with option tags, but we could take that out and see if people complained about it missing if you'd like...? https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:167: */ On 2012/06/25 23:29:56, Alan Knight wrote: > On 2012/06/25 20:25:35, Emily Fortuna wrote: > > On 2012/06/22 00:16:53, Alan Knight wrote: > > > Maybe it's just my ignorance of the usage of these things, but it would be > > nice > > > if the comments gave an indication of what things might be used for. The > wrap > > > calls are pretty obvious. This one seems less clear - why do I want the > marker > > > for the direction that I'm already going? > > > > This one, like the others, I took from the library this is based off of. We > can > > certainly take it out. I was erring on the side of providing functions that I > > was less sure about the usage of since I don't have a lot of experience with > > writing products that regularly use bidi myself. > > Yeah, I worry about us just reproducing a bunch of messy stuff that we don't > understand. I think I'm inclined to err on the side of leaving things out and > letting people complain about their absence, at which point we should be able to > get a use case for why they want it. > > The triple value logic is particularly odd. It doesn't actually return the > Unicode Bidi mark, it returns either that or an empty string. This seems to be > treating a null context direction as meaning UNKNOWN, even though we have a > representation for that and probably ought to initialize it that way rather than > having the possibility for it to be null. > > And if we had the methods on TextDirection to return the markers, then this > becomes close to a no-op. return contextDirection.unicodeMark; One more line > if we want to handle the null case. *Please* look at the updated version of the file I uploaded when I sent out the email again. I already deleted these methods. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_forma... lib/i18n/bidi_formatter.dart:198: if (resetDir && On 2012/06/25 23:29:56, Alan Knight wrote: > On 2012/06/25 20:25:35, Emily Fortuna wrote: > > On 2012/06/22 00:16:53, Alan Knight wrote: > > > We've defined the class TextDirection. Couldn't we simplify a bunch of this > if > > > we defined methods it that would return the Unicode Bidi indicator > characters > > > for the corresponding and the opposite directions, and maybe equality. e.g. > > > if (contextDirection != direction) return > > contextDirection.unicodeOpposite(); > > > > This function requires knowledge of the last strongly directional text in > > "text", so although we could subsitute isDirectionChange in for the > > contextDirection and direction checks, we'd still have to add back in the one > > check with context Direction to make sure that the context is LTR but the text > > ends with RTL. Adding unicode characters to the TextDirection class feels like > > we're muddying the distinctions between the classes for a class that's just > > represents text direction, not expose characters to help you do formatting. > > So we can go from an 8 clause if statement to 3 (and 2 if we also factor out the > obvious guard clause) > > if (!resetDir) return ''; > if ((contextDirection != textDirection) || > (contextDirection != endDirection(text,isHTML)) > return contextDirection.unicodeOpposite(); > } else { > return contextDirection.unicodeMarker(); > } > > That seems like a significant improvement. As far as putting unicode characters > onto text direction, it already has HTML constants, so that doesn't seem like a > stretch. Or you could store the constants elsewhere and just use functions to > ask for > unicodeOpposite(contextDirection) > though I think I'd rather have them accessible from the object. You can't get rewrite this and preserve the "short-circuit" properties that this expression has, which you had mentioned is good because we don't want to be calling both endsWithRtl AND endsWithLtr. Rewriting it would look like: if (contextDirection.isOpposite(direction) && contextDirection.isOpposite( BidiUtils.endDirection(text, isHtml, contextDirection))) { contextDirection.unicodeMarker(); } And then the endDirection method implemented would be like: static TextDirection endDirection(String text, [isHtml=false, expectedDirection=TextDirection.UNKNOWN]) { if (expectedDirection == TextDirection.LTR) { if(endsWithRtl(text, isHtml) {...} } else if(etc...) } In which case we're rearranging the deck chairs on the Titanic -- we're still doing the same comparison of explicit LTR and RTL, just in a different location in the file. So either, we don't have the short circuit property, which might be slow, but it looks clean or we leave it as is and it's faster but is a little complex in this if statement. https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... File lib/i18n/bidi_utils.dart (right): https://chromiumcodereview.appspot.com/10592011/diff/2001/lib/i18n/bidi_utils... lib/i18n/bidi_utils.dart:188: */ On 2012/06/25 23:29:56, Alan Knight wrote: > On 2012/06/25 20:25:35, Emily Fortuna wrote: > > On 2012/06/22 00:16:53, Alan Knight wrote: > > > How badly do we need these? The other things don't use it, it's trying to > > parse > > > and modify HTML, which is scary, and comments like "(tested on FF and IE)" > > sound > > > rather scary. Is that true for this code, or was that copied from the > Closure > > > comment? > > > > Since this is being developed not on a Windows machine, I can't test it on IE, > > but since I'm replicating the closure behavior here exactly, I preserved the > > comment intact from the closure library. > > Maybe we should change it to something like "should work on FF and IE". And > maybe we should mention Chrome. Done.
check to see if it's ready to go in, please.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
