More C++y API

[ErikPrantare]

The C++ wrapper atm is still (understandabley) pretty C-y.
Some things that could be improved would be to use std::basic_string instead of unique_ptr,
and to use std::filesystem::path to represent paths.

I understand that this could possibly mean API breaks, so I'm wondering how backwards compatible this project tries to be?
We could also use [[deprecated(reason)]] to faze out old functions if that would help.

One issue could possibly be the need to maintain two separate APIs for C and C++, as they'd diverge more.

I'm thinking about forking this and just do away with all the C, but I'd rather contribute to an already existing repo if it's possible :^)

[btzy]

The C++ wrapper is meant to confer RAII semantics rather than using specific C++ types (e.g. std::basic_string and std::filesystem::path) - this ensures that users remember to free memory in the correct way. It is also intended to be a thin wrapper (zero-cost, or at most constant-cost) over the raw C interface.

I would prefer not to use std::basic_string for these reasons:

• We will be forcing the user to use std::basic_string. Some applications might have a third party struct to represent strings, or be just manipulating them as pointers. Raw C-style strings are used here because the underlying system calls also use null-terminated C strings, making it the 'de-facto' choice.
• We will need to copy the bytes from the raw C buffer into the string.

I would prefer not to use std::filesystem::path for the same reasons as above. Furthermore, it requires C++17, and many applications and games also prefer not to use the standard filesystem library.

I'm opposed to replacing the current C++ API by your proposal, but am neutral to adding a separate header file (e.g. nfd_stdc++.hpp) for people who want to write code using the proper committee-sanctioned C++ standard library.

[ErikPrantare]

I got the impression that this project uses C++17. The following line should probably be edited in that case:

nativefiledialog-extended/CMakeLists.txt

Line 24 in d1b8b19

set (CMAKE_CXX_STANDARD 17)

std::basic_string::c_str accesses the underlying C-string, effectively acting the same way unique_ptr::get would in this case.
You are correct however that this would require copying the bytes.
Is this reasonably going to be a bottleneck in any program?
It's a quick operation compared to selecting the files to begin with.
But yeah, it wouldn't be zero cost.

[btzy]

I think set (CMAKE_CXX_STANDARD 17) should use the latest available C++ standard if C++17 is not available (at least, that should be the intention, anyway). So it isn't really a hard requirement if they use some slightly older compiler. I think the project can compile with C++98 currently.

It's unlikely to be a performance bottleneck in any case, but it would make this library opinionated (in the sense that it forces users to convert to/from std::basic_string unnecessarily even if they use something else in their application). Having std::string/std::wstring APIs may be useful, but I don't really find it justifiable to deprecate/remove the current "lower level" APIs since I don't think the current APIs are strictly worse than std::string/std::wstring.

[ErikPrantare]

Checking the CMake docs, you're right.
However, I think we may want to use the CXX_STANDARD property to not interfere with other CMake code (not related to this issue).
Also, setting it to 98 would signal that the library aims to be backwards compatible with older standards.
Btw, isn't std::unique_ptr C++11 specific? https://en.cppreference.com/w/cpp/memory/unique_ptr

I don't think it would be more opinionated than using a std::unique_ptr,
but it'd would make more sense in C++ code as char* generally is discouraged for string representation.
The default would probably be to want a std::basic_string,
and to use std::basic_string::c_str or std::basic_string::data if you actually wanted the underlying char*
(the same way you have to do with std::unique_ptr::get now).

I think adding another C++ API on top of the existing 2 would risk making stuff painful to maintain,
not to mention confusing to learn.
At that point, it's probably a better idea to simply fork out the new API.

[btzy]

However, I think we may want to use the CXX_STANDARD property to not interfere with other CMake code (not related to this issue).
Also, setting it to 98 would signal that the library aims to be backwards compatible with older standards.

I'm not too sure what you mean - it appears that CMAKE_CXX_STANDARD sets the default value of CXX_STANDARD on all targets, which is what we want here. This option will be set on NFDe only, and not your project that uses this library, according to this StackOverflow answer.

Btw, isn't std::unique_ptr C++11 specific? https://en.cppreference.com/w/cpp/memory/unique_ptr

Yeah that's right. But I believe the CMAKE_CXX_STANDARD option that we set only applies to the code that is actually compiled into a static library, which excludes that .hpp file. The standard version used to compile that nfd.hpp will depend on the project that includes it.

I don't think it would be more opinionated than using a std::unique_ptr,
but it'd would make more sense in C++ code as char* generally is discouraged for string representation.
The default would probably be to want a std::basic_string,
and to use std::basic_string::c_str or std::basic_string::data if you actually wanted the underlying char*
(the same way you have to do with std::unique_ptr::get now).

It would be the default in most small applications, but there are still many situations where the default string representation isn't std::basic_string. For example, SFML uses its own string class. Furthermore, the application might already be optimizing allocations in some manner (e.g. concatenating strings into the same block of memory).

I think adding another C++ API on top of the existing 2 would risk making stuff painful to maintain,
not to mention confusing to learn.

I'm not fully sure if I agree or disagree with this, so I'm currently neutral to having a second C++ API. But I still do not think that replacing the current API is a good thing to do.

At that point, it's probably a better idea to simply fork out the new API.

Perhaps you could consider git submodules if you would like to get future bug fixes, like what this repo does.

[ErikPrantare]

I'm not too sure what you mean - it appears that CMAKE_CXX_STANDARD sets the default value of CXX_STANDARD on all targets, which is what we want here. This option will be set on NFDe only, and not your project that uses this library, according to this StackOverflow answer.

Nice, now I know :)

Good idea about submodules, may try that.

[ErikPrantare]

I implemented a basic wrapper around the API in my project using nfde. Thought I'd show you how it looks/what I had in mind:

namespace util::nfd {
namespace fs = std::filesystem;
using string = fs::path::string_type;

struct FilterItem {
    string niceName;
    string suffixes;
};

[[nodiscard]] auto
saveDialog(
        std::vector<FilterItem> const& filterItems,
        fs::path const& startPath,
        string const& defaultName) -> std::pair<fs::path, nfdresult_t>;

[[nodiscard]] auto
openDialog(
        std::vector<FilterItem> const& filterItems,
        fs::path const& startPath) -> std::pair<fs::path, nfdresult_t>;

[[nodiscard]] auto
openDialogMultiple(
        std::vector<FilterItem> const& filterItems,
        fs::path const& startPath)
        -> std::pair<std::vector<fs::path>, nfdresult_t>;
}    // namespace util::nfd

auto
operator""_nfd(char const* str, size_t size)
        -> std::filesystem::path::string_type
{
    return {str, str + size};
}

Used like this:

auto
loadTerrain(
        Terrain& terrain,
        ShaderController& shaderController,
        ShaderProgram& shaderProgram) -> void
{
    namespace fs = std::filesystem;

    std::vector<util::nfd::FilterItem> filterItems{
            {"Terrain files"_nfd, "lua,frag"_nfd}};

    auto const [paths, result] = util::nfd::openDialogMultiple(
            filterItems,
            fs::current_path() / "presets");

    if(result != NFD_OKAY) {
        return;
    }

    for(auto const& path : paths) {
        if(util::endsWith(path.native(), "shape.lua"_nfd)) {
            std::ifstream in(path);
            terrain.loadLua(util::getContents(in));
        }
        // ...more code...
    }
}

[btzy]

Sorry, I was quite busy for the past few days. Indeed, this API looks quite concise, and it is nice that std::filesystem::path is internally stored as strings using the native character type. It does seem like a net positive to have this kind of API in NFDe, though I'm still wary of removing the current C++ API, for those who do not want to use C++ stdlib types.

I'm also wondering if the vector should be changed to a pair of iterators instead, so that it can also accept other iterable types (this won't add any overhead since in all cases we need to build the array of nfdnfilteritem_t to call the C API):

template <typename InIt>
[[nodiscard]] auto
saveDialog(
        InIt filterItems_begin,
        InIt filterItems_end,
        fs::path const& startPath,
        string const& defaultName) -> std::pair<fs::path, nfdresult_t>;

[ErikPrantare]

Sorry, I was quite busy for the past few days. Indeed, this API looks quite concise, and it is nice that std::filesystem::path is internally stored as strings using the native character type. It does seem like a net positive to have this kind of API in NFDe, though I'm still wary of removing the current C++ API, for those who do not want to use C++ stdlib types.

We should probably then teach the simpler API first, and refer to other documentation for those who
want to use the old one.

I'm also wondering if the vector should be changed to a pair of iterators instead, so that it can also accept other iterable types (this won't add any overhead since in all cases we need to build the array of nfdnfilteritem_t to call the C API):

template <typename InIt>
[[nodiscard]] auto
saveDialog(
        InIt filterItems_begin,
        InIt filterItems_end,
        fs::path const& startPath,
        string const& defaultName) -> std::pair<fs::path, nfdresult_t>;

I agree that we should be agnostic about the input container.
We could also go for using a template over the container type, like

template<class Container>
[[nodiscard]] auto
saveDialog(Container const& filterItems, ...)  -> std::pair<fs::path, nfdresult_t>;
{
    // use std::cbegin(filterItems), std::cend(filterItems)
}

This would eventually be pretty nice when C++20 comes around to be viable,
as we could use this concept for the Container parameter:
https://en.cppreference.com/w/cpp/ranges/range

Should be possible to emulate that concept already using some SFINAE magic...

[btzy]

We should probably then teach the simpler API first, and refer to other documentation for those who
want to use the old one.

Currently the documentation (i.e. contract) for the API is in comments in the header file itself. I would think that is reasonable, though it might be useful to have an additional page of documentation (in markdown) for each header file.

We could also go for using a template over the container type

I'm not really sure if templating on the input container would be good, since sub-slices cannot be represented by a standard container type until C++20. The pair of iterators seem to be more "idiomatic" C++, even in C++20 unless the user is manipulating ranges.

I'm not too keen on trying to emulate concepts here though, since that may confuse people who try to read the header file. In any case, in C++<20, we would just let the compiler produce long-ish error messages when instantiating templates with incorrect types, so there might not be reason to deviate from the "standard" way of doing things. Just saying that begin and end must satisfy ForwardIterator in the comments/documentation should be enough.

[ErikPrantare]

I don't think I can think of a situation where you'd want a sub-slice of a collection here.
In any case, I also think iterators is the way to go.
Maybe we'll deprecate this sometime in 2023+ when concepts are more widely supported ;^)
But for now at least, iterators is the KISS way.

[btzy]

I don't think I can think of a situation where you'd want a sub-slice of a collection here.

Maybe the user wants to display a prefix of some list of filters, depending on the game settings?

Maybe we'll deprecate this sometime in 2023+ when concepts are more widely supported ;^)

Yeah that's possible, by then we would have more clarity on whether using std::ranges should be the default way to do things. Right now I'm not sure if it's best practice to use std::ranges instead of stuff in the <algorithm> header... I'm not really sure why they are using a separate sub-namespace for ranges. In any case, if the iterator pair functions in <algorithm> aren't deprecated, I see no reason to deprecate/remove the iterator pair functions from NFDe.

[ErikPrantare]

The main reason to deprecate would be for simpler maintenance. I don't think iterator pairs will be seen as "wrong" before like 2030 though.

I think the reason for the sub-namespace was to be able to redefine old functions, such as std::ranges::transform. It's probably a lot easier both for language lawyers and implementers than to retrofit old functions to work with ranges too.

[btzy]

That's possible. I guess I'm not certain we should pull in <ranges> and force the user to convert their collections into ranges when we're just going to convert them back. Do you know what difference it will make if we just take in any (collection) type as an argument and then use ADL begin(coll) and end(col) to get the iterator pair?

[ErikPrantare]

No big difference as far as I can see. One thing to keep in mind is that begin vs std::begin is more pemissive, so it would be easier to switch from using std::begin if we would see a need for it, than the other way around.