lmms icon indicating copy to clipboard operation
lmms copied to clipboard

PathUtil std::string support

Open messmerd opened this issue 1 year ago • 9 comments

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

messmerd avatar May 08 '24 01:05 messmerd

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 avatar May 08 '24 09:05 sakertooth

@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.

messmerd avatar May 08 '24 15:05 messmerd

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.

DomClark avatar May 08 '24 20:05 DomClark

I would prefer we prefix the UTF-8 based functions with ut8 so its clear that the result from these functions are a std::string/std::string_view storing a UTF-8 string. That being said, stuff like basePrefixQString can just be left as basePrefix, while your new function would then be named utf8BasePrefix (If we had C++20 support, we could return a std::u8string for even more clarity).

It also might be easier to wrap around the QString based 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 the QString versions. It didn't click at first, and so I thought your new functions were just duplicating the QString ones).

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.

DomClark avatar May 12 '24 21:05 DomClark

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.

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 avatar May 12 '24 23:05 sakertooth

@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.

messmerd avatar May 13 '24 05:05 messmerd

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?

Spekular avatar May 13 '24 07:05 Spekular

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.

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 avatar May 13 '24 11:05 sakertooth

@sakertooth I'll see about removing some of the other QString functions later today.

messmerd avatar May 13 '24 14:05 messmerd