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.
Created attachment 29923 [details] Add PositionConstructors.h 26 files changed, 142 insertions(+), 51 deletions(-)
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!)
Created attachment 29924 [details] Add PositionConstructors.h 26 files changed, 148 insertions(+), 51 deletions(-)
Comment on attachment 29923 [details] Add PositionConstructors.h Uploaded one with a slightly nicer changelog.
Created attachment 29926 [details] Rename positionBeforeNode to positionInParentBeforeNode 15 files changed, 90 insertions(+), 39 deletions(-)
Created attachment 29927 [details] Add more position constructors 6 files changed, 64 insertions(+), 8 deletions(-)
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.
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.
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.
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.
(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
(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.
(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.
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).
Created attachment 30846 [details] Add PositionCreationFunctions.h 26 files changed, 148 insertions(+), 51 deletions(-)
Created attachment 30847 [details] Rename positionBeforeNode to positionInParentBeforeNode 15 files changed, 90 insertions(+), 39 deletions(-)
Created attachment 30848 [details] Add more position constructors 6 files changed, 64 insertions(+), 8 deletions(-)
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. ;)
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.
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.
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".
Created attachment 33799 [details] Add more position constructors --- 6 files changed, 62 insertions(+), 8 deletions(-)
Created attachment 33800 [details] Rename positionBeforeNode to positionInParentBeforeNode --- 16 files changed, 96 insertions(+), 46 deletions(-)
Created attachment 33801 [details] Move them back to Position.h per mjs's request --- 7 files changed, 68 insertions(+), 45 deletions(-)
Comment on attachment 33799 [details] Add more position constructors A victim of bug 27849.
Comment on attachment 33800 [details] Rename positionBeforeNode to positionInParentBeforeNode A victim of bug 27849.
The patches are posted here in reverse order. Again due to some minor troubles with post-commits. ;)
Reviewed Notification.cpp. Event listener logic looks flawless now. Kudos!
Oops, commented in wrong bug.
Comment on attachment 33799 [details] Add more position constructors yay for removing duplicate code
Comment on attachment 33800 [details] Rename positionBeforeNode to positionInParentBeforeNode This patch appears to be a pure alpha-transformation.
Landed as r48233, r48234, r48235.