lmms
lmms copied to clipboard
PathUtil std::string support
I'm moving some of the additions from #7199 into this separate PR to make reviewing the CLAP PR easier.
- Adds UTF8-encoded std::string/std::string_view support to
PathUtil - Adds "Internal" Base location for referring to internal DSO (dynamic shared object) locations (needed for CLAP presets stored inside CLAP plugins)
- Adds new tests to RelativePathsTest
I have doubts about Windows compatibility and the use of std::string/std::string_view here. Might be more useful to utilize the std::filesystem library here (if we were to disregard the CI).
@sakertooth Windows shouldn't be an issue. When I convert from QString to std::string, I use QString.toStdString which converts to UTF-8, and when I convert from std::string to QString, I use QString::fromUtf8. Many of the std::string/std::string_view versions of PathUtil functions are just wrappers around the QString versions.
The issue with narrow strings on Windows only occurs when calling Windows API functions. As long as the strings are converted to wide strings before they're passed to the OS, there's no issue with using UTF-8 internally.
I would prefer we prefix the UTF-8 based functions with
ut8so its clear that the result from these functions are astd::string/std::string_viewstoring a UTF-8 string. That being said, stuff likebasePrefixQStringcan just be left asbasePrefix, while your new function would then be namedutf8BasePrefix(If we had C++20 support, we could return astd::u8stringfor even more clarity).It also might be easier to wrap around the
QStringbased functions rather than re-implementing them to use UTF-8 and going the other way around (Naturally, I thought you were implementing your new functions based on theQStringversions. It didn't click at first, and so I thought your new functions were just duplicating theQStringones).
I would argue the other way - the aim is eventually to remove Qt from the core, so the default choice should be to use UTF-8-encoded std::strings, with QStrings being the exception.
I would argue the other way - the aim is eventually to remove Qt from the core, so the default choice should be to use UTF-8-encoded
std::strings, withQStrings being the exception.
Then we should remove the QString ones entirely, otherwise this lacks clarity. And the fact that we aren't even returning a std::u8string, saying that we are returning a UTF-8 string in the documentation, or prefixing the function with the utf8 makes this even less clear. Doing one of these things is better than doing none of them at all. The basePrefixQString function also feels odd due to the lack of consistency, since this is practically the only function that needed this suffix because of the identical return types.
I simply don't see the point keeping the QString functions around if we intend to make such a transition. If the QString functions are the exception, remove them and let the call sites handle it with QString::fromStdString rather than having functions that have names with awkard suffixes.
@sakertooth I just removed basePrefixQString entirely, which wasn't difficult to do since it hardly had any usage outside of PathUtil internally. I also clarified that the new functions use UTF-8.
Also, could you elaborate what you mean by DSO locations? The abbreviation is not as clear to me (dynamic shared objects?).
Yes, dynamic shared objects such as .dll and .so.
CLAP presets can use a pseudo path which is used to identify a preset stored within the CLAP plugin DSO itself rather than on the file system, and by giving those paths PathUtil support, I was able to create generic preset classes that should be usable by other parts of the LMMS codebase besides just CLAP. In the future, this will allow CLAP, LV2, VST2/3, and built-in LMMS plugins to share the same preset interface and the same preset widgets so we don't need to reinvent the wheel each time. It should also allow for a consistent user experience regardless of plugin type.
Potentially out of scope: I assume the bool* error = nullptr stuff was added before we had optional support, but can't we replace it now and use options everywhere?
I just removed
basePrefixQStringentirely, which wasn't difficult to do since it hardly had any usage outside ofPathUtilinternally. I also clarified that the new functions use UTF-8.
That's better, but I'm still confused as to why we need to keep the other QString functions. Keeping both the QString functions and the std::string/std::string_view ones make statements like PathUtil::baseLookup("foobar"); ambiguous. The compiler refutes it because of the ambiguity.
Edit: Unless you want callers to explicitly specify which one to use with QString{} or std::string{}/std::string_view{}, which I think is somewhat verbose.
@sakertooth I'll see about removing some of the other QString functions later today.