Allegro.cc - Online Community

Allegro.cc Forums » Allegro Development » Windows: File Selection and Clipboard broken after unicode change

Credits go to Peter Hull and SiegeLord for helping out!
This thread is locked; no one can reply to it. rss feed Print
Windows: File Selection and Clipboard broken after unicode change
tobing
Member #5,213
November 2004
avatar

Hi all, I have recently updated my allegro from git to the current master branch. After that, I noticed that two things don't work (for me) as they did before:

1) copy to clipboard from my app only shows "I" after paste into an editor.

2) the file selection dialog, when called with a file filter, shows no files.

I looked back into the history and found that the last changes to win_dialog.c (bac7bddc9d79cf6d520f27335e01f78ddbb99d34) and wclip board.c (b2e6f42d9525b16553913a041063118d803ab3f5) broke it, so I copied those files back from the version before and things works fine again (for me).

Not sure what it is. I'm using MSVC 2017, platform toolset is "Visual Studio 2017 (v141)" and WIndows SDK is "10.0.17763.0". So I'm not aware that this is so much out of date, and I see that I'm also using MBCS instead of Unicode, but I would expect that the allegro code adapts to that setting in a more graceful way. Or would I need to switch to Unicode here? Hm.

I guess that this question goes mostly to Peter Hull, he did the changes some months ago.

SiegeLord
Member #7,827
October 2006
avatar

Interesting! Can you clarify what you are doing? Are you passing non-UTF8 strings to Allegro? I think the idea of those changes was that they were mostly internal, Allegro always expects UTF8 coming in, and internally converts it to a 16 bit encoding that Windows wants. It shouldn't matter what the host program is compiled under.

"For in much wisdom is much grief: and he that increases knowledge increases sorrow."-Ecclesiastes 1:18
[SiegeLord's Abode][Codes]:[DAllegro5]:[RustAllegro]

tobing
Member #5,213
November 2004
avatar

Well, it's easy enough, here's some code that runs at the beginning of main:

#SelectExpand
1 2int main(int argc, char* argv[], char* envp[]) 3{ 4 // first change current dir, that should not relate to this problem though 5 char fd_buf[1024]; 6 7 { 8 // determine where the executable is and change working dir to that directory 9 // so we can find out ini file later on 10 strcpy(fd_buf, argv[0]); 11 char*p_slash = 0; 12 for(char*p = fd_buf; *p; ++p) 13 { 14 if(*p == ALLEGRO_NATIVE_PATH_SEP) 15 p_slash = p; 16 } 17 if(p_slash) 18 *p_slash = 0; 19 al_change_directory(fd_buf); 20 } 21 22 // init allegro 23 al_init(); 24 al_install_keyboard(); 25 al_install_mouse(); 26 27 al_init_font_addon(); 28 al_init_ttf_addon(); 29 al_init_image_addon(); 30 al_init_primitives_addon(); 31 al_init_physfs_addon(argv[0]); 32 33 //... some other configuration like setting up display and such things 34 35 if(argc <= 1) 36 { 37 ALLEGRO_FILECHOOSER* pfch = al_create_native_file_dialog("", "Select xyz file", "*.xyz;*.cxyz;*.zip", 0); 38 al_show_native_file_dialog(p_display, pfch); 39 if(al_get_native_file_dialog_count(pfch) == 1) 40 { 41 const char* filename = al_get_native_file_dialog_path(pfch, 0); 42 strcpy(fd_buf, filename); 43 } 44 else 45 { 46 exit(-1); 47 } 48 } 49 else 50 { 51 strcpy(fd_buf, argv[1]); 52 }

So what worked before is that the box showed the matching files, and the button is labelled "Select xyz file". With the changes the button is correctly labelled, but no files seem to match, so matching files are shown.

However, when I type "*.xyz" into the selection field, then the matching files appear...

Peter Hull
Member #1,136
March 2001

Alright it is I think because the filter has embedded NULLs in it and my al_ustr->utf16 conversion didn't account for that.
Filter is assembled in create_filter_string (win_dialog.c:128) and conversion is via _twin_ustr_to_tchar which you will see assumes a C-style null termination.

Might be good for the ex_native_filechooser to make use of the filter which it doesn't currently.

Haven't looked at the clipboard thing yet.

[edit] Please can someone file a bug or bugs? Thanks.

SiegeLord
Member #7,827
October 2006
avatar

Good call, Peter! I could reproduce tobings issue with the file dialog, and made a fix based on your observation: https://github.com/liballeg/allegro5/pull/1114.

tobing, would you mind testing it? I also looked at the clipboard stuff, but it didn't appear to have any obvious issues. Can you explain a bit more what you do there? Are you passing a unicode string to al_set_clipboard_text?

"For in much wisdom is much grief: and he that increases knowledge increases sorrow."-Ecclesiastes 1:18
[SiegeLord's Abode][Codes]:[DAllegro5]:[RustAllegro]

Peter Hull
Member #1,136
March 2001

Great. I had written my own fix which was almost word-for-word identical with your PR :) . But, does it need fixing for the non-UNICODE build too?

[edit]
On the clipboard, we are always converting ALLEGRO_USTR to UTF-16, but we are setting it onto the clipboard using either CF_UNICODETEXT or CF_TEXT depending on the value of UNICODE. See https://github.com/liballeg/allegro5/blob/36c1549bae816c35a79d9a842beb777bde4b4a28/src/win/wclipboard.c#L91

I think regardless of the value UNICODE used when compiling Allegro, we should be using CF_UNICODETEXT as I don't think there are supported non-unicode-aware versions of Windows any more. (Windows ME?)
What do you think?
Also there is probably something equally wrong on the 'paste' side.

SiegeLord
Member #7,827
October 2006
avatar

But, does it need fixing for the non-UNICODE build too?

I thought we decided to get rid of the ANSI code path? We could still add it in the meantime, I suppose.

Quote:

On the clipboard, we are always converting ALLEGRO_USTR to UTF-16, but we are setting it onto the clipboard using either CF_UNICODETEXT or CF_TEXT depending on the value of UNICODE.

Interesting. That does look wrong for the non-UNICODE case, but the UNICODE path seems fine, no? The only way to compile Allegro with ANSI is to edit the CMakeLists.txt, and that seems unlikely to have happened in this case.

"For in much wisdom is much grief: and he that increases knowledge increases sorrow."-Ecclesiastes 1:18
[SiegeLord's Abode][Codes]:[DAllegro5]:[RustAllegro]

Peter Hull
Member #1,136
March 2001

SiegeLord said:

and that seems unlikely to have happened in this case.

Tobing did say he was using MBCS, in the first post. But I agree, there should be no reason not to use Unicode these days.

Actually, I mis-remembered - I thought it was easy to build a non-Unicode version but, as you say, it does require editing the CMake files. How are you building Allegro, tobing?

tobing
Member #5,213
November 2004
avatar

Sorry, I was a bit captured by something else for some days. Thanks for looking into it.

I'm using more or less hand-made visual studio 2017 projects, and the settings are the same for allegro and all the other libraries I'm building this way. Almost all is statically linked, and I also have zip png jpg ogg lua freetype physfs alguichan in the boat. So it won't be easy to share the exact configuration, but I think I could extract the typical parameter lines for compiler and linker, if needed.

I'll try to figure out how to check out the fix you provided, so I can test it.

For the copy&paste, most of the time I'm pasting into an empty Notepad++ tab, and sometimes into outlook mail directly, without difference in behavior - either both work fine, or they don't. For the question about how I copy the text into the clipboard, I'll have to look up the code...

Edit: checked out your change, and tested. When I finally compiled allegro, aladdons and the main program with UNICODE, the fix worked. So that's nice, but to be honest, why doesn't it also work when I don't use UNICODE? Seems that the code uses _al_win_utf8_to_ansi instead of _al_win_utf8_to_utf16 which has been replaced by your change, so maybe the MBCS-part also needs some fix?

Peter Hull
Member #1,136
March 2001

tobing said:

but to be honest, why doesn't it also work when I don't use UNICODE?

The reason is simply that SL's patch didn't make any changes to the non-Unicode code path.

But, does it need fixing for the non-UNICODE build too?

I believe we are on the verge of making the Unicode version the only version - all supported Windows use Unicode internally and all Allegro's string handling works via UTF-8. However it would not be too difficult to maintain if you've got a good reason for not using Unicode?

Thanks,
Pete

SiegeLord
Member #7,827
October 2006
avatar

I see, it's a custom built Allegro, things are starting to make sense! I'd still love to hear more about what's done to hit the clipboard bug.

We could certainly keep the ANSI versions around, in principle, thanks to Peter's hard work it's essentially free. It's easy to fix the file dialog for ANSI as well, I just didn't do it since I didn't know you were compiling Allegro in ANSI mode yourself.

EDIT: I added the implementation of the fix for ANSI as well.

"For in much wisdom is much grief: and he that increases knowledge increases sorrow."-Ecclesiastes 1:18
[SiegeLord's Abode][Codes]:[DAllegro5]:[RustAllegro]

tobing
Member #5,213
November 2004
avatar

The extended fix solves the issues with the file selection box, both UNICODE and MBCS.

About the question why I used MBCS, there's no reason in particular, just used to have that setting all the time and didn't need to change it. But switching to UNICODE for everything I do (with allegro) is certainly possible, and I think I'll do that for future builds. Because when I'm using strings and text (also from files), I'm thinking UTF8 actually.

The clipboard thing is a different story. Works with UNICODE, but does not work with MBCS. I'll check the details and come back with more information, especially about how I feed my string into the clipboard...

Thanks a lot for the file selection box fix!!!

Edit:

I#m using al_set_clipboard_text with a char* grom a std::string. Some debugging shows that the string is converted using _al_win_utf8_to_utf16 and a few lines later, memmove is used to copy/move the bytes to the dst buffer. There's no obvious difference between UNICODE and MBCS, except that after the memmove, in case of UNICODE, the dst string looks correct, while with MBCS the dst string only shows the first character, even though all bytes have been moved...

That brought me onto the right track, so here's the changed method that works for both UNICODE and MBCS:

#SelectExpand
1static bool win_set_clipboard_text(ALLEGRO_DISPLAY *display, const char *text) 2{ 3 HWND handle = get_window_handle(display); 4 HANDLE hMem = NULL; 5#ifdef UNICODE 6 wchar_t *tstr = NULL; 7#else 8 char *tstr = NULL; 9#endif 10 size_t size; 11 size_t len; 12 LPTSTR dst; 13 14 if (!OpenClipboard(handle)) { 15 ALLEGRO_DEBUG("Could not open clipboard for handle %p", handle); 16 return false; 17 } 18 19 /* Convert the text from UTF-8 to Windows Unicode */ 20#ifdef UNICODE 21 tstr = _al_win_utf8_to_utf16(text); 22 len = wcslen(tstr); 23 size = (len + 1) * sizeof(wchar_t); 24#else 25 tstr = _al_win_utf8_to_ansi(text); 26 len = strlen(tstr); 27 size = len + 1; 28#endif 29 /* Save the data to the clipboard */ 30 hMem = GlobalAlloc(GMEM_MOVEABLE, size); 31 32 if (!hMem) { 33 al_free(tstr); 34 ALLEGRO_DEBUG("GlobalAlloc failed to allocate memory for the clipboard data"); 35 return false; 36 } 37 38 dst = (LPTSTR)GlobalLock(hMem); 39 /* Copy the text over. Unlike SDL, do NOT convert newlines, that's for the 40 * use to decide. */ 41 memmove(dst, tstr, size); 42 dst[len] = 0; 43 GlobalUnlock(hMem); 44 EmptyClipboard(); 45 if (!SetClipboardData(TEXT_FORMAT, hMem)) { 46 al_free(tstr); 47 ALLEGRO_DEBUG("Couldn't set clipboard data"); 48 return false; 49 } 50 al_free(tstr); 51 CloseClipboard(); 52 return true; 53}

Peter Hull
Member #1,136
March 2001

Great, makes sense! But might need a bit more thought about edge-cases etc.

Also the API/docs are a bit vague, al_set_clipboard_text takes const char* which has to be UTF-8 but the docs don't say that - if you were writing ansi code with a specific code-page and using characters > 127 it would fail.

tobing
Member #5,213
November 2004
avatar

Thanks for the fixes!

Go to: