Last Comment Bug 855732 - getTextBeforeOffset for word boundaries: evolving
: getTextBeforeOffset for word boundaries: evolving
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: alexander :surkov
:
Mentors:
Depends on: 869750
Blocks: 493354
  Show dependency treegraph
 
Reported: 2013-03-28 09:22 PDT by alexander :surkov
Modified: 2013-05-07 18:42 PDT (History)
2 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (42.11 KB, patch)
2013-03-28 10:55 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
for landing (43.09 KB, patch)
2013-03-30 20:17 PDT, alexander :surkov
no flags Details | Diff | Splinter Review

Description alexander :surkov 2013-03-28 09:22:35 PDT
a next round after bug 853340
Comment 1 alexander :surkov 2013-03-28 10:55:04 PDT
Created attachment 730759 [details] [diff] [review]
patch
Comment 2 David Bolter [:davidb] 2013-03-28 12:17:39 PDT
Goodbye kTodo :)
Comment 3 Trevor Saunders (:tbsaunde) 2013-03-29 17:58:59 PDT
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 2013-03-29 20:01:24 PDT
(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) 2013-03-29 20:13:59 PDT
(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 2013-03-29 23:32:57 PDT
(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) 2013-03-30 13:00:30 PDT
(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 2013-03-30 20:17:57 PDT
Created attachment 731606 [details] [diff] [review]
for landing

1) comments were added
2) getTextAtOffset code was reformatted per comment #7
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-04-01 11:01:01 PDT
https://hg.mozilla.org/mozilla-central/rev/7a32d12d1dce

Note You need to log in before you can comment on or make changes to this bug.