Fix export ffilter and extensions

[gdolle]

This PR fix issues #183 #176

• Concerning Bug with export function call and args number #176, ffilter argument removed
• Fix ffilter. Files should now be listed in import/export dialog
• Fix writers/readers extension check to allow dot in file names Bug export filename with dot char #183
• Fix an export .fif crash when the named file exist (managed via QDialogFile)

NB: This just fix export issues but isn't an export refactoring for #178

[gdolle]

Related to your issue @Qdelannoy

[cbrnr]

Thanks @gdolle! Let me unwrap the various issues:

Concerning #176, ffilter argument removed

Why is this still necessary? I think this was fixed by #177.

Fix an export .fif crash when the named file exist (managed via QDialogFile)

OK thanks, this makes sense. There's just a PEP8 warning about a missing space after the comma.

Fix ffilter. Files should now be listed in import/export dialog

Can you point me to the code that is relevant to this issue? What exactly does this address (i.e. I do not understand what you mean by "files should now be listed in import/export dialog")?

Fix writers/readers extension check to allow dot in file names #183

This is tricky. First, a dot is always seen as a marker to separate the file name from an extension - can you give me a use case where it would make sense to have dots that should not be interpreted as a separator?

Second, here's how I think MNELAB should handle data export. Users explicitly select a file type that they want to export. They can enter a file name, and if the extension matches the selected file extension, this just works. However, if the extension does not match, MNELAB should just prepend the correct extension. For example, let's assume we select the ".FIF.GZ" export. If we use the name test, MNELAB changes it to test.fif.gz. If we use test.fif.gz, MNELAB does nothing and directly uses this name. If we use test.fif, MNELAB changes it to test.fif.gz. And if we use test.edf.gz, MNELAB changes it to test.edf.gz.fif.gz.

I'm not even sure if this makes sense. Wouldn't it be easier if users were not allowed to include extensions and MNELAB always appends it? This means that users would not be allowed to type dots (e.g. MNELAB could strip everything after the first dot and use the preceding characters for the file name). WDYT?

[gdolle]

Hi @cbrnr

Why is this still necessary? I think this was fixed by #177.fix

It is linked to third point changes (I should have say "related" rather than "fix").

There's just a PEP8 warning about a missing space after the comma.

Ok I didn't see this warn, I will check

Can you point me to the code

dc56f3c#diff-975205fb7e7c214f502926a07cc973e12f868b5e81d815bb0a41bd9263980980L135
When you open a save dialog getSaveFileName(), the files are filtered with *.ext (ffilter). But the filter was set to .ext (could not list any files).
Note that it would be better while doing #178 to set Description ext (*.ext);; All Files (*) for data exporter.

This is tricky. First, a dot is always seen as a marker to separate the file name from an extension - can you give me a use case where it would make sense to have dots that should not be interpreted as a separator?

Yes it is. I personally don't use dot in file names, but @Qdelannoy. It's not a good practice to use dot in filename, but it's tolerated in most operating system and there will certainly be "one" user who will use it (=> the current behavior is a GUI crash).
I see two possibilities which are either to support dot character in names or to disable them (and add a dialog to warn the user). I chose the first.

However, if the extension does not match, MNELAB should just prepend the correct extension. For example, let's assume we select the ".FIF.GZ" export. If we use the name test, MNELAB changes it to test.fif.gz. If we use test.fif.gz, MNELAB does nothing and directly uses this name. If we use test.fif, MNELAB changes it to test.fif.gz. And if we use test.edf.gz, MNELAB changes it to test.edf.gz.fif.gz.

This PR treats the case where the user write the extension, there should not be "double" extension.
The default extension is added only if the name has no extension, or if the user did not set one of the supported writers/readers extension (listed).

Wouldn't it be easier if users were not allowed to include extensions and MNELAB always appends it?

I thought about that, but if the user put "test.fif", it means he wish to overwrite test.fif (with confirm dialog) and not create a "test.fif.fif" as you said in previous comment.

[cbrnr]

When you open a save dialog getSaveFileName(), the files are filtered with .ext (ffilter). But the filter was set to .ext (could not list any files). Note that it would be better while doing #178 to set Description ext (.ext);; All Files (*) for data exporter.

I see, so the problem is that the filter is currently .ext, but it should be *.ext, correct?

Regarding extensions, here's how Word handles it:

In the Save dialog, you can select a file type (with corresponding extension), e.g. "Word Document (.docx)". Then you can type a file name:

• If you do not include a file extension (i.e. at least one dot) such as test, Word automatically adds the selected extension (.docx). The file name will be test.docx.
• If you include the correct extension in the file name (.docx) such as test.docx, this will be the name the file is saved as.
• If you include an extension (i.e. at least one dot) that doesn't match the selected file type such as test.doc, Word pops up a dialog asking how to proceed:

I think this is a good approach and MNELAB could also do it like this. This seems to be implemented when I export to FIF and type test.fif.gz, then I get this dialog:

But when I select "Use .fif", the file is called test.fif.fif. When I select "Use both", the file is called test.fif.gz.fif. Is this OK? It seems like the double extension fif.gz causes some issues. Could you write up some test cases so that we know the behavior of file renaming during export?

In any case, your changes look good, but I don't have much time currently, so I apologize if I will take a bit longer to review and merge.

[gdolle]

I see, so the problem is that the filter is currently .ext, but it should be *.ext, correct?

Yes

If I understand you want a specific confirmation for the case the user set a wrong extension. I agree that it would be better.

Maybe this suggestion could be rather part of the export refactor #178 as a further changes rather than this current fix. What do you think ?

[cbrnr]

Yes, I agree that #178 should take care of such things, and a quick workaround now is better than not being able to export. Like I said, I will need some time to review this.

[cbrnr]

@gdolle please add a changelog entry and this is ready to be merged.

[cbrnr]

Thanks @gdolle!