Small caption in IFileSystem::createFileList (fix proposed)

[Squarefox]

Hi,

IFileSystem::createFileList returns small letter filenames only.
This is annoying under windows and probably a much larger problem under Linux.

The problem is in CFileSystem::createFileList when CFile is create with "CFileList(Path, true, false)" (line 839).
The true specifies IgnoreCase.
So the filename is converted to lower caps.

The fix is simple: just change the true to false. I just tested it.
This also fixes the caption of the file list in the FileOpenDialog (tested also).

Regards,
Squarefox

[CuteAlien]

Yeah, I know and I know it's ugly. The problem is that there was likely some reason it was written like that (usually no programmer just adds a flag simply to be evil...). And I didn't find time yet to investigate this. I suppose the reason had to do with filesystems which can't distinguish upper-case and lower-case (might have been zip files - not sure). Or maybe because Irrlicht doesn't want to distinguish upper-case/lower-case. I rewrote the filename handling already a few years ago to fix some of those problems, but missed this place. And also not 100% certain if this place was really about that (I don't really know more about it than anyone else reading the code - this was written years before I started with Irrlicht).

This has been on my todo for a while already and is one of the things I absolutely wanted to work at before the 1.9 release. But just never found the time so far. To change it I have to check all places where it's used first and figure out what's really going on. I don't want for example to suddenly have all kind of material files failing. There's for example some tools out there which write all texture-names in upper-case letters because this works on Windows. Which I think is the reason why Irrlicht internally doesn't distinguish. But certainly when creating files or handling filenames the names never should actually change (just comparisons are allowed to ignore case). So... it's unfortunately a little bit more work than just flipping a flag and so no idea yet when I finally manage to get to it :-(

[christianclavet]

Hi, I've already proposed a small patch for this. This is an issue since the beginning it was implemented. From what I see the implementation is incomplete. Just toggling the state as proposed would "fix it" while a better solution is found. If the implementation would be complete. we should have access to a "buffered" list and display list. So when you click on a filename that has been converted in lower/uppercase, the result would be the real filename.

I don't want for example to suddenly have all kind of material files failing. There's for example some tools out there which write all texture-names in upper-case letters because this works on Windows.

Windows doesn't care if it's uppercase or lowercase, but Linux need to have the exact name with all the characters intact. If you save your filename in Uppercase letters it will still display and load with this little fix. This problem is there for so long that I wonder why nobody on the Linux side reported this...

Each time I update my Irrlicht distribution (Trying to work on the trunk version) to use with IRB. I have to patch this little annoying thing. IRB is running with this patch since last year (0.21 - 0.31), and no problems on the material side or any other thing. File lists now represent the file as-is and I can load anything I want.

The only little problem now is to resolve accented characters. (Found a way on Linux to read the list of files as UTF8, but still checking on Windows). But this can be done outside the filelist, since I have my own file selector that take care of this. My File selector need more work, but once it's getting better, I'll release it as a separate component as the script editor.

[chronologicaldot]

What about just adding params to createFileList() ? It doesn't have any at the moment. We could change it to have a flag for setting the CFileList. And you could make the defaults such that it wouldn't break any existing code.
That said, I still like the idea of changing the default behavior in general, as it would be more expected to NOT ignore case.

[christianclavet]

Hi, A parameter to put the conversion to on/off would be nice. Personally, I would strip that feature off (upper/lowercase conversion). It's not really difficult to convert a string to upper/lowercase if needed.

For a file list, an expected behavior is to have the list of filename AS-IS. Converting it to lower/upper cases will cause problems on some OS. (Excepting Windows that doesn't care)

[CuteAlien]

Basically irr::io::path was created to handle this stuff. It's just not used in filelists yet (and not sure yet if it should). I was ill all week, so didn't take another look yet. But will try to get to it soon.

[christianclavet]

Thanks CuteAlien! Take care!
EDIT:
Checked the latest revision of the trunk and this only applies on WINDOWS!

Code: Select all

//! Construct from native filesystem
    if (FileSystemType == FILESYSTEM_NATIVE)
    {
        // --------------------------------------------
        //! Windows version
        #ifdef _IRR_WINDOWS_API_
        #if !defined ( _WIN32_WCE )
 
        r = new CFileList(Path, true, false);
 
        // TODO: Should be unified once mingw adapts the proper types
#if defined(__GNUC__)
        long hFile; //mingw return type declaration
#else
        intptr_t hFile;
#endif
 
        struct _tfinddata_t c_file;
        if( (hFile = _tfindfirst( _T("*"), &c_file )) != -1L )
        {
            do
            {
                r->addItem(Path + c_file.name, 0, c_file.size, (_A_SUBDIR & c_file.attrib) != 0, 0);
            }
            while( _tfindnext( hFile, &c_file ) == 0 );
 
            _findclose( hFile );
        }
        #endif
 
        //TODO add drives
        //entry.Name = "E:\\";
        //entry.isDirectory = true;
        //Files.push_back(entry);
        #endif
 
        // --------------------------------------------
        //! Linux version
        #if (defined(_IRR_POSIX_API_) || defined(_IRR_OSX_PLATFORM_))
 
 
        r = new CFileList(Path, false, false);
 
        r->addItem(Path + _IRR_TEXT(".."), 0, 0, true, 0);
 
        //! We use the POSIX compliant methods instead of scandir
        DIR* dirHandle=opendir(Path.c_str());
        if (dirHandle)
        {
            struct dirent *dirEntry;
            while ((dirEntry=readdir(dirHandle)))
            {
                u32 size = 0;
                bool isDirectory = false;
 
                if((strcmp(dirEntry->d_name, ".")==0) ||
                   (strcmp(dirEntry->d_name, "..")==0))
                {
                    continue;
                }
                struct stat buf;
                if (stat(dirEntry->d_name, &buf)==0)
                {
                    size = buf.st_size;
                    isDirectory = S_ISDIR(buf.st_mode);
                }
                #if !defined(_IRR_SOLARIS_PLATFORM_) && !defined(__CYGWIN__)
                // only available on some systems
                else
                {
                    isDirectory = dirEntry->d_type == DT_DIR;
                }
                #endif
 
                r->addItem(Path + dirEntry->d_name, 0, size, isDirectory, 0);
            }
            closedir(dirHandle);
        }
        #endif
    }
    else
    {
        //! create file list for the virtual filesystem
        r = new CFileList(Path, false, false);
 
        //! add relative navigation
        SFileListEntry e2;
        SFileListEntry e3;
 
        //! PWD
        r->addItem(Path + _IRR_TEXT("."), 0, 0, true, 0);
 
        //! parent
        r->addItem(Path + _IRR_TEXT(".."), 0, 0, true, 0);
 
        //! merge archives
        for (u32 i=0; i < FileArchives.size(); ++i)
        {
            const IFileList *merge = FileArchives[i]->getFileList();
 
            for (u32 j=0; j < merge->getFileCount(); ++j)
            {
                if (core::isInSameDirectory(Path, merge->getFullFileName(j)) == 0)
                {
                    r->addItem(merge->getFullFileName(j), merge->getFileOffset(j), merge->getFileSize(j), merge->isDirectory(j), 0);
                }
            }
        }
    }

If I'm reading right, if I'm using a virtual filesystem (archives), the filenames are untouched (even on windows).

So that's not as bad as I was thinking! Only bad for one thing: PORTABILITY
My Windows IRB users that would create projects using this would be forced to check their projects ONLY on windows because the filenames that would be saved would have their letters all put in uppercases.

But Linux users, and other platforms could save the files and that would work on any other since they are untouched. So If I still want to maintain portability I still have to patch and I will.

[chronologicaldot]

Can we add the param to createFileList() for setting the case-sensitivity type?

Having read this: https://www.python.org/dev/peps/pep-0235/
imo, It seems someone programmed createFileList() to be platform-specific (i.e. Windows doesn't care about case and Linux does) and didn't think anyone would be using/sharing their program's project files (such as christianclavet) on multiple operating systems. It makes perfect sense when the software is compiled to be used on only one platform, and it's still "cross-platform". It's just that our needs are now extending beyond the rigidity of the original system.

To quote the end of the article I linked: "Case-destroying filesystems are a vanishing breed, and support for them is ugly."

[CuteAlien]

I'm aware of all that. That's why creating the irr::path stuff was one of the first things I did when I joined the engine - to get rid of case-sensitivity in all kind of places. Just had missed this one back then. It's not that I don't want to fix it, it's just that I always got other open tasks are bothering me even more when I find time for Irrlicht coding. Or which are easier to do (this has to be handled rather carefully to not mess up archive loading).

[Cube_]

if other linux folks are like me the likely reason for not reporting it is simply just already using all lowercase names - that's pretty much all I have to add here since the issue doesn't affect me (given my naming conventions).

[CuteAlien]

I guess the main reason it's not reported more is that not many people need the file-open dialog itself :-)

[christianclavet]

Hi, That will affect people that are using the file open dialog but also people that create similar stuff using Irrlicht to create a filelist.

This will simply put all in uppercase the filename (ONLY ON WINDOWS). But from what I know, Windows don't care if it's lowercase or uppercase so I consider this part of code "evil". There is not good reason to put filenames in uppercase. If you see a good reason to put filenames in uppercase when creating a filelist, please explain... If there would be need, this should be done normally after. (displaying in another list in uppercase for some could be nice). But we should have a way to have the original filename intact.

As for the problem that this part of code create, that will only apply to developers that create multi-platform applications. A project that will use this on Windows, will not work on other platform because the filenames will be in uppercase.

So if you don't do application that save in Windows and open on others, you would never see this.

[CuteAlien]

It's not just about Windows/Linux but about the archives (which are used by some formats). Sorry, I don't know details without reading through that code first. I didn't write all this and so it means I have to invest some time digging through it. I just remember there had been a discusssion about this a few years ago and archives had been mentioned as reason why it was implemented like this. I agree in theory with you, but I simply can't tell without investing more time into this first to figure out what this was about. I hope just using the path class here can help in the end (it was created for exactly that reason -so we can keep original filenames around).

Sorry, it's one of those stupid tasks that needs a lot of motivation to work at. Digging through other people's old code and trying to figure out why it behaves as strange as it does and fixing it without breaking anything. So I tend to move it away as there's always enough other tasks to choose from...

[Cube_]

and that's exactly why I never volunteer as a maintainer - I'd go mad, I much prefer just ripping out code I don't like but that doesn't work if other people also work on the project.

[chronologicaldot]

First thing's first: Mesh loaders and textures don't use IFileList, and, in fact, nothing that I found directly uses it aside from the open-file dialog (I looked through all of the mesh loaders). But here's the key:

Edit: scratch the following, as after reading hasFileExtension(), I found that it converts the extension to lower case, making the comments below a moot point. I've left the text anyways for reference. Took me awhile to edit this post since apparently the forum wasn't loading for me for a few hours. Ironically, though, it does mean that hasFileExtension() will cause "different" files (".TGZ" files and ".tgz" files for example) to be treated as the same.

I looked through CFileSystem to see where archives would be affected, and it seems to be primarily in the indirect but assumed chain of user usage of the API.
IFileSystem::addFileArchive() uses IArchiveLoader, which is inherited by every special XXXXReader.h (such as CZipReader.h), which assume that all file extensions have lowercase lettering. For example, on Linux ".tgz" and ".TGZ" are "two different things", but on Windows, they are the same.
Line 238 of CZipReader.cpp uses core::hasFileExtension() (from coreutil.h), which does not ignore case and only checks the lower case version of ".tgz". This would be fine, but the assumed order of usage is:
1) Open file dialog
2) Creation of IFileList, ignore case set by operating system
3) IFileList files loaded, converted based on ignoreCase
4) File selected by user (in this case, let it be an archive)
5) IArchiveLoader requested to load file, checking features and extensions in lower_case

Obviously, then, for accuracy on the Windows version, IArchiveLoader should be told to load files with regards to ignoring the case (as it does). Ironically, on Linux, this results in a not-loaded file if someone on Windows happens to name their archive ".TGZ".
Again, the problem only comes for people trying to use files on multiple systems.

"Fixing" it then does become a sticky situation. Do you change file extensions to lower case in the file open dialog? You could have a parameter on the open file dialog for "loading with OS defaults", but that's vague and messy.

There may be more cases that just the Zip file archives. I haven't looked yet, but those are my initial findings.