Re: [lmi] a less trivial patch: don't use deprecated MakeDefaultName() + refactoring

[Greg Chicares]

On 2008-05-11 12:51Z, Vadim Zeitlin wrote:
> On Sun, 11 May 2008 02:38:12 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> >  And I also should have probably mentioned in my previous email that I'm
> GC> > going to submit a slightly less trivial patch fixing a similar problem 
> with
> GC> > MakeDefaultName() very soon so if you prefer to apply both of them at 
> once
> GC> > you should wait for it.
> GC> 
> GC> I was about to ask whether you had a replacement for that
> GC> function, too.
> 
>  Attached is the patch which fixes this. I got a little carried away while
> fixing this as I didn't want to duplicate my fix and so I also corrected a
> TODO in census_document.hpp:
> 
> // TODO ?? This class and the illustration-document class have much in
> // common that should be factored into a base class.
> 
>  In order to do this I introduced a new base class, LifeViewDocument, from
> which both CensusDocument and IllustrationDocument inherit now and which
> encapsulates the modification to wxDocument way of working which is used by
> both of these classes. I'm not too happy with the (rather meaningless) name
> of this class but unfortunately I couldn't find anything better, if you
> have any suggestions I'd be grateful.

I think that "TODO" comment had become erroneous since it was
originally written. Probably there was a great deal of duplicated
code many years ago, which was alleviated by creating:
  multiple_cell_document.?pp
  single_cell_document.?pp
and refactoring. There's still some duplication:
 - numerous similar calls to these two functions:
       convert_from_ihs()
       convert_to_ihs()
     but I plan to remove them soon, along with the data
     members they use
 - a lot of similar code like [simplified]
         fstream fs(filename);
         doc_.write(fs); // read() as well as write()
         if(!fs) ... ... ...
     but that complexity should be moved to doc_'s class
     so that we can just write one line here:
         doc_.write(filename); // read() as well as write()
 - parts of OnCreate() and DoSaveDocument()
     but the common part should be moved to the
     [multiple|single]_cell_document classes;
 - all of DoOpenDocument()
     but the implementation is just {return true;}
 - all of OnNewDocument()
     but it's just a few lines of duplicated code, and I don't
     think that alone would justify creating a base class

BTW, it looks like DoWriteDocument() is never called. But instead
of investigating that, I think we should step back and question
the "TODO" comment in light of the foregoing discussion, and the
conclusion is probably that we shouldn't introduce a new base
class. Thanks for trying; sorry about the misleading comment, and
also for the delay in replying--we found a fundamental error in
some other code last week, and now a new volcano has erupted next
to the first one, so I'm going to be very busy for a few more
days at least. It's some insurance stuff that nobody understands,
or else I might have asked you to help with it. But I figure I'd
better write this email now, before I forget what I was going to
say and have to analyze it all over again.

>  Other than refactoring needed to fix this TODO there are only 2 real
> changes in the patch:
> 
> 1. Use MakeNewDocumentName() instead of deprecated MakeDefaultName().
> 
>    Unfortunately the new method will only exist in (yet unreleased) wx
>    2.8.8 so I had to add an "#if wxCHECK_VERSION" check around it, this
>    is a bit ugly but can be removed as soon as 2.8.8 is released and LMI
>    starts using it.

Committed 20080516T1354Z, a few hours before our first volcanic
incident.

> 2. Remove LMI_WX_CHILD_DOCUMENT before passing flags to wxDocument.
> 
>    I think it is wrong to use undefined (at wx level) flags with
>    wxDocument, at the very least this may (and should, although currently
>    doesn't) result in an assert about unknown flag.

That's a good point. Can you write a patch that fixes this one
problem only please?