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

PathUtil std::string support #7246

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

PathUtil std::string support #7246

wants to merge 14 commits into from

Conversation

messmerd
Copy link
Member

@messmerd messmerd commented May 8, 2024

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

- Add std::string/std::string_view support
- Add "Internal" Base location for referring to internal DSO locations
@messmerd messmerd changed the title PathUtil UTF-8 support PathUtil std::string support May 8, 2024
@sakertooth
Copy link
Contributor

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

@messmerd
Copy link
Member Author

messmerd commented May 8, 2024

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

@DomClark
Copy link
Member

DomClark commented May 8, 2024

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.

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds UTF8-encoded std::string/std::string_view support to PathUtil

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

Adds "Internal" Base location for referring to internal DSO locations (needed for CLAP presets stored inside CLAP plugins)

Also, could you elaborate what you mean by DSO locations? The abbreviation is not as clear to me (dynamic shared objects?).

src/core/PathUtil.cpp Outdated Show resolved Hide resolved
@DomClark
Copy link
Member

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.

@sakertooth
Copy link
Contributor

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.

- Remove PathUtil::basePrefixQString()
- Clarify the usage of UTF-8
@messmerd
Copy link
Member Author

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

@Spekular
Copy link
Member

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?

@sakertooth
Copy link
Contributor

sakertooth commented May 13, 2024

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.

@messmerd
Copy link
Member Author

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

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

Successfully merging this pull request may close these issues.

4 participants