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.

[ErikPrantare]

https://en.cppreference.com/w/cpp/iterator/begin
Seems like std::begin by default calls x.begin(), so should work with custom collections too.
With std::ranges::begin, it seems to first try x.begin(), and then begin(x), as per point 2 and 3 here
https://en.cppreference.com/w/cpp/ranges/begin

[btzy]

Yeah, it seems that std::ranges::begin does ADL, and on top of that also does some concept checking.

[btzy]

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.

I think we shouldn't use std::begin(), since begin() is meant to be a customization point (i.e. the proper way to get the begin of a collection is to use ADL). As to whether we should simply use ADL directly (i.e. using std::begin; begin(coll)) or use std::ranges::begin, I'm not so sure. In any case I don't think it's something we should worry about until C++ Ranges becomes more commonplace.

[ErikPrantare]

Ah yeah. Using begin() and end() should be equivalent to using their ranges equivalents.
So are we going for generic containers or iterator pairs?

[btzy]

I would prefer iterator pairs because they're more flexible and the C++ standard library is using it. I am also okay with an additional convenience function that takes in a container and uses ADL and forwards to the iterator pair version.

However, maybe it would be better to not require the type of end to be the same as begin, i.e.:

template <typename Begin, typename End>
ReturnType OpenDialog(Begin begin, End end, ...)

instead of

template <typename It>
ReturnType OpenDialog(It begin, It end, ...)

so that sentinel end iterators can be accepted too.

Also, if we're accepting any InputIterator (not just ForwardIterators), I believe we will need a specialization for the case when we get InputIterator. I think this is too much work to be justifiable, so I would recommend just mandating that users only give us ForwardIterators.

Note that C++20 continues to introduce "fixes" to existing iterator pair algorithms (e.g. https://en.cppreference.com/w/cpp/algorithm/ranges/for_each), so I think iterator pairs will continue to be a "recommended" way of doing things even after Ranges is widely adopted.

[ErikPrantare]

Yeah true, writing overloads for generic containers will be trivial if we have iterator overloads. Sounds good.

[btzy]

Just to let you know, I'm considering implementing some iterator-ish API (e.g. something like GetNextPath(prev_path)) for iterating the path set for OpenDialogMultiple. It would eliminate this known limitation "Iterating the path set on Linux when opening multiple files is an O(N²) operation because Linux uses linked lists" on the readme, and would allow the C++ bindings to have more proper iterators. I'm working on Rust bindings for NFDe and intend to consume this new iterator API.

[ErikPrantare]

Hmm okay, so would it make the functions opening multiple files/directories return an iterator instead? Would that create any issues with lifetime management? Or is the idea that you use the iterator like advance(pathIterator), as in std::advance?

[btzy]

It would probably be something like this (https://github.com/btzy/nativefiledialog-extended/pull/35/files). Only the Windows backend is implemented currently, but I've checked the Mac and GTK backends and they have a similar enumerator interface. See the function signatures and comments in nfd.h. The general idea is that you can create an enumerator that references the underlying collection, and on that enumerator there is a GetNext() function that advances the enumerator and produces the next file path.

(While implementing this, I'm starting to wonder if the Windows IShellItem::GetItemAt API that we inherited from upstream NFD is actually O(1)... maybe it is O(N), the docs don't seem to say. If it is indeed O(N), then two out of our three backends actually prefer linked-list style enumeration, which makes this addition even more justifiable.)

I'm open to suggestions about the API though. One possibility is to make it more flexible by decoupling advancing the iterator from dereferencing it (i.e. decoupling ++it from tmp = *it instead of doing it at one go like tmp = *it++). I don't think that will be useful because there is no guaranteed ordering in the path set, so one probably has to read all of them even if they just want to choose a few to use.

In C++ you should be able to implement begin() and end() member functions for a pathset, where begin() returns some iterator I, and end() (probably) returns an empty struct that satisfies the std::sentinel_for<I> concept. Then range-based for loops and C++20 algorithms will work on a pathset. I'm not sure if we should have a polyfill to make C++<20 work (since the older algorithms need end() to return the same type as begin()); I don't have any strong opinions about it.

[ErikPrantare]

If the PathSet refers to some underlying data, wouldn't it simply be able to write an iterator wrapper around a pointer, letting

Iterator(nullptr);

act as the sentinel?

begin(PathSet) == {.ptr = /* some pointer */};
end(PathSet) == {.ptr = nullptr};

advance(Iterator) == Iterator::operator++();
get(Iterator) == Iterator::operator*();

//c++:
for_each(
    begin(pathSet),
    end(pathSet),
    [](auto x) { std::cout << *x << "\n"; });

//c:
Iterator it = begin(pathSet);
while(it.ptr) {
  printf("%s", get(it));
  advance(it);
}
freePathSet(pathSet);

Which seems to look pretty nice and idiomatic for both c and c++.
The c++ classes would then be a simple wrappers around the c structs.
Should be "simple enough" and also backwards compatible while future-proof
(as it should work with ranges library).

[btzy]

If I understand you correctly, you're proposing two things:

1. Instead of a sentinel, we find an unused bit representation in the iterator type (in this case nullptr) and leverage that
2. Decouple advancing the iterator from dereferencing it

That would work for GTK, but not so sure about Windows/MacOS:

The underlying implementation of a PathSet may not always be a plain old linked list. In fact, it is only the case in GTK.

• On GTK, PathSet is a GSList, so that's a plain linked list, so the PathSet and the Iterator are essentially equivalent.
• On Windows, the PathSet is a IShellItemArray, and the enumerator is a IEnumShellItems obtained by calling the EnumItems() method of the IShellItemArray. The IEnumShellItems implementation is behind a pimpl interface so we can actually only hold a IEnumShellItems*.
• On MacOS, the PathSet is a NSArray, and the enumerator is a NSEnumerator. Similar to Windows, the NSEnumerator is behind a pimpl interface so we can only hold a NSEnumerator*.

This means that the .ptr in your implementation is never modified for Windows and MacOS; only the thing being pointed to (whose layout and size we don't know) is being modified. This means that an enumeration that has reached the end still has a non-null ptr, and we have to check if we are at the end by calling a method on the IEnumShellItems/NSEnumerator object. Having a second representation for ended enumerations (.ptr = nullptr) may make iterator equality awkward. Also, neither Windows nor MacOS seem to have a way to dereference the iterator without advancing it; they act something like C++ InputIterator, you can only read the value once.

[ErikPrantare]

Hmm, I see. Looking a little at the docs for win. Just thinking out loud:

class It {
  IShellItem* current;
  IEnumShellItems* enumerator;
public:
  It(IShellItemArray* a) : enumerator(a->EnumItems()) {
    next();
  }
  auto next() {
    auto res = enumerator->next(1, current, nullptr);
    if(res == S_FALSE) current = nullptr;
  }
  auto get() {
    return getPath(current); // Don't know how you get the actual path from an IShellItem
  }
}

I'm not sure about the semantics, but hopefully it makes sense. Do you think something like that would be possible?

[btzy]

It seems possible, though I think you'd need a few checks in case the list is empty. But I'm wondering if it's now easier to implement it directly in the C++ API instead of adding it to the C API.

It seems that the main benefit of this is to decouple advancing from dereferencing. I'm thinking this seems to be a C++ specific thing, since C doesn't have the concept of iterators (and Rust iterators have a single next() method that dereferences and advances together). Doing it this way complicates the base C API, because apart from the equivalent of NFD_PathSet_GetEnum and NFD_PathSet_FreeEnum (see nfd.h in https://github.com/btzy/nativefiledialog-extended/pull/35/files), we still need a next(), get(), and some other function to construct an ended iterator, as well as something to test if an iterator has reached the end.

Furthermore, it is possible to implement exactly what you propose directly inside the C++ header, using my proposed API in #35 (I didn't compile the following code so there might be bugs, but you get the idea):

class It {
    nfdpathsetenum_t enumerator;
    nfdnchar_t* currentPath;
  public:
    It(nfdpathset_t* a) {
        NFD_PathSet_GetEnum(a, &enumerator);
        next();
    }
    It& operator++() {
        NFD_PathSet_EnumNextN(&enumerator, &currentPath);
        return *this;
    }
    auto operator*() {
        return currentPath;
    }
};

It has a bit of extra overhead if the user doesn't dereference the iterator, but otherwise there is no other overhead. There would be negative overhead if the user dereferences the same value more than once.

[ErikPrantare]

Yeah, that looks pretty good. Haven't done much C so don't know what's idiomatic there really.
Using the enums directly seems pretty low-level/clunky, as that wouldn't be the way I'd conceptualize a collection.
At least in C++.

[btzy]

I don't think there's really a single idiomatic way to do it in C, because they have no concept of "iterators".

Also, perhaps it might still be good to have an implementation using an empty sentinel for the end iterator, perhaps via an opt-in #define macro somewhere? Since it might yield speedups and is "blessed" by C++20.

[ErikPrantare]

Hmm, because what I'm thinking is that if you want to keep the C++ API as thin as possible, you could implement the iterator in C and then just wrap that in a C++ class or something to make it compatible with algos and such. That would also make the library more uniform across the two languages, same semantics expressed differently.

Yeah having a sentinel struct would simplify some of the code I think. I don't know how noticeable any speedup would be though, compared to the rest of the operation.

[btzy]

Hmm, because what I'm thinking is that if you want to keep the C++ API as thin as possible, you could implement the iterator in C and then just wrap that in a C++ class or something to make it compatible with algos and such. That would also make the library more uniform across the two languages, same semantics expressed differently.

I don't think we should be concerned about the library being more uniform across the two languages. My concern was not about wanting to keep the C++ API thin; it is more of not wanting to force users to use the C++ abstractions (e.g. std::string, std::filesystem) if they don't want it. Since I'm not intending to deprecate the existing C++ wrapper (for those who just want automatic destruction), it's okay for a slightly more complicated C++ wrapper as long as it doesn't introduce too much overhead. By the same argument, we should not force C users to "think in terms of C++ iterators" when the underlying platform implementation (i.e. the Windows/MacOS/GTK implementation of PathSet) behaves much more like a linked list / enumeration.

Yeah having a sentinel struct would simplify some of the code I think. I don't know how noticeable any speedup would be though, compared to the rest of the operation.

I agree, the speedup will probably be quite negligible. But it feels like an unnecessary transformation from iterator+sentinel (which is standard in C++20) to iterator+iterator, since the underlying platform implementation is already essentially iterator+sentinel. In any case, such a thing can be controlled with a compilation flag, and we can make it opt-in anyway, so we can ignore this and go with iterator+iterator for now.

[ErikPrantare]

By the same argument, we should not force C users to "think in terms of C++ iterators" when the underlying platform implementation (i.e. the Windows/MacOS/GTK implementation of PathSet) behaves much more like a linked list / enumeration.

Ok, so do I understand correctly that one of the design goals for the C API is to be as close to the platforms API as possible?

[btzy]

By the same argument, we should not force C users to "think in terms of C++ iterators" when the underlying platform implementation (i.e. the Windows/MacOS/GTK implementation of PathSet) behaves much more like a linked list / enumeration.

Ok, so do I understand correctly that one of the design goals for the C API is to be as close to the platforms API as possible?

Yes, that is right :) All the overhead is only meant to abstract over platform-specific differences and provide a consistent API to the user.

Actually, the current C++ API was also written with that goal in mind.

[ErikPrantare]

If that's the case, maybe it's best to go with building other APIs around this, kind of like your original suggestion of using this as a submodule,
and then let this specific project have as a focus to provide the minimal code to accomplish the goal of a platform-agnostic API.

[btzy]

Yeah, that works too.

[ErikPrantare]

We could see it kind of like the role of the intermediate representation of compilation. We need to be able to support X platforms in Y languages. Doing it for every language directly would need X*Y implementations, but having this intermediate stage API in C/C++, it would only need X + Y implementations (one for every platform -> C, one for every C -> language). This library would then act as the translation from the different platforms to this intermediate API.