Re: [wx-dev] Faulty code in wxDocument::OnOpenDocument/stream opening?

[Dimitri]

At 08:31 10/10/2002 +0100, you wrote:
>At 17:44 09/10/2002 +0200, you wrote:
>>wxDocument::OnOpenDocument in src/common/docview.cpp:338 :
>> wxFileInputStream store(wxString(file.fn_str()));
>> if (store.LastError() != wxSTREAM_NOERROR)
>> {
>> [...]
>> }
>>
>>If I try to open a file that doesn't exist, the IF branch is
>>not taken, I think it should (wxFile::wxFile doesn't do anything
>>with the return value of Open() which is FALSE in this case).
>>
>>I can't solve the problem later on by letting (my overriden)
>>LoadObject fail, because that means I would have to set the stream
>>to EOF, but I can't use SeekI() because the stream is not opened
>>(ofcourse).
>>
>>So what's a proper solution? Setting the stream's error variable
>>to something when m_file's m_fd is fd_invalid (-1) ?
>
>That would seem reasonable; or the calling code (docview) could
>test for the existence of the file first.

That latter would seem even more reasonable. Because with my current changes
(Setting m_lasterror to wxStream_READ_ERR when !m_file->IsOpened() ) I get
two messages boxes right now: One from the docview code and one wxLogSysError
from wxFile::Open ("can't open file"). I guess to avoid the wxLogSysError msg
one is always supposed to call wxFile::Exists() first instead of Open (or
constructing the wxFile).

I locally added the file existance check in wxDocument::OnOpenDocument (Which
seemed more backwards-comp safe than modifying the file/stream code). When the
file doesn't exist I just return FALSE. Or do you think it's better to show
the MessageBox, like code later on in OnDocumentOpen does? Personally I don't
like the MessageBox calls in there, my program doesn't always have a GUI (And
I might want to have it in a silent, non user-intrusive mode). Also the message
seems a bit non-standard ("Sorry, could not open this file."). If you agree,
what would you suggest? I was thinking of an OnOpenDocumentError (or similar),
which can be overridden.

BTW, with StoryLines, do you use these classes? (wxDocument[Manager]/
wxDocTemplate/wxView) If so, also on the (future) Mac version? Because with the
current wxDocument implementation I foresee problems, since on the Mac it's
common
(or at least used to be) for files to have no file extension (Which wxDocument
currently bases the file type on, and there's no "*.*" support as yet).

Regards,
Dimitri

[Vadim Zeitlin]

On Thu, 10 Oct 2002 23:22:22 +0200 Dimitri <dim...@shortcut.nl> wrote:

D> That latter would seem even more reasonable. Because with my current changes
D> (Setting m_lasterror to wxStream_READ_ERR when !m_file->IsOpened() ) I get
D> two messages boxes right now: One from the docview code and one wxLogSysError
D> from wxFile::Open ("can't open file"). I guess to avoid the wxLogSysError msg
D> one is always supposed to call wxFile::Exists() first instead of Open (or
D> constructing the wxFile).

If you want to avoid it -- yes. But usually you don't. If the docview code
called wxLogError instead of wxMessageBox when opening the file failed,
you'd get only one nice message box with the last error message and the
"Details" button allowing to see the previous ones.

D> I locally added the file existance check in wxDocument::OnOpenDocument (Which
D> seemed more backwards-comp safe than modifying the file/stream code). When the
D> file doesn't exist I just return FALSE. Or do you think it's better to show
D> the MessageBox, like code later on in OnDocumentOpen does? Personally I don't
D> like the MessageBox calls in there, my program doesn't always have a GUI

Another reason to replace them with wxLogError()...

Regards,
VZ

[Dimitri]

Hello,

wxDocument::OnOpenDocument in src/common/docview.cpp:338 :
wxFileInputStream store(wxString(file.fn_str()));
if (store.LastError() != wxSTREAM_NOERROR)
{
[...]
}

If I try to open a file that doesn't exist, the IF branch is
not taken, I think it should (wxFile::wxFile doesn't do anything
with the return value of Open() which is FALSE in this case).

I can't solve the problem later on by letting (my overriden)
LoadObject fail, because that means I would have to set the stream
to EOF, but I can't use SeekI() because the stream is not opened
(ofcourse).

So what's a proper solution? Setting the stream's error variable
to something when m_file's m_fd is fd_invalid (-1) ?

Regards,
Dimitri

[Dimitri]

At 00:42 10/11/2002 +0200, you wrote:
>On Thu, 10 Oct 2002 23:22:22 +0200 Dimitri <dim...@shortcut.nl> wrote:
>
>D> That latter would seem even more reasonable. Because with my current
>changes
>D> (Setting m_lasterror to wxStream_READ_ERR when !m_file->IsOpened() ) I get
>D> two messages boxes right now: One from the docview code and one
>wxLogSysError
>D> from wxFile::Open ("can't open file"). I guess to avoid the
>wxLogSysError msg
>D> one is always supposed to call wxFile::Exists() first instead of Open (or
>D> constructing the wxFile).
>
> If you want to avoid it -- yes. But usually you don't. If the docview code
>called wxLogError instead of wxMessageBox when opening the file failed,
>you'd get only one nice message box with the last error message and the
>"Details" button allowing to see the previous ones.

Ofcourse, I did this now and it feels/looks a lot better. I did keep in the
wxFile::Exists() so there won't be 2 of the same errors in the log dialog. I'll
file a patch soon.

Thanks,
Dimitri