Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A better way to implement IMGUI::filesystem. #13

Open
daysofbeingwild opened this issue Jan 19, 2017 · 7 comments
Open

A better way to implement IMGUI::filesystem. #13

daysofbeingwild opened this issue Jan 19, 2017 · 7 comments

Comments

@daysofbeingwild
Copy link

My operating system is Windows 10 64-bits and the system's language is chinese.
Here I have a folder, its cotents show like these:
1
When I use the IMGUIFs::Dialog and enter this folder, I got:

2
It seems that:

  1. All files or directries with non-ASCII name are not listed;
  2. Files with long name are shrinked.

Both 1 and 2 are caused by using of API scandir. I hadn't known this API before. But when I debug into the Directory::GetFiles and Directory::GetDirectories I make sure it is the causing reason.

(By the way, I got compiler error when I compiler filesystem.cpp under Visual Studio 2015 X64 model. I fix it by adding #ifdef _WIN32 #include <Windows.h> #endif piror to the first include directive in this file.)

For myself, I modified the filesystem.cpp to resolve this as following:
Add supporting header and API:

#ifdef IMGUI_FILESYSTEM_USE_STD_FILESYSTEM
#include <experimental\filesystem> // VS-builtin experimental library for c++17
#include <boost\locale.hpp> // encoding related
/* the so-called `exec_char_enc` is a kind of encoding that C++'s specified compiler use in C++ standard API such as printf std::cout and so on. When you write "IMGUI", the string has this encoding. ` */
inline std::string to_exec_char_enc(
	const std::string &text,
	const std::string &fromencoding)
{
	return boost::locale::conv::between(text, "GB2312", fromencoding);
}

inline std::string u8_to_exec_char_enc(
	const std::string &text)
{
	return to_exec_char_enc(text, "UTF-8");
}
#endif

Subsitute the code fragments in Directory::GetFiles or Directory::GetDirectorys by:

#ifdef IMGUI_FILESYSTEM_USE_STD_FILESYSTEM
namespace fs = std::experimental::filesystem;
		{
			auto srcdir = u8_to_exec_char_enc(directoryName);
			result.clear();
			if (pOptionalNamesOut)
				pOptionalNamesOut->clear();
			fs::path rootpath(srcdir);
			std::error_code errc;
			auto r = fs::directory_iterator(rootpath, errc);
			if (errc)
				return;
			for (auto &item : r)
			{
				if (item.status().type() == fs::file_type::directory) // or, fs::file_type::regular_file
				{
					auto stdstr = item.path().u8string();
					String::PushBack(result, stdstr.c_str());
					auto fnamepath = item.path().filename().u8string();
					if (pOptionalNamesOut)
						String::PushBack(*pOptionalNamesOut, fnamepath.c_str());
				}
			}
			return;
		}
#endif

The result:

3
Hope this helps. ------ Leslie.

@Flix01
Copy link
Owner

Flix01 commented Jan 19, 2017

First of all, thanks for your feedback!
Now, let's examine your observations one by one:

When I use the IMGUIFs::Dialog and enter this folder, I got:
All files or directries with non-ASCII name are not listed;
Files with long name are shrinked.

Well, this is documented in README_FIRST.txt and it shouldn't happen if you define in your Project Settings DIRENT_USES_UTF8_CHARS. Have you tried it ?

(By the way, I got compiler error when I compile filesystem.cpp under Visual Studio 2015 X64 model. I fix it by adding Windows.h prior to the first include directive in this file.)

Thanks for this tip, I think I'll add it in next commit, since many people compile the cpp files of the addons by themselves to include them as stand-alone (without using the provided imgui_user.h imgui_user.inl), and I'm recently making them compilable this way too, wherever possible. Added.

For myself, I modified the filesystem.cpp to resolve this as following: [...]

Thanks, but I'd rather not depend on c++17 and/or boost.
Actually I try not to depend on nothing: no c++11, and I even try to support old compilers (AFAIK Visual C++ 2012 or newer has the header dirent.h, but I don't use it to support older versions).

Hope this helps.

It helped. Thank you!

Flix01 added a commit that referenced this issue Jan 19, 2017
Added windows.h to make it easier to compile as stand-alone. See #13.
@Flix01
Copy link
Owner

Flix01 commented Jan 19, 2017

Actually, on a second thought:

1- All files or directries with non-ASCII name are not listed;

This shouldn't happen... the paths with non-ASCII name should be converted to short paths... this is very strange...

... maybe I could add your workaround... if it must be explicitly enabled I think it's acceptable.
Is it possible to cut out the boost dependency, and the hard-coded simplified chinese locale (GB2312) ?

Having some other point of view on this could be helpful... I'm on Linux and I cannot test it on Windows (other than under Wine,and it seems to work there (but I didn't test it too much)).

@Flix01
Copy link
Owner

Flix01 commented Jan 22, 2017

@daysofbeingwild:

  • Can you please try defining DIRENT_USES_UTF8_CHARS and see if it works this way ?

  • If it doesn't work, would you mind posting here (as text), the names of the files/directories that don't work as expected ? This way I can try to have a reproducible example to confirm the bug. [Also I think I need a .ttf font that contains simplified-chinese glyphs. Where can I find one ?]

[Edit] I made some tests and I could not reproduce the issue.

  • My test folder is like this:
    chinese-paths-test-nq8

  • When I compile it for Windows (and I run it through Wine), without DIRENT_USES_UTF8_CHARS, I get:
    chinese-test_windows01-nq8

  • When I compile it for Windows (and I run it through Wine), with DIRENT_USES_UTF8_CHARS, I get:
    chinese-test-windows02-nq8

As you can see all the 5 paths appear in the second pic, ad the correct UTF8 paths are displayed by the third pic [Well, I assume it's correct, even if the chinese glyphs don't look exactly the same to me 😕 ].

  • When I compile it and run for Linux I always get the correct results (DIRENT_USES_UTF8_CHARS affects only Windows users):
    chinese-test-linux-nq8

So if there's still a bug it might be because:

  • Wine does not behave like a real Windows environment.

Or:

  • We have a different default C locale and that affects the behaviour of the string conversion.

If this is the case, maybe you can try fixing it changing the encodings in the methods MultiByteToWideChar(...) and WideCharToMultiByte(...) inside dirent_portable.h.

If you manage to do it and it works, we can consider adding an optional definition to correct this.

I prefer this solution (if it works), rather than adding an optional std::filesystem branch.

@daysofbeingwild
Copy link
Author

Hi Flix.

I tried to define DIRENT_USES_UTF8_CHARS macro. All directoires are correctly displayed but NONE of the regular files are displayed.


As I know, the header <dirent.h> and the function you use scandir are not part of C/C++ standard. It seems like a POSIX interface. The Windows OS doesn't implement a compelte POSIX interfaces. So it wouldn' t be surprising that it behaves strange on Windows.

Actually, all things we should do are making a right form directory or file path and passing it to our C++ compiler to list or get it's content. But the path forms are different on Windows and Unix-like system. So it's hard to do this. Because you want write some codes like:

#if is_windows
#elif is_unix_like
#end

std::expermental::filesystem(VS2015), std::filesystem(C++17), boost::filesystem(boost-dependent) are best choices. I understand your reasons why you don't like them -- they make your library large.
But if you plan to solve the paths-issuse on all platforms, you must use them or write it by your selft.


The following is my directory structure(font file 10M+):
新建文件夹.zip

ARIALUNI.zip

Note: you should use UTF-8 for final displaying becase IMGUI only recognize UTF-8 encoding for chinese fonts(If I'm wrong, tell me).

@Flix01
Copy link
Owner

Flix01 commented Jan 23, 2017

All directories are correctly displayed but NONE of the regular files are displayed.

That's a pity! But at least, the encoding problems seem to be solved!
Indeed regular files should be displayed (only non-regular files like links should be hidden): so there must be some kind of bug I cannot replicate...

As I know, the header dirent.h and the function you use scandir are not part of C/C++ standard. It seems like a POSIX interface. The Windows OS doesn't implement a compelte POSIX interfaces. So it wouldn' t be surprising that it behaves strange on Windows.

Yes, but a lot of people are successfully going this route (try searching: "dirent for Windows" on Google).

I understand your reasons why you don't like them -- they make your library large.
But if you plan to solve the paths-issuse on all platforms, you must use them or write it by your selft.

Correct, but this isn't going to make my library too larger if I just add an optional definition...
Still, your code depends on the hard-coded simplified chinese locale ("GB2312"), so I could add a definition like IMGUIFILESYSTEM_USE_STD_FILESYSTEM_FOR_SIMPLIFIED_CHINESE.

Anyway I cannot test it at all, so I'd need all the exact changes...

The following is my directory structure(font file 10M+): 新建文件夹.zip

I can't use it, when I decompress it on my system, I see something like this:
selection_044
and inside:
selection_045

As you can see, all the names are wrong and it seems there are only folders and a single file in the first directory (is this correct?)!

ARIALUNI.zip

At least a good font for simplified chinese. Thanks!

Note: you should use UTF-8 for final displaying becase IMGUI only recognize UTF-8 encoding for chinese fonts(If I'm wrong, tell me).

That's probably correct. But it seems the encoding problems are gone now. The only problem left is that files don't show up, as far as I can see.

In short:
I could add such definition IMGUIFILESYSTEM_USE_STD_FILESYSTEM_FOR_SIMPLIFIED_CHINESE, but I'm still not sure if your changes are enough. For example:

  • Directory::GetFiles() has two overloads.

  • The inclusion of dirent_portable.h (and maybe sys/stat.h too?) should be excluded.

and I should make all these changes without being able to test the result...

If you could make a gist with a correct version (or a pull request), it will be easier for me to include it!

P.S. I mark this as enhancement, because I cannot confirm the bug on my system.

@Flix01
Copy link
Owner

Flix01 commented Jan 23, 2017

In the meantime, in a recent commit I've:

  • removed DIRENT_USES_UTF8_CHARS (made the default).

  • added the optional (and clearer) definition IMGUIFILESYSTEM_USE_ASCII_SHORT_PATHS_ON_WINDOWS to disable it.

I think this is what most Window users expect.

@Flix01
Copy link
Owner

Flix01 commented Jan 24, 2017

[Just an idea] Have you tried to explicitly set your C and C++ locale to your native locale (it should be simplified-chinese) and see if with these settings you can have correct automatic conversions to the GB2312 encoding (so that you can remove the boost dependency)?

Not sure at all if this works, but maybe if you define something like:

#include <locale.h> // for the C locale
#include <locale> // for the C++ locale
#include <stdio.h> //fprintf

void main() {

// This sets your default C locale to native and displays it (so that you can check it):
fprintf(stderr,"setlocale(LC_ALL, \"\")=%s\n",setlocale(LC_ALL, "")); // default is setlocale(LC_ALL, "C") AFAIK and it's NOT localized...
// Same for the C++ locale (I suspect this changes the C locale as well...)
std::locale::global(std::locale(""));

}

then it could work.
It's just a try... I'm not too confident that this could work... but it's worth trying... 😕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants