Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16814 closed defect (wontfix)

Incorrect document template search when !wxDOC_NEW && !wxDOC_SILENT

Reported by: neil_mayhew Owned by:
Priority: normal Milestone:
Component: base Version: 3.0.2
Keywords: docview wxDocManager Cc: troelsk@…
Blocked By: Blocking:
Patch: yes

Description

In an app that has multiple document types (eg samples/docview) a call to:

wxDocManager::CreateDocument(filename)

behaves incorrectly when the filename has a suffix that matches one of the templates. It prompts the user instead of using the appropriate template.

This is due to a typo in wxDocManager::CreateDocument which checks !path.empty() instead of path.empty().

The error goes all the way back to 2008, svn r54965

Attachments (1)

0001-Fix-error-searching-templates-when-opening-documents.patch download (1.1 KB) - added by neil_mayhew 5 years ago.
Fix error searching templates when opening documents

Download all attachments as: .zip

Change History (7)

Changed 5 years ago by neil_mayhew

Fix error searching templates when opening documents

comment:1 Changed 5 years ago by troelsk

  • Cc troelsk@… added

comment:2 Changed 5 years ago by neil_mayhew

  • Resolution set to wontfix
  • Status changed from new to closed

Although this probably is a typo, when you fix it the consequences are undesirable. Previously, it asked the user for the document type as if it was a new document. With this change, however, the code shows a File-Open dialog. The intent is for the user to associate a document type via the filter control at the bottom. However, the dialog isn't primed with the given file path, and instead opens on a different directory with no file selected. This is very confusing for a user, because they've just specified a file and now they're being asked for another one. This behaviour is actually worse than before, so I'm withdrawing this issue.

The solution is never to use CreateDocument without wxDOC_SILENT, because the non-silent logic is completely broken, IMHO. I'll open a different issue for that.

Last edited 5 years ago by neil_mayhew (previous) (diff)

comment:3 follow-up: Changed 5 years ago by vadz

Sorry, I was completely lost here since the beginning but after the latest comment I'm even more lost than before. What exactly is/was wrong here and how could possibly "never use CreateDocument(wxDOC_SILENT)" be implied by "CreateDocument(!wxDOC_SILENT) is broken"??

I have a feeling there is at least one and possibly more missing or extra negations here...

comment:4 in reply to: ↑ 3 Changed 5 years ago by neil_mayhew

Replying to vadz:
Yes, you're right! Sorry about that. I've corrected my comment.

comment:5 follow-up: Changed 5 years ago by neil_mayhew

I'll try to expand on my original description to make things a bit clearer.

The code in question is in src/common/docview.cpp:1482:

// for the new file we need just the template, for an existing one we
// need the template and the path, unless it's already specified
if ( (flags & wxDOC_NEW) || !path.empty() )
    temp = SelectDocumentType(&templates[0], numTemplates);
else
    temp = SelectDocumentPath(&templates[0], numTemplates, path, flags);

So when the path isn't empty it calls a function that doesn't use the path, and when the path is empty (ie there is no path) it calls a function that does use the path. I'm pretty sure that's backwards. Also, you would expect the wxDOC_NEW case to be the same as the one where the path is empty.

However, if you make the change given in my patch (remove the !) it then calls SelectDocumentPath when there is a path, but SelectDocumentPath is broken, IMHO. It doesn't do anything sensible with the path and is confusing to the user because it opens a file dialog located at a different part of the filesystem.

So it's better to leave the broken code in CreateDocument as it is, to avoid calling the even more broken code in SelectDocumentPath. Currently, the only way SelectDocumentPath can be called is if wxDOC_NEW is off and path is empty, which is very unlikely and probably never happens in practice, so no-one's noticed the weird behaviour.

I added code to the docview sample that allows opening files at document startup (eg from the command line) which I'll submit as a separate issue.

comment:6 in reply to: ↑ 5 Changed 5 years ago by vadz

Replying to neil_mayhew:

The code in question is in src/common/docview.cpp:1482:
...
So when the path isn't empty it calls a function that doesn't use the path, and when the path is empty (ie there is no path) it calls a function that does use the path. I'm pretty sure that's backwards.

I don't see at all why would it be so. If the path is not empty, we don't need to do anything with it, so we don't pass it to SelectDocumentType(). If the path is empty, we do need to get it from the user, so we pass it to SelectDocumentPath(). Perhaps you didn't notice that the path is an output parameter here?

This may be a little confusing, but I still don't see anything wrong here.

Also, you would expect the wxDOC_NEW case to be the same as the one where the path is empty.

wxDOC_NEW implies that the path is empty, we probably should have an assert checking that this is indeed the case. However the path being empty does not imply wxDOC_NEW, we can be asking the user for the file name to open. So these cases are not the same.

SelectDocumentPath is broken, IMHO. It doesn't do anything sensible with the path

As we've just established, it is only called with an empty path, so what can it do with it? Of course it does nothing, sensible or otherwise, it's an output parameter for it.

and is confusing to the user because it opens a file dialog located at a different part of the filesystem.

Different from what?

So it's better to leave the broken code in CreateDocument as it is, to avoid calling the even more broken code in SelectDocumentPath.

Sorry, I still have no idea why is anything broken. You should really try to formulate a concrete case in which this code doesn't behave as expected.

Currently, the only way SelectDocumentPath can be called is if wxDOC_NEW is off and path is empty, which is very unlikely and probably never happens in practice

Err, this is a very common case of calling this function, it is called like this every time a "File|Open..." menu item is used in an docview application. I dare hope we would have noticed if it were broken in this case.

Note: See TracTickets for help on using tickets.