Bug 25494 – Add more Position constructor functions and make them all inline

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Eric Seidel (no email)]

add PositionConstructors.h in preparation for adding more position constructor functions

It seems no matter what I do, the Position constructors remain confusing.  So I think we'll likely move away from them in the future and move towards constructor functions.

[Eric Seidel (no email)]

Created attachment 29923 [details]
Add PositionConstructors.h

 26 files changed, 142 insertions(+), 51 deletions(-)

[Eric Seidel (no email)]

My next step will be to rename positionBeforeNode to positionInParentBeforeNode and add a positionBeforeNode which returns a neighbor anchored position (which is more efficient to construct and sufficient in many cases... and better in some others!)

[Eric Seidel (no email)]

Created attachment 29924 [details]
Add PositionConstructors.h

 26 files changed, 148 insertions(+), 51 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 29923 [details]
Add PositionConstructors.h

Uploaded one with a slightly nicer changelog.

[Eric Seidel (no email)]

Created attachment 29926 [details]
Rename positionBeforeNode to positionInParentBeforeNode

 15 files changed, 90 insertions(+), 39 deletions(-)

[Eric Seidel (no email)]

Created attachment 29927 [details]
Add more position constructors

 6 files changed, 64 insertions(+), 8 deletions(-)

[Maciej Stachowiak]

Comment on attachment 29924 [details]
Add PositionConstructors.h

I don't think calling this header PositionConstructors.h is right. "Constructor" means a specific thing in C++, and these aren't it, so it's kind of confusing.

I actually think it's fine for these functions to be in Position.h, since they are effectively part of the interface to Position, but if you can think of a good alternate header name, that's ok too.

Also, while I'm nitpicking, I don't think the names of functions that take nodes necessarily need Node in them.It could just be:

static inline Position positionBefore(const Node* node)
static inline Position positionAfter(const Node* node)
static inline Position firstDeepEditingPosition(Node* anchorNode)
static inline Position lastDeepEditingPosition(Node* anchorNode)

r- for the header name. Function renames are optional.

[Maciej Stachowiak]

Comment on attachment 29926 [details]
Rename positionBeforeNode to positionInParentBeforeNode

I repeat my earlier comments about removing "Node" from the names of these functions. Otherwise looks ok, but this depends on the previous patch which I r-'d, so r- for this too.

[Maciej Stachowiak]

Comment on attachment 29927 [details]
Add more position constructors

r- because this depends on the previous r-'d patches. Also: I would request that you not add unused functions. Please add them with at least one real use that has test coverage, so we don't have untested dead code in the tree.

[Maciej Stachowiak]

Comment on attachment 29924 [details]
Add PositionConstructors.h

I forgot to mention before. Functions in headers should just be "inline", not "static inline". This also applies to the other two patches.

[Eric Seidel (no email)]

(In reply to comment #7)
> (From update of attachment 29924 [details] [review])
> I don't think calling this header PositionConstructors.h is right.
> "Constructor" means a specific thing in C++, and these aren't it, so it's kind
> of confusing.
> 
> I actually think it's fine for these functions to be in Position.h, since they
> are effectively part of the interface to Position, but if you can think of a
> good alternate header name, that's ok too.

I think a separate header is better for keeping more classes out of Position.h

[Eric Seidel (no email)]

(In reply to comment #9)
> (From update of attachment 29927 [details] [review])
> r- because this depends on the previous r-'d patches. Also: I would request
> that you not add unused functions. Please add them with at least one real use
> that has test coverage, so we don't have untested dead code in the tree.

These get used in later patches in this series.  But I've been trying to keep the patches small.

[Eric Seidel (no email)]

(In reply to comment #8)
> (From update of attachment 29926 [details] [review])
> I repeat my earlier comments about removing "Node" from the names of these
> functions. Otherwise looks ok, but this depends on the previous patch which I
> r-'d, so r- for this too.

These get renamed in the next patch to positionInParentBeforeNode.  I could rename that to positionBeforeInParent, but I think that's more confusion.

[Eric Seidel (no email)]

Ok, uploading a new first patch only changing the header name from PositionContructors to PositionCreationFunctions.h

No other changes to the patches (except making them apply to TOT).

[Eric Seidel (no email)]

Created attachment 30846 [details]
Add PositionCreationFunctions.h

 26 files changed, 148 insertions(+), 51 deletions(-)

[Eric Seidel (no email)]

Created attachment 30847 [details]
Rename positionBeforeNode to positionInParentBeforeNode

 15 files changed, 90 insertions(+), 39 deletions(-)

[Eric Seidel (no email)]

Created attachment 30848 [details]
Add more position constructors

 6 files changed, 64 insertions(+), 8 deletions(-)

[Eric Seidel (no email)]

Oh, I remember now why I split these out of Position.h... so that Position.h didn't have to include htmlediting.h for lastOffsetForEditing.  Eventually lastOffsetForEditing will probably move into some Position file.

Maybe I should just move first/lastDeepEditingPositionForNode into htmlediting.h and leave the rest in Position.h.

It sounds like both Justin and Maciej think these patches are just fine... I just need an r+ from someone. ;)

[Maciej Stachowiak]

Comment on attachment 30846 [details]
Add PositionCreationFunctions.h

r=me but please apply your recently suggested plan to keep most of this in Position.h itself.

[Maciej Stachowiak]

Comment on attachment 30847 [details]
Rename positionBeforeNode to positionInParentBeforeNode

This still has "static inline" functions in headers, which I am pretty sure is wrong. I think you missed this comment before.

[Maciej Stachowiak]

Comment on attachment 30848 [details]
Add more position constructors

Once again I am pretty sure functions in headers should just be "inline", not "static inline".

[Eric Seidel (no email)]

Created attachment 33799 [details]
Add more position constructors


---
 6 files changed, 62 insertions(+), 8 deletions(-)

[Eric Seidel (no email)]

Created attachment 33800 [details]
Rename positionBeforeNode to positionInParentBeforeNode


---
 16 files changed, 96 insertions(+), 46 deletions(-)

[Eric Seidel (no email)]

Created attachment 33801 [details]
Move them back to Position.h per mjs's request


---
 7 files changed, 68 insertions(+), 45 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 33799 [details]
Add more position constructors

A victim of bug 27849.

[Eric Seidel (no email)]

Comment on attachment 33800 [details]
Rename positionBeforeNode to positionInParentBeforeNode

A victim of bug 27849.

[Eric Seidel (no email)]

The patches are posted here in reverse order.  Again due to some minor troubles with post-commits. ;)

[Maciej Stachowiak]

Reviewed Notification.cpp. Event listener logic looks flawless now. Kudos!

[Maciej Stachowiak]

Oops, commented in wrong bug.

[Adam Barth]

Comment on attachment 33799 [details]
Add more position constructors

yay for removing duplicate code

[Adam Barth]

Comment on attachment 33800 [details]
Rename positionBeforeNode to positionInParentBeforeNode

This patch appears to be a pure alpha-transformation.

[Eric Seidel (no email)]

Landed as r48233, r48234, r48235.