855732 - getTextBeforeOffset for word boundaries: evolving

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

a next round after bug 853340

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Goodbye kTodo :)

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 730759 [details] [diff] [review]
patch

>+    case BOUNDARY_WORD_START: {
>+      if (offset == 0) {
>+        *aStartOffset = *aEndOffset = 0;
>+        return NS_OK;
>+      }
>+
>+      int32_t midOffset = FindWordBoundary(offset, eDirPrevious, eStartWord);
>+      *aEndOffset = FindWordBoundary(midOffset, eDirNext, eStartWord);
>+      if (*aEndOffset == offset) {
>+        *aStartOffset = midOffset;
>+        return GetText(*aStartOffset, *aEndOffset, aText);
>+      }
>+
>+      *aStartOffset = FindWordBoundary(midOffset, eDirPrevious, eStartWord);
>+      *aEndOffset = midOffset;
>+      return GetText(*aStartOffset, *aEndOffset, aText);

this all could really use comments explaining cases and such.

>+    }
>+
>+    case BOUNDARY_WORD_END: {
>+      if (offset == 0) {
>+        *aStartOffset = *aEndOffset = 0;
>+        return NS_OK;
>+      }
>+
>+      *aEndOffset = FindWordBoundary(offset, eDirPrevious, eEndWord);
>+      *aStartOffset = FindWordBoundary(*aEndOffset, eDirPrevious, eEndWord);
>+      return GetText(*aStartOffset, *aEndOffset, aText);

same, the offset == 0 part is especially suprising here

>+                            (moveForwardThenBack ? eDirNext : eDirPrevious),
>+                            wordMovementType);
>+  if (moveForwardThenBack)
>     *aEndOffset = offset;
>   else
>     *aStartOffset = offset;

maybe it would be nice to have type to return that is tupple start end text?

what is the story with the test regressions?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> >+    case BOUNDARY_WORD_END: {
> >+      if (offset == 0) {
> >+        *aStartOffset = *aEndOffset = 0;
> >+        return NS_OK;
> >+      }
> >+
> >+      *aEndOffset = FindWordBoundary(offset, eDirPrevious, eEndWord);
> >+      *aStartOffset = FindWordBoundary(*aEndOffset, eDirPrevious, eEndWord);
> >+      return GetText(*aStartOffset, *aEndOffset, aText);
> 
> same, the offset == 0 part is especially suprising here

"The returned string will contain the word before the offset" so empty string. An alternative is failure. I will add comments.

> >+                            (moveForwardThenBack ? eDirNext : eDirPrevious),
> >+                            wordMovementType);
> >+  if (moveForwardThenBack)
> >     *aEndOffset = offset;
> >   else
> >     *aStartOffset = offset;
> 
> maybe it would be nice to have type to return that is tupple start end text?

you mean

struct TextRange {
  int startOffset;
  int endOffset;
  nsAString& name;
}

?

I could separate these on separate 'case's too as I did in getTextBeforeOffset if it make code nicer (I professedly leaved two different approaches)

> what is the story with the test regressions?

which regressions? This patch fixes exiting regressions but it doesn't seem introduce new ones.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> 
> > >+    case BOUNDARY_WORD_END: {
> > >+      if (offset == 0) {
> > >+        *aStartOffset = *aEndOffset = 0;
> > >+        return NS_OK;
> > >+      }
> > >+
> > >+      *aEndOffset = FindWordBoundary(offset, eDirPrevious, eEndWord);
> > >+      *aStartOffset = FindWordBoundary(*aEndOffset, eDirPrevious, eEndWord);
> > >+      return GetText(*aStartOffset, *aEndOffset, aText);
> > 
> > same, the offset == 0 part is especially suprising here
> 
> "The returned string will contain the word before the offset" so empty
> string. An alternative is failure. I will add comments.

yeah, I figured it out eventually it just wasn't anything like obvious from just reading.

> > >+                            (moveForwardThenBack ? eDirNext : eDirPrevious),
> > >+                            wordMovementType);
> > >+  if (moveForwardThenBack)
> > >     *aEndOffset = offset;
> > >   else
> > >     *aStartOffset = offset;
> > 
> > maybe it would be nice to have type to return that is tupple start end text?
> 
> you mean
> 
> struct TextRange {
>   int startOffset;
>   int endOffset;
>   nsAString& name;
> }
> 
> ?

that's more or less what I was thinking of

> I could separate these on separate 'case's too as I did in
> getTextBeforeOffset if it make code nicer (I professedly leaved two
> different approaches)

not sure what you mean off hand

> > what is the story with the test regressions?
> 
> which regressions? This patch fixes exiting regressions but it doesn't seem
> introduce new ones.

hm ok, maybe its just the diff being weird

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> > struct TextRange {
> >   int startOffset;
> >   int endOffset;
> >   nsAString& name;
> > }
> > 
> > ?
> 
> that's more or less what I was thinking of

maybe as followup, as dexpcomination part

> > I could separate these on separate 'case's too as I did in
> > getTextBeforeOffset if it make code nicer (I professedly leaved two
> > different approaches)
> 
> not sure what you mean off hand

compare

getTextBeforeOffset
case word_start:
  // logic
  break;
case word_end:
  // logic;
  break;

getTextAtOffset
case word_start:
  // variables setting
  break;
case word_end
  // variables setting
  break;

// shared logic for both word start and end

which one do you prefer?

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > > struct TextRange {
> > >   int startOffset;
> > >   int endOffset;
> > >   nsAString& name;
> > > }
> > > 
> > > ?
> > 
> > that's more or less what I was thinking of
> 
> maybe as followup, as dexpcomination part

sure

> > > I could separate these on separate 'case's too as I did in
> > > getTextBeforeOffset if it make code nicer (I professedly leaved two
> > > different approaches)
> > 
> > not sure what you mean off hand
> 
> compare
> 
> getTextBeforeOffset
> case word_start:
>   // logic
>   break;
> case word_end:
>   // logic;
>   break;
> 
> getTextAtOffset
> case word_start:
>   // variables setting
>   break;
> case word_end
>   // variables setting
>   break;
> 
> // shared logic for both word start and end
> 
> which one do you prefer?

probably the first, but only slightly and it depends on the exact case.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: for landing

1) comments were added
2) getTextAtOffset code was reformatted per comment #7

Comment 9

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a32d12d1dce

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7a32d12d1dce