Re: [lmi] wx-2.9.2: undesired change in behavior of MRU list

[Vadim Zeitlin]

On Tue, 19 Jul 2011 14:38:25 +0000 Greg Chicares <address@hidden> wrote:

GC> >  So finally I think that we should modify wx to:
GC> > 
GC> > 1(a) Not remove the file from MRU if it just failed to open.
GC> >  (b) Call a virtual function if a MRU file doesn't exist any longer. This
GC> >      function would remove the file from MRU by default but could be
GC> >      overridden to never do it or ask the user if he wans to do it.
GC> > 
GC> > Alternatively we could do
GC> > 
GC> > 2. Just always call some virtual function when the file from MRU couldn't
GC> >    be opened, whether it was because it was not found or some other reason
GC> >    (and pass the flag indicating its existence, or not, to this function).
GC> > 
GC> > (1) is somewhat simpler while (2) is maximally flexible -- but maybe too
GC> > much so? What do you think?
GC> 
GC> Both those solutions would let me do exactly what I want, but I feel they're
GC> special-purpose tweaks that tend to make wx too...well, rococo, so let's
GC> think about what else could be done.

 They're special in the sense that they're ad hoc hooks added to make the
behaviour of the code removing the MRU list entry configurable but they're
not special at all to LMI. IOW I really don't think I'm tying myself in
knots for LMI sake here, the problem is real and exists for all wx
programs that may fail opening the file otherwise than because of some
fatal problem which will always prevent us from opening it again in the
future.

GC> Any messagebox is obtrusive; is one really warranted here, or would it be
GC> better to remain silent, whether or not the MRU entry is removed?

 I think the user does need to be notified if an MRU entry is removed,
otherwise the entry disappearance could be very mysterious. Also, and here
I realize that my proposed solutions are not enough anyhow, we give an
error message when we fail to create the document anyhow -- but in LMI case
this wouldn't be desirable neither.

GC> Removal from the MRU is desirable in some cases, but not in others, so is
GC> there some way to choose the behavior in a virtual function that wouldn't
GC> seem so odd? For instance, if we want the test to be
GC>   if(!fexist(file))
GC>     remove_from_mru(file);
GC> then it might not be too unnatural to provide a virtual fexist() in the
GC> doc-view or file-history class.

 But maybe you don't know if the file can be opened or not before trying to
do it. An example: trying to open a file created by a newer version of the
program in an older version. This would fail but it doesn't mean that the
file need to be removed from MRU which could be shared by all the program
versions.

GC> Or an OnCreateFailure() virtual would provide the hook we want here;
GC> that seems unremarkable, whereas an OnFailureToOpenFromMRU() function
GC> seems awfully exotic.

 The advantage of OnFailureToOpenFromMRU() is that it's much easier to
write it correctly. OnCreateFailure() could be called from many places and
it's not clear what could do in general.

GC> wxView::OnCreate() returns bool. Should it instead return an enumerated
GC> type that could include wxCREATION_CANCELLED_BY_USER, in which case the
GC> file wouldn't be removed from the MRU?

 This is a nice idea but:

1. It won't solve the problem completely as there can be other reasons for
   not wanting to remove the file from MRU than user cancelling opening it.
2. It will require adding a ton of new methods because we can't modify the
   existing ones because of backwards compatibility so we'd need to add
   new versions in parallel with the existing ones which would be messy.

GC> Another idea: add a flag to control whether the file is removed from the
GC> MRU, perhaps in the 'long flags' argument to wxView::OnCreate(), though
GC> it might be better to add a 'flags' argument to the wxFileHistory ctor
GC> for this purpose.

 This could definitely work, although I'd rather add a method to
wxDocManager governing this, e.g. SetRemoveFromMRUOnFailureToOpen(bool).
It would be simple and it would solve the problem but:

1. It won't help with the issue of the error message and also adding
   SetReportErrorOnFailureToOpenMRU() is unappealing.
2. It still won't be as flexible as a virtual OnFailureToOpenFromMRU()
   as it wouldn't allow to distinguish between failing to open because
   the file doesn't exist and because opening it failed and so on.


 So on balance I still think adding a new virtual method remains the best
solution as it's not that much more difficult than setting a flag or
calling a function but is more flexible and should really satisfy everyone.
I'm still not sure why do you think it's so particular so I could be
missing something.

 In any case, thanks for your suggestions, if nothing else, I realized that
I missed the error message issue while forming my objections to them ;-)

 Thanks,
VZ

P.S. Vaclav: I believe you're not available today but I'd welcome your
     thoughts about this too when you're back, TIA.