329031 - Clean up home/end/ctrl+home/ctrl+end offset calculation

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Eli Friedman]

Patch coming up.  I did all the tests could think of; no regressions are caused by this, as far as I can tell.

Tell me if you don't understand anything, or you think I should refactor things differently.

Comment 1

[Eli Friedman]

Attachment: Patch

The new code is conceptually the same as the old code, but it's written to use helpers from my mouse offset work.

Comment 2

[Uri Bernstein]

Comment on attachment 213664 [details] [diff] [review]
Patch

Yes, this looks good and includes some cleanup which I really should have done in some of my previous patches. Just a few comments / questions:

To me, it would be more intuitive if AdjustFrameForSelectionStyles() would return the original frame (instead of null) if no adjustments were made. You could then use |if (adjustedFrame != this)| if you need to know whether any adjustment happened.

>+      PRBool endOfFrame = (eSelectBeginLine != aPos->mAmount);

wouldn't this more simply be written as...?
>+      PRBool endOfFrame = (eSelectEndLine == aPos->mAmount);

Also, I think endOfFrame could be better named. perhaps "endOfLine" or even "findEndOfLine"?

>-          // force the offset to the logical beginning (for HOME) or end (for END) of the frame
>-          // (if it is an RTL frame it will be at the visual beginning or end, which we don't want in this case)
This is something that I could have removed back when I fixed bug 16311, it doesn't depend on the other parts of this patch, right?

These are all to your (and roc's) consideration. r+ anyway from me.

Comment 3

[Eli Friedman]

(In reply to comment #2)
> (From update of attachment 213664 [details] [diff] [review] [edit])
> Yes, this looks good and includes some cleanup which I really should have done
> in some of my previous patches. Just a few comments / questions:
> 
> To me, it would be more intuitive if AdjustFrameForSelectionStyles() would
> return the original frame (instead of null) if no adjustments were made. You
> could then use |if (adjustedFrame != this)| if you need to know whether any
> adjustment happened.

Agreed; done.

> >+      PRBool endOfFrame = (eSelectBeginLine != aPos->mAmount);
> 
> wouldn't this more simply be written as...?
> >+      PRBool endOfFrame = (eSelectEndLine == aPos->mAmount);
> 
> Also, I think endOfFrame could be better named. perhaps "endOfLine" or even
> "findEndOfLine"?

Yeah.  One of those code evolution things; the name made more sense when I first wrote it.

> >-          // force the offset to the logical beginning (for HOME) or end (for END) of the frame
> >-          // (if it is an RTL frame it will be at the visual beginning or end, which we don't want in this case)
> This is something that I could have removed back when I fixed bug 16311, it
> doesn't depend on the other parts of this patch, right?

Pretty sure so; I guess the underlying bug got fixed since it was first written.

> These are all to your (and roc's) consideration. r+ anyway from me.

Thanks.

New patch coming up.

Comment 4

[Eli Friedman]

Attachment: Patch v2

Comment 5

[Eli Friedman]

roc, when can I expect an sr?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 214378 [details] [diff] [review]
Patch v2

sorry about the delay!

Comment 7

[Eli Friedman]

Checked in.