Skip to content

Correction bug #1021 Windows File Dialog doesn't append file extension #220

Closed
wants to merge 1 commit into from

2 participants

@bestel74
bestel74 commented Mar 5, 2014

Correction bug #1021 Windows File Dialog doesn't append file extension.

bug ref : https://sourceforge.net/p/geany/bugs/1021/

Signed-off-by: bestel steven.valsesia@gmail.com

@b4n b4n and 1 other commented on an outdated diff Mar 6, 2014
- if (initial_file != NULL)
- MultiByteToWideChar(CP_UTF8, 0, initial_file, -1, w_file, sizeof(w_file));
+ if (DOC_FILENAME(doc) != NULL)
+ MultiByteToWideChar(CP_UTF8, 0, DOC_FILENAME(doc), -1, w_file, sizeof(w_file));
+
+ // Convert the extension for of.lpstrDefExt
@b4n
Geany member
b4n added a note Mar 6, 2014

I don't like C++-style comments… but some people told me to shut up about that :)

@bestel74
bestel74 added a note Mar 6, 2014

Don't like me neither, make noisy warning with some setting, don't know why I used C++ style !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@b4n b4n and 1 other commented on an outdated diff Mar 6, 2014
- if (initial_file != NULL)
- MultiByteToWideChar(CP_UTF8, 0, initial_file, -1, w_file, sizeof(w_file));
+ if (DOC_FILENAME(doc) != NULL)
+ MultiByteToWideChar(CP_UTF8, 0, DOC_FILENAME(doc), -1, w_file, sizeof(w_file));
+
+ // Convert the extension for of.lpstrDefExt
+ MultiByteToWideChar(CP_UTF8, 0, doc->file_type->extension, -1, w_file_type, sizeof(w_file_type));
@b4n
Geany member
b4n added a note Mar 6, 2014

You should check doc->file_type->extension != NULL before passing it to MultiByteToWideChar(), I doubt NULL is a valid value for the lpMultiByteStr argument. Also, although there currently is a bug in many calls, the last argument should be the number of wide chars in w_file_type, not number of bytes. Use G_N_ELEMENTS(w_file_type).

Also, you should check if doc->file_type also before accessing its content. I would think it's not required, but the code is a bit tricky, and we do the check in a lot of places, so maybe it's really required. And anyway, it doesn't hurt.

@bestel74
bestel74 added a note Mar 6, 2014
  • I've got no problem of passing a NULL arguments, but maybe it's a bad idea so I'll correct that.
  • I copy-paste the form used before, thought it was ok.
  • Ok !
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@b4n b4n and 1 other commented on an outdated diff Mar 6, 2014
{
OPENFILENAMEW of;
gint retval;
gchar tmp[MAX_PATH];
wchar_t w_file[MAX_PATH];
wchar_t w_title[512];
+ wchar_t w_file_type[64];
@b4n
Geany member
b4n added a note Mar 6, 2014

Do we really need 64 characters for an extension? Not like it matters much, but this looks awfully long, doesn't it?

@bestel74
bestel74 added a note Mar 6, 2014

Yes, I totally forget that !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@b4n b4n commented on an outdated diff Mar 7, 2014
- MultiByteToWideChar(CP_UTF8, 0, title, -1, w_title, sizeof(w_title));
+ /* Convert the extension for of.lpstrDefExt */
+ if ((doc->file_type->extension != NULL) && (doc->file_type != NULL))
@b4n
Geany member
b4n added a note Mar 7, 2014

You need to swap the tests: in C the tests are executed from left to right, and lazily (if at any point the result is known, further test are not executed). So, if you check first doc->file_type->extension != NULL and then doc->file_type != NULL, the second test is useless since if doc->file_type was actually NULL the first test accessing a field of it would have crashed the program already. So, just swap those tests :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@b4n
Geany member
b4n commented Mar 7, 2014

Apart from the remark, changes look good. However, I already fixed the existing MultiByteToWideChar() calls, so it will conflict. Not a big problem, this can be fixed when merging, but you could also rebase your branch on top of master and fix it at this point ;)

Also, again, please squash the commits together ;)

@bestel74 bestel74 Correction bug #1021 Windows File Dialog doesn't append file extension
bug ref : https://sourceforge.net/p/geany/bugs/1021/

Signed-off-by: Steven Valsesia <steven.valsesia@gmail.com>
b5bbddd
@b4n
Geany member
b4n commented Mar 11, 2014

Hum. Although the code now looks good, I'm not convinced the behavior is acceptable.

First, the default extension is not shown. This is maybe normal to a Windows user, but I think this is most generally inappropriate here because we target programmers, and generally file extensions are important (unlike when e.g. saving an application-specific file).

Then the real problem: if the user chooses a weird extension (?), Windows (XP?) will return the filename as if the user didn't chose any extension at all. For example, if the file type is C++ and the user types foo.xyz, the dialog will return foo.xyz.cpp. However, if the user typed foo.py, the dialog would have returned foo.py as expected. I don't think this is acceptable.

I then suggest we could simply append the default extension to the filename if it doesn't already exist, and leave the extension empty, since Windows don't seem to try to be smart about it then:

diff --git a/src/win32.c b/src/win32.c
index 76b1b0e..0303b2a 100644
--- a/src/win32.c
+++ b/src/win32.c
@@ -426,18 +426,20 @@ gchar *win32_show_document_save_as_dialog(GtkWindow *parent, const gchar *title,
    gchar tmp[MAX_PATH];
    wchar_t w_file[MAX_PATH];
    wchar_t w_title[512];
-   wchar_t w_file_type[16];
+   int n;

    w_file[0] = '\0';
-   w_file_type[0] = '\0';

    /* Convert the name of the file for of.lpstrFile */
-   MultiByteToWideChar(CP_UTF8, 0, DOC_FILENAME(doc), -1, w_file, G_N_ELEMENTS(w_file));
+   n = MultiByteToWideChar(CP_UTF8, 0, DOC_FILENAME(doc), -1, w_file, G_N_ELEMENTS(w_file));

-   /* Convert the extension for of.lpstrDefExt */
-   if ((doc->file_type != NULL) && (doc->file_type->extension != NULL))
-       MultiByteToWideChar(CP_UTF8, 0, doc->file_type->extension, -1, w_file_type,
-               G_N_ELEMENTS(w_file_type));
+   /* If creating a new file name, convert and append the extension if any */
+   if (! doc->file_name && doc->file_type && doc->file_type->extension && n + 1 < G_N_ELEMENTS(w_file))
+   {
+       w_file[n - 1] = L'.';
+       MultiByteToWideChar(CP_UTF8, 0, doc->file_type->extension, -1, &w_file[n],
+               G_N_ELEMENTS(w_file) - n - 1);
+   }

    MultiByteToWideChar(CP_UTF8, 0, title, -1, w_title, G_N_ELEMENTS(w_title));

@@ -458,7 +460,7 @@ gchar *win32_show_document_save_as_dialog(GtkWindow *parent, const gchar *title,
    of.nMaxFile = 2048;
    of.lpstrFileTitle = NULL;
    of.lpstrTitle = w_title;
-   of.lpstrDefExt = w_file_type;
+   of.lpstrDefExt = L"";
    of.Flags = OFN_FILEMUSTEXIST | OFN_EXPLORER;
    retval = GetSaveFileNameW(&of);

What do you guys think? And does it behave differently on more recent Windowses (Vista, 7, 8)?

@bestel74

Works better than mine ! I close my PR :)

@bestel74 bestel74 closed this Mar 11, 2014
@bestel74 bestel74 deleted the bestel74:bug_#1021_no_ext_with_native_dialogs branch Mar 11, 2014
@b4n
Geany member
b4n commented Mar 11, 2014

I just committed my amended version in bd6db90. Thanks anyway for the initial work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.