Closed Bug 66345 Opened 24 years ago Closed 23 years ago

[embed] need to reorganize the editor directory structure

Categories

(Core :: DOM: Editor, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.6

People

(Reporter: rubydoo123, Assigned: akkzilla)

References

Details

(Keywords: embed, Whiteboard: In; Need to remove files in editor/base)

Attachments

(6 files, 21 obsolete files)

1.36 KB, text/plain
Details
10.64 KB, application/zip
Details
7.02 KB, text/plain
Details
7.59 KB, text/plain
Details
9.87 KB, text/plain
Details
7.21 KB, patch
Details | Diff | Splinter Review
task: need to reorganize the editor directory structure
Blocks: 34477
Keywords: embed
Priority: -- → P1
Target Milestone: --- → mozilla0.9
Status: NEW → ASSIGNED
Don't want to start on this 'til Joe's plaintext changes are checked in, since
that's likely to affect any directory reorg.  Marking dependency.
Depends on: 66290
JIt's probably time to start talking about what the directory structure ought to
look like (at least if this is Severity Critical).  Any thoughts?  Should rules
go in a separate directory?  Should plaintext go in a separate directory?  (My
guess is no, but I'll wait to see what Joe checks in)  Do we actually need
directory restructuring?  I'm not sure what the goal here is.
I think the main goal is to separate composer from editor (as text widget) files. 
So the minimal stuff that should be done is to move all the composer-only files 
out of editor/base and into editor/composer/ (which I already made). If we want 
to go further, and separate out rules etc, that would be OK by me.
the rules should be pulled out as well
Just thought I'd mention that Joe and I also toyed with the idea about putting 
the transactions in a seperate dir.
For those lurking here:
I have something that compiles and basically runs.  I'm not ready to post diffs
yet, because I need to update my tree, re-do the way logging is defined (right
now we have it hardwired in the makefile, which obviously won't work when we
have a hierarchy of makefiles; on Unix I'm making it a configure option, will
need help with what to do on mac and windows) but here's the organization I
currently have:

editor/base:
ChangeAttributeTxn.cpp  IMETextTxn.cpp          nsEditor.cpp
CreateElementTxn.cpp    InsertElementTxn.cpp    nsEditorCommands.cpp
DeleteElementTxn.cpp    InsertTextTxn.cpp       nsEditorParserObserver.cpp
DeleteRangeTxn.cpp      JoinElementTxn.cpp      nsEditorService.cpp
DeleteTextTxn.cpp       PlaceholderTxn.cpp      nsEditorUtils.cpp
EditAggregateTxn.cpp    SetDocTitleTxn.cpp      nsInterfaceState.cpp
EditTxn.cpp             SplitElementTxn.cpp     nsSelectionState.cpp
IMECommitTxn.cpp        TransactionFactory.cpp  nsStyleSheetTxns.cpp

editor/text:
nsAOLCiter.cpp              nsInternetCiter.cpp          nsTextEditRules.cpp
nsEditorController.cpp      nsPlaintextDataTransfer.cpp  nsTextEditUtils.cpp
nsEditorEventListeners.cpp  nsPlaintextEditor.cpp        nsWrapUtils.cpp

editor/html:
TextEditorTest.cpp  nsHTMLDataTransfer.cpp  nsHTMLEditorLog.cpp
TypeInState.cpp     nsHTMLEditRules.cpp     nsHTMLEditorStyle.cpp
nsEditProperty.cpp  nsHTMLEditUtils.cpp     nsTableEditor.cpp
nsEditorTxnLog.cpp  nsHTMLEditor.cpp

editor/composer:
nsComposerCommands.cpp  nsEditorShell.cpp  nsEditorShellMouseListener.cpp

A few comments:
I tried, several times, to split the transactions out into a separate library,
but that's a big job and it would be a lot easier to do that separately.  There
are lots of interdependencies between nsEditor.cpp and specific transactions
(especially PlaceholderTransaction, but then TransactionManager depends on
PlaceholderTransaction and includes the rest of the transactions in the same
library) so they have to stay in base for now.

TextEditorTest sounds like it should be in text/ but it actually tests the html
editor.

Should transaction logging go in composer rather than html?  Should it go in
text?  I assume logging should be enabled by default in the unix builds, since
it always has been in our makefile.

Any other classifications I should revisit before starting to batch this up and
make diffs?
These should be moved to editor/composer:
nsEditorParserObserver.cpp
nsInterfaceState.cpp

This should go in base, I think:
nsEditorController.cpp

I think this is dead and should be removed:
TextEditorTest.cpp

Isn't this also fundamental and should be in base:
TypeInState.cpp 
TypeInState is only used to remember lists of html styles to apply or unapply to 
the next thing typed.  As such, it properly belongs in editor/html.

If at some point in the future we split into a minimal html vs whole nine yards 
html, it ca go in the minimal group.
you might as well get ready for the minimal verses the whole nine yards
I'll keep it in mind, but I'm not going to do a minimal/full split in this pass.
 That involves splitting nsHTMLEditor and making a new subclass and a new
interface and changing the references in half the classes which currently refer
to nsIHTMLEditor.

I think that should really be a separate bug, especially since this one is
currently marked critical for embedding whereas I don't think we actually have a
definite list yet of what customers would want in a minimal vs. full html editor.
agreed, don't do that now, but just wanted you to have that frame of reference
I've moved the files Simon suggested (except TypeInState, per Joe's comment) and
am attaching a diff of the changes to various editor files which go along with
the move.  This diff doesn't contain Makefiles or the new locations, only the
changes that had to be made to the existing editor files.
Here's the current list of files moved out of base:

public:
nsIEditProperty.h

text:
nsAOLCiter.cpp              nsInternetCiter.h            nsTextEditRules.h
nsAOLCiter.h                nsPlaintextDataTransfer.cpp  nsTextEditUtils.cpp
nsEditorEventListeners.cpp  nsPlaintextEditor.cpp        nsTextEditUtils.h
nsEditorEventListeners.h    nsPlaintextEditor.h          nsWrapUtils.cpp
nsInternetCiter.cpp         nsTextEditRules.cpp          nsWrapUtils.h

html:
TextEditorTest.cpp  nsEditorTxnLog.h        nsHTMLEditor.h
TextEditorTest.h    nsHTMLDataTransfer.cpp  nsHTMLEditorLog.cpp
TypeInState.cpp     nsHTMLEditRules.cpp     nsHTMLEditorLog.h
TypeInState.h       nsHTMLEditRules.h       nsHTMLEditorStyle.cpp
nsEditProperty.cpp  nsHTMLEditUtils.cpp     nsIHTMLEditRules.h
nsEditProperty.h    nsHTMLEditUtils.h       nsTableEditor.cpp
nsEditorTxnLog.cpp  nsHTMLEditor.cpp

composer:
nsComposerCommands.cpp      nsEditorShell.h
nsComposerCommands.h        nsEditorShellFactory.h
nsEditorController.cpp      nsEditorShellMouseListener.cpp
nsEditorController.h        nsEditorShellMouseListener.h
nsEditorParserObserver.cpp  nsInterfaceState.cpp
nsEditorParserObserver.h    nsInterfaceState.h
nsEditorShell.cpp
Attached file base/Makefile.in (obsolete) —
Attached file text/Makefile.in (obsolete) —
Attached file html/Makefile.in (obsolete) —
Attached file composer/Makefile.in (obsolete) —
Attached file editor/Makefile.in (obsolete) —
Seeking review of the changes before extending the build to the other two
platforms.  The actual code changes aren't really that large (mostly moving code
around, and some warning fixes); we can get together and go over them in a
visual tool, if that would be easier.

Additional help testing would be greatly appreciated, since Linux doesn't show
me undefined symbols until they're actually called.

I'll definitely need Windows help, since I don't have a machine that can build
Windows.  (I can write some makefile.wins, but can't test them.)  I may need Mac
help if I have to create new projects for the new folders, or if my Mac decides
to be flaky about building, and just in getting the files moved onto the mac
with the right file type (I'll ask offline).

Kin, can you help with the Windows build, or should I ask Anthony?
Whiteboard: needs review
I forgot to mention the last directory:
build:
nsEditorRegistration.cpp     nsTextEditorReg.cpp
And almost forgot to attach the diffs to add editor logging as a a configure
option.

Turns out the log is only needed in the html and build directories, so I can
take it out of Makefile.in in the other directories.  Also I've changed the
"EDITOR_LOGGING" references in configure.in and autoconf.mk.in to
"EDITOR_API_LOG" to match the name already used in the editor files (I don't
know why I had previously used the other name).
Attached file build/Makefile.in (obsolete) —
Attached file text/nsTextEditUtils.h (obsolete) —
Attached file text/nsTextEditUtils.cpp (obsolete) —
Kin has it building and working on Windows now.  We moved a few more things
around.  I've attached the most recent diffs.  Now it's just Mac and sr left to
go ...
r=jf
lots of good cleanup.  a few notes: 
kin revamped the IsFoo() nsHTMLEditUtils functs.  We should merge in his changes
with yours.
I don't understand why IsInlineNode() etc are gone.  Now some of the code is
messier, and the only reason I can think of for this would be to force error
propogation, but the errors aren't being propogated in the patch, so I'm
confused.  If it's just style, I'd rather you not do it.  I'm the one who has to
maintain most of this...
I've merged with Kin's factoring -- moved his base routines into nsTextEditUtils
and I call nsTextEditUtils::NodeIsType from the remaining nsHTMLEditUtils
methods.  Should I attach a patch for those two files?

I also have nsComposerController moved to composer/src now; for some reason,
when I integrated with the changes of the last week, things didn't work, so I
had to work on those classes anyway, and I went ahead and moved
nsComposerController.

The IsInline stuff had to be factored because it lived in nsEditor, yet it
incorporated a lot of built-in knowledge of html tags.  Also, it was very
confusing trying to keep straight the difference between IsBlockNode,
NodeIsBlock and IsNodeBlock.  What's an example of code which is now messier
than it was before?  Maybe we can fix it and make it easy for you to maintain
without having the html dependencies in the base editor and without having the
confusion about which methods can be called from where.  It might just be an
issue of choosing more appropriate names.

I'll attach a patch for the current state, in my tree, of the files discussed in
this comment, to make sure we're all on the same page.
/i saw places where:

if (IsInlineNode(foo)) return PR_FALSE;

became:

PRBool isBlock;
NodeIsBlock(&isBlock);
if (!isBlock) return PR_FALSE;


I might have the routine names wrong but the pattern is right.
This kind of change seems gratuitous.  There's clearly a loss of succinctness and 
there is no corresponding win.
Joe: in nsHTMLEditRules, I made local helper apps since IsBlock and IsInline
were called so often, so as not to inflate the code.  (I think some of the early
diffs didn't have these helper apps; is it possible that you were looking at an
early diff when you had that reaction?)  If there are other files (which ones?)
which make these calls often enough to need the shorthand, we can move the
helper functions into nsHTMLEditUtils -- would that be better? 
Phase 1 is checked in: the changes to the existing files, factoring into a
couple of new files, and the new directory structure with unix/win makefiles.

Phase 2, once we get the mac build working, will be to copy the files in the cvs
repository and cvs remove the old locations.
Whiteboard: needs review → phase 1 is in, needs mac build for phase 2
Moving to 0.9.1.
Target Milestone: mozilla0.9 → mozilla0.9.1
Per discussion with Beth yesterday, we're going to hold off a while on checking
in Phase 2.
Target Milestone: mozilla0.9.1 → mozilla1.0
The current plan is to try to get this in for 0.9.4 if we can get it tested on
all platforms in time.
Target Milestone: mozilla1.0 → mozilla0.9.4
Here are some updated attachments for what needs to be done now: patches for all
the Unix Makefile.ins, and a csh script to link or copy the files to the proper
places.  Windows makefile.wins need to be converted, and it needs mac love from
someone, and testing on both windows and mac.

This does not move any UI files.  That should be done by someone who understands
the UI build (I wasn't able to get it building in anything other than the top
level directory) and is better done as a separate step, preferably under a
different bug, since it isn't needed for embedding.
Attached patch Unix makefile diffs (obsolete) — Splinter Review
Nobody's looked at this yet, as far as I know.   Obviously it's not going to
make it for 0.9.4.
Severity: critical → normal
Target Milestone: mozilla0.9.4 → mozilla0.9.5
do we have a gameplan on this anymore?  Am I holding up the works?  Or are they 
held up without me, as it were...
Status (and sorry, I should have updated the status whiteboard accordingly):
This bug is waiting for mac and windows testing of the latest set of patches.
I think Kin was going to test Windows; I wasn't clear who was going to test Mac.
Would it help to rewrite the reorg script in perl so it would be cross-platform?
 (I wasn't sure if translating to perl would help that, since there's still the
filename issue, e.g. what to use for pathname separators when moving files.)
Whiteboard: phase 1 is in, needs mac build for phase 2 → Waiting for mac/windows testing/review
This bug has been open for 6 months. Is it critical work? Just like to know, now
that we reorg'd:

what the benefits are other than cosmetic.
why this would be more important than critical bugs, or publishing.
what milestone is realistic. is 0.9.5 (early October) realistic?
spam composer change
Component: Editor: Core → Editor: Composer
Component: Editor: Composer → Editor: Core
Attached file Kin's Windows reorg script (obsolete) —
Attachment #27084 - Attachment is obsolete: true
Attachment #27191 - Attachment is obsolete: true
Attachment #29754 - Attachment is obsolete: true
Attachment #28376 - Attachment is obsolete: true
Attachment #27644 - Attachment is obsolete: true
Attachment #27424 - Attachment is obsolete: true
Attachment #27423 - Attachment is obsolete: true
Attachment #27212 - Attachment is obsolete: true
Attachment #27208 - Attachment is obsolete: true
Attachment #27195 - Attachment is obsolete: true
Attachment #27194 - Attachment is obsolete: true
Attachment #27192 - Attachment is obsolete: true
Attachment #27193 - Attachment is obsolete: true
I've marked all the old diffs invalid, so the attachment list should be more
correct now.  I've also attached Kin's old Windows .bat file and his
makefile.wins zip filefor posterity; they may need to be worked over a bit since
the directory structure was changed since the first iteration.  Charley's
looking at it on Windows; Simon said he'd look at it on Mac.  Simon, let me know
if you want a tar file of the editor directory to avoid having to translate or
hand-apply the scripts.  It's 2.5M, so I probably shouldn't attach it here.
Attachment #49286 - Attachment is obsolete: true
Attachment #49158 - Attachment is obsolete: true
Attachment #49159 - Attachment is obsolete: true
Attachment #46094 - Attachment is obsolete: true
Attached file Revised copy/link script for Unix (obsolete) —
cmanske noticed a new file that wasn't being handled in my script, so I've
updated it, and also fiddled a few things so it will work better when it's used
to copy files in the cvs repository.
The attached script, run from a populated mozilla/editor, will do the reorg.

I've made some changes to the mac editor project access paths; I'll check that
in when Akkana is ready to land.  Everything builds fine.

My only quiblle at the moment is that a libeditor/public is created by the
windows script that I adapted this from, but no files are ever placed there.  Is
this intentional?

If we want to move public headers around then I have to touch the perl-based mac
build system to change a path name for manifest execution.
Good catch: libeditor/public being created but empty is not intentional, and
I've now removed it from my copy of the script.

Okay, now I need an sr.  Simon?  Kin?
Whiteboard: Waiting for mac/windows testing/review → Needs sr
I need to eyeball joe's post-move layout on mac
Ugh.  It turns out that none of the diffs apply any more, probably because of
all the relicinsing foo.  I've been unable to get an answer on when the real
relicensing will land, but it sounds like it's probably at least going to be
starting in the next week.  So it's going to beat us to the punch and I'll need
to make another set of diffs after relicensing lands (sigh).
sr=sfraser on the new directory organization.
Ok, if libeditor/public was just a typo then r=jfrancis.  I have an updated
editor.mcp ready to go and have tested the build process for the normal editor
library and also the lightweight text-only editor library.
Checked in some of the makefiles (the ones for the directories that aren't built
yet).  Now we need to get leaf or another build person to help us move files
around in the repository.

Meanwhile, at Simon's request, I filed bug 101464 to cover moving the idl files,
which we should also do some day.
Attachment #46093 - Attachment is obsolete: true
Attachment #49289 - Attachment is obsolete: true
Attachment #51798 - Attachment is obsolete: true
Sigh.  Seemingly the only person capable of moving the files in the repository
has been away during the allowed 0.9.5 checkin window, and the deadline is
today, so it looks like this gets put off yet again.

Meanwhile, the new REQUIRES build system landed, so I've updated the Unix
Makefiles and figured out the requirements.  (I may have some extras, but at
least the list is smaller than it was before.)  I think Kin said that windows
already used REQUIRES, so if we're really lucky we won't have to change the
Windows makefiles for this, and we can pester Leaf to make this happen shortly
after the tree reopens.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
I'll do the repository copy today or tomorrow, and will update the bug when i'm 
done. Once the makefile changes are checked in (after getting a=drivers, now 
that we're frozen), the old file locations will need to be cvs removed.
Thanks!  All the Makefiles are in except the main one under editor (to remove
base and add libeditor and composer directories).  I'll mail drivers and
coordinate with Joe for changing the Mac project files when we flip the switch.
Attachment #51799 - Attachment is obsolete: true
a=dbaron.  Hopefully any regressions will be painfully obvious, but do try to
get the right packaging changes in the first time. :-)
Whiteboard: Needs sr → Ready to check in
Incredibly enough, the patch is in!  We are now building from the new directories.

The old directories have not yet been removed (I'd like to make sure that
everything works first) so I'm leaving the bug open for the job of removing the
old files in base.
Whiteboard: Ready to check in → In; Need to remove files in editor/base
I've removed the old files in editor base.  Finally closing the bug!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Akkana, this is such a monolithic bug fix...do you think you can
verify this and mark verified-fixed? thanks.
Marking verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: