The default bug view has changed. See this FAQ.

getTextBeforeOffset for word boundaries: evolving

RESOLVED FIXED in mozilla22

Status

()

Core
Disability Access APIs
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({access})

unspecified
mozilla22
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
a next round after bug 853340
(Assignee)

Comment 1

4 years ago
Created attachment 730759 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #730759 - Flags: review?(trev.saunders)
Goodbye kTodo :)
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?
(Assignee)

Comment 4

4 years ago
(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.
(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
(Assignee)

Comment 6

4 years ago
(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?
(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.
Attachment #730759 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 731606 [details] [diff] [review]
for landing

1) comments were added
2) getTextAtOffset code was reformatted per comment #7
Attachment #730759 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a32d12d1dce
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/7a32d12d1dce
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(Assignee)

Updated

4 years ago
Blocks: 493354
No longer blocks: 352340
Depends on: 869750
You need to log in before you can comment on or make changes to this bug.