Subject: (longish) Re: crash on importing RTF
From: Mike Nordell (tamlin@algonet.se)
Date: Mon Dec 04 2000 - 08:27:37 CST
Martin Sevior wrote:
> <This is a long email but I think it contains some valuable information
> not present in Sam's collection of Abiword Documents.>
>
> HI Mike,
> I had a brief look at the code you mentioned. Adding a listener in
> the constructor is the process of adding a view to the document. You can't
> have a document without a view. The special change record is
> needed to initialize the new view. See later.
Ouch. Didn't know that but it "feels" like a mistake. Adding a view (just
"something" that displays the doc) isn't a change to the document per se and
does therefore not validate an injection of a change record. Neither is
adding a "controller" (since the view doubles as "controller") a change to
the document.
Could this also be the problem I experience when importing an MSWord
document and AW insists on asking me if I want to revert to the on-disk
document everytime I open the same doc again (without changing anything)?
> The change record is the signal to build the Layout structure from the
> Piece Table.
IMO that was a bad choice since (from the users POV) no change to the
document has occured.
I might be wrong, but shouldn't the Layout only be the Views "helper" in
displaying the contents of the document?
> As far as I've been able to figure out the abiword overview is something
> like this. (Excuse the ascii art, one day I'll do a proper picture)
A love ASCII art, especially when that's the only gfx available. :-)
Thanks.
> So pd_Document is the controller in the model-view-controller
> paradim.
Wierd, since the view also has controller capabilities, e.g.
FV_View::insertParagraphBreak which bypass the document and call piecetable
and Strux stuff directly. Shouldn't insertParagraphBreak be an operation
that the document supplies? Exactly *how* the document chooses to create
that Paragraph Break is of *no concern* to *anyone* (any other class) but
the document. The view(s) should only be notified that a document update has
occured, possibly/preferrable with hints regarding at least which
paragraph(s) was/were affected by the change (to not try to (re)format the
complete document again).
> fv_View operations can modify the piece table through calls to
> pd_Document. These changes are then reflected in all views (or
> layouts) attached to the document.
Have a look at e.g. FV_View::cmdUndo. Is really *anything* between (and
including) the call
m_pDoc->notifyPieceTableChangeStart();
and the call
m_pDoc->notifyPieceTableChangeEnd();
the responsibility of the View? I'd again vote: No.
This responsibility belongs in the doc. It's only the doc that knows when it
is in an invariant consistent state. It's therefore only the doc that knows
when to notify any of its listeners that a *complete* (atomic) change has
occured. I have also been bitten by the fact that change notifications have
arrived prematurely (remember the "special hack" you had to apply for
lists), and that's because the responsibility of notifying listeners of
updates isn't where it should be, in the document class.
> Population is the act of filling the layout with actual data. Now as
> I recall the way a paragraph is actually built is by stuffing everything
> in a span onto a single line via _stuffAllRunsOnALine() in fl_BlockLayout
> then let the line breaking code workout how to actually position the text
> within the layout. There may be bugs in this process but I haven't had the
> time to work out what bugs are present and exactly how to trigger them.
Thanks for the info. If this is the (straighforward) way it actually works,
it will help a lot when I start to debug this stuff.
[...]
> Just doing this would be an enormous help. We have problems with our
> importer which I've plastered over by judicous use of
> fl_BlockLayout()->format() which appears to fix most errors in the intial
> population of layout structures.
Double ouch. This should however be quite easy to fix if we move "document
modifying operations" into the document itself. What possibly can become a
problem is that I don't know if change records are saved per-view or
per-document (yet). There is merit in both ways, but it's probably *very*
hard to get keep consistency if views are to hold on to change records, why
I for now assume it's the document that keeps them in its undo-stack.
> I've seen plenty of these asserts during imports but abi manages to
> recover and import correctly. In your example if you continue past this
> assert do you see a crash?
Yes. After about a gazillion asserts (I've never counted them, the RTF is
7.2 MB :-) ).
> The act of attaching the listener to the pd_Document
> requires some additional transfer of information from pd_Document to the
> layout strcuture being built. At the very least each section and paragraph
> in the layout has to know where in the pieceTable the data corresponding
> to the section and paragraph actually is. These are stored as protected
> member variables in the layout class.
Yet again, ouch. So subclasses are allowed to (possibly) screw up the owning
class by modifying its "protected" data? :-(
> Good Luck!
Thanks...
/Mike - please don't cc
This archive was generated by hypermail 2b25 : Mon Dec 04 2000 - 08:25:31 CST