Abdelrazak Younes wrote: > Georg Baum wrote: >> We avoided to use the ..W functions because that would add yet another >> string type: std::wstring on windows, which we have to convert to and >> from. > > So what? We have a lot of conversions all other the place. These > conversions are done on user actions so they don't have to be extra > fast. Doing an ucs4_to_ucs2() and using GetLongPathNameW will solve the > problem for all users, not only the users in the latin world as your > patch is doing.
Feel free to do so (after 1.5.0 I suggest), but then you have to do it in a lot of places, including using the wide version of boost::filesystem. That wide version does not work on unix (because there is no wstring API to access the filesystem in unix), so you will have to wrap it somehow. It is possible to use unicode filenames on windows, but a lot of work which I am not willing to do now, especially since the benefit will be very small AFAIK: GetLongPathNameA will use the local code page, so you are only struck if e.g. you want to use hebrew filenames in a german windows, but hebrew filenames in a hebrew windows will work. >> I am strongly against that, too. The attached patch gets rid of that and >> the result is less code with less conversions. This is more readable >> IMHO. Some more toqstr/fromqstr are traded in for less code to read. If >> you have multiple versions of internal_path() there is the danger that >> you use the wrong one and add a 'to_utf8' or something like that (this is >> the case currently, look e.g. at src/frontends/qt4/QPrefsDialog.C. > > The compiler will issue an error if you used the wrong one but anyway, I > don't care much. You misunderstood me. What I meant will not be covered by a compiler warning. Look what my patch does in src/frontends/qt4/QPrefsDialog.C: - rc.isp_pers_dict = to_utf8(internal_path(persDictionaryED->text())); + rc.isp_pers_dict = internal_path(fromqstr(persDictionaryED->text())); If you add all conversions the old version was QString -> docstring -> std::string -> docstring -> std::string The new one is QString -> std::string OK, the old version could be shortened to QString -> std::string -> docstring -> std::string if you used fromqstr() instead of to_utf8(qstring_to_ucs4()), but this is actually just another example of the problem of too many conversion paths. > My little patch solved two crashes (maybe more) and > motivated you to do some polishing, not too bad for a 10 minutes work :-) Only because It looked so wrong to me. I prefer to work for other reasons ;-) > So I suggest that we do the extra step and finish the Filename class > instead of moving little by little everytime a bug is discovered. That will not work. We cannot convert all file name utility functions to FileName, since FileName either needs to be empty or an absolute name. This is on purpose, so that functions using it can rely on that. If we want to go further we have to decide whether we want to make more use of boost::filesystem, or whether we want to use QFile internally. I designed the FileName class in such a way that both would be possible with little effort. As you can see from the other responses we don't have anzy consensus here, and I am personally not sure at all what the best solution is. Georg
