[embed] need to reorganize the editor directory structure

VERIFIED FIXED in mozilla0.9.6

Status

()

P1
normal
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: rubydoo123, Assigned: akkzilla)

Tracking

({embed})

Trunk
mozilla0.9.6
embed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(6 attachments, 21 obsolete attachments)

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
(Reporter)

Description

18 years ago
task: need to reorganize the editor directory structure
(Reporter)

Updated

18 years ago
Blocks: 34477
Keywords: embed
Priority: -- → P1
Target Milestone: --- → mozilla0.9
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

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

Comment 2

18 years ago
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.

Comment 3

18 years ago
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.
(Reporter)

Comment 4

18 years ago
the rules should be pulled out as well

Comment 5

18 years ago
Just thought I'd mention that Joe and I also toyed with the idea about putting 
the transactions in a seperate dir.
(Assignee)

Comment 6

18 years ago
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?

Comment 7

18 years ago
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 

Comment 8

18 years ago
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.
(Reporter)

Comment 9

18 years ago
you might as well get ready for the minimal verses the whole nine yards
(Assignee)

Comment 10

18 years ago
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.
(Reporter)

Comment 11

18 years ago
agreed, don't do that now, but just wanted you to have that frame of reference
(Assignee)

Comment 12

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

Comment 13

18 years ago
Created attachment 27084 [details] [diff] [review]
Patch: Changes to existing editor files
(Assignee)

Comment 14

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

Comment 15

18 years ago
Created attachment 27191 [details]
base/Makefile.in
(Assignee)

Comment 16

18 years ago
Created attachment 27192 [details]
text/Makefile.in
(Assignee)

Comment 17

18 years ago
Created attachment 27193 [details]
html/Makefile.in
(Assignee)

Comment 18

18 years ago
Created attachment 27194 [details]
composer/Makefile.in
(Assignee)

Comment 19

18 years ago
Created attachment 27195 [details]
editor/Makefile.in
(Assignee)

Comment 20

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

Comment 21

18 years ago
Created attachment 27208 [details] [diff] [review]
editor log diffs for configure.in and config/autoconf.mk.in
(Assignee)

Comment 22

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

Comment 23

18 years ago
Created attachment 27212 [details]
build/Makefile.in
(Assignee)

Comment 24

18 years ago
Created attachment 27423 [details]
text/nsTextEditUtils.h
(Assignee)

Comment 25

18 years ago
Created attachment 27424 [details]
text/nsTextEditUtils.cpp
(Assignee)

Comment 26

18 years ago
Created attachment 27644 [details] [diff] [review]
Latest diffs of existing editor files
(Assignee)

Comment 27

18 years ago
Created attachment 28376 [details] [diff] [review]
Diffs of existing editor files after Kin's windows testing
(Assignee)

Comment 28

18 years ago
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 ...

Comment 29

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

Comment 30

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

Comment 31

18 years ago
Created attachment 29754 [details] [diff] [review]
Files modified in the past 3 days

Comment 32

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

Comment 33

18 years ago
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? 
(Assignee)

Comment 34

18 years ago
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

Comment 35

18 years ago
Moving to 0.9.1.
Target Milestone: mozilla0.9 → mozilla0.9.1
(Assignee)

Comment 36

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

Comment 37

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

Comment 38

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

Comment 39

17 years ago
Created attachment 46093 [details] [diff] [review]
Unix makefile diffs
(Assignee)

Comment 40

17 years ago
Created attachment 46094 [details]
csh script to copy or link files to their new locations
(Assignee)

Comment 41

17 years ago
Created attachment 46095 [details]
csh to undo the reorg script (used for my own debugging)
(Assignee)

Comment 42

17 years ago
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

Comment 43

17 years ago
do we have a gameplan on this anymore?  Am I holding up the works?  Or are they 
held up without me, as it were...
(Assignee)

Comment 44

17 years ago
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

Comment 45

17 years ago
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?

Comment 46

17 years ago
spam composer change
Component: Editor: Core → Editor: Composer
(Assignee)

Updated

17 years ago
Component: Editor: Composer → Editor: Core
(Assignee)

Comment 47

17 years ago
Created attachment 49158 [details]
Kin's Windows reorg script
(Assignee)

Comment 48

17 years ago
Created attachment 49159 [details]
Kin's Windows makefiles (out of date now)
(Assignee)

Updated

17 years ago
Attachment #27084 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #27191 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #29754 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #28376 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #27644 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #27424 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #27423 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #27212 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #27208 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #27195 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #27194 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #27192 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #27193 - Attachment is obsolete: true
(Assignee)

Comment 49

17 years ago
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.

Comment 50

17 years ago
Created attachment 49285 [details]
All makefile.win files needed in reorganized directories

Comment 51

17 years ago
Created attachment 49286 [details]
Windows batch file to copy files from editor\base to reorganized directories (save and run from "editor" directory)

Comment 52

17 years ago
Created attachment 49287 [details]
Windows .bat file to copy files from editor\base dir to new directories (Save and run from "editor" dir)

Updated

17 years ago
Attachment #49286 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #49158 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #49159 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #46094 - Attachment is obsolete: true
(Assignee)

Comment 53

17 years ago
Created attachment 49289 [details]
Revised copy/link script for Unix
(Assignee)

Comment 54

17 years ago
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.

Comment 55

17 years ago
Created attachment 50491 [details]
perl script to create directories and move files (mac only)

Comment 56

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

Comment 57

17 years ago
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

Comment 58

17 years ago
I need to eyeball joe's post-move layout on mac
(Assignee)

Comment 59

17 years ago
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).

Comment 60

17 years ago
sr=sfraser on the new directory organization.

Comment 61

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

Comment 62

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

Comment 63

17 years ago
Created attachment 51798 [details] [diff] [review]
New Unix Makefile diffs (with REQUIRES)
(Assignee)

Updated

17 years ago
Attachment #46093 - Attachment is obsolete: true
(Assignee)

Comment 64

17 years ago
Created attachment 51799 [details] [diff] [review]
Change to editor/Makefile.in only
(Assignee)

Comment 65

17 years ago
Created attachment 51801 [details]
Up-to-date reorg.csh (has been tested in "live" mode)
(Assignee)

Updated

17 years ago
Attachment #49289 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #51798 - Attachment is obsolete: true
(Assignee)

Comment 66

17 years ago
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

Comment 67

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

Comment 68

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

Comment 69

17 years ago
Created attachment 51979 [details] [diff] [review]
Patch including post-REQUIRES windows makefiles
(Assignee)

Updated

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

Updated

17 years ago
Whiteboard: Needs sr → Ready to check in
(Assignee)

Comment 71

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

Comment 72

17 years ago
I've removed the old files in editor base.  Finally closing the bug!
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 73

17 years ago
Akkana, this is such a monolithic bug fix...do you think you can
verify this and mark verified-fixed? thanks.
(Assignee)

Comment 74

17 years ago
Marking verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.