(wip) song/DirectorySongFilter: new filter

[thjbdvlt]

Adds a new filter directory similar to base but non-recursive.

See #2418

(The commit adds a GetDirectory method on LightSong struct, similar to GetURI. It could be done from within the filter functions, but I think that other (future) features could use this, e.g. group directory or list directory.)

In its current state (a partial and unoptimized copy of BasSongFilter), it's still very slow!

[MaxKellermann]

This method creates an allocated copy of the directory field, but I find that rather useless, what's the point of that copy? I think the whole method is not useful at all. You create a copy on the heap only to get a C string, but you already have a C string.

[thjbdvlt]

That's right, i remove the allocation.

I actually added the method mostly to create a string if directory is null:

if (directory == nullptr)
      return std::string("");

But it seems rather useless, I remove the method. It's also prettier with ternary operator:

  return StringIsEqual(value.c_str(), song.directory == nullptr ? "" : song.directory);

[MaxKellermann]

Please add protocol documentation

[thjbdvlt]

I'm really sorry: I thought it was easier to implement that it's actually. Thus I thought that my kinda superficial C++ knowledge would do it; it doesn't.
I've now specified "work-in-progress", and I'll come with a better proposal, maybe by suclassing or wrapping BaseSongFilter (because of functions HasOtherThanBase and others). Or maybe by adding a recursive field in BaseSongFilter if it has no impact on performance.

[MaxKellermann]

I forgot about HasOtherThanBase - but it seems this method is obsolete and can be removed; the last user was removed 8 years ago in commit 9cc960a

I thought this PR was good, I just didn't like the unnecessary allocation.

[thjbdvlt]

But it (or WithoutBasePrefix/GetBase) seems to do a great job for optimization. I didn't figured out yet how exactly and how DirectorySongFilter can take advantage of it without duplicating code.

My directory filter is much slower.

I'm probably not using MPD protocol very well (there's a loop in which I call list album base DIR, because I need an array of Albums,Artists,Directory tuples, and I would actually like to do list album group albumartist group directory), but the difference is huge; I just can't use my new filter in my client while base is instantaneous.

[MaxKellermann]

True, your code is completely unoptimized. It's a naive approach - and it works. I like naive code that just works!

But of course, it's at the same time somewhat clumsy. A search with your filter will traverse the whole database and check the URI of each and every song. The right approach would be to go to that one Directory object and apply the rest of the filter on all Song objects inside that directory. So, for the common case, your code would never actually be called; your new class would just be a marker, just like BaseSongFilter is - its code is just as well not called most of the time.

First step towards optimizing it: SongFilter::GetBase() should recognize your new class, too. That way, you get acceptable performance because only a small branch will be searched. At this point, yes, you could consider renaming BaseSongFilter to DirectorySongFilter and add the recursive flag - you had that idea, and I think it's a good idea. That would automatically give you this optimization.

Next: in DatabaseSelection::DatabaseSelection(), copy the recursive flag. Then the search will not visit subdirectories.

For experts: make your new feature work with ProxyDatabasePlugin. This requires adding the feature to libmpdclient, checking libmpdclient version at compile time, check other MPD version at runtime.

[thjbdvlt]

Thank you so much! Your hints gave me good insights into the code. This leads to a solution that's both faster and more concise.

Now I have a new question.

To access BaseSongFilter.recursive flag from within DatabaseSelection(), I need to loop over filters in the same way GetBase, HasOtherThanBase and WithoutBasePrefix do:

for (const auto &i : filter->GetItems()) {
  const auto *f = dynamic_cast<const BaseSongFilter *>(i.get());
  if (f != nullptr && !f->recursive) {
    recursive = false;
    break;
  }
}

I can either make recursive flag public and put the loop directly inside DatabaseSelection(), since it's used only once and is very small, or I can add a method to the ...Base... toolkit. A new method (e.g. HasNonRecursiveFlag) seems more consistent and helps to keep DatabaseSelection code small, but also seems a bit unnecessary.

[MaxKellermann]

Don't use negated names. Call it IsRecursive(), not HasNonRecursiveFlag(). It's too easy to get double negation wrong.
And I'd rather have this getter method than making the field public. Sure, getters are ugly boilerplate, but I tend to prefer that and making the field inaccessible. Whoever has a non-const reference to such an object should not be able to manipulate the field, as these objects are supposed to be immutable.

[MaxKellermann]

I don't like that this traverses the filter list again (was already done by SongFilter::GetBase()). This seems fragile because you never know if the two copies of that loop are looking at the same BaseSongFilter instance.
Maybe refactor SongFilter::GetBase() to something that returns both the base directory and the recursive flag? Something like:

struct Base { const char *uri; bool recursive; };
Base GetBase() ...

[thjbdvlt]

Great! I didn't like to do the same loop twice, so I'm doing this.
It also avoids a SongBase::IsRecursive() (only BaseSongFilter::IsRecursive()).

[MaxKellermann]

When you do fixups for unmerged commits, please don't add fixup commits on top; instead, amend the existing commit.
This PR has 4 commits, but they do just one single feature, therefore should result in only one commit.
A mention in NEWS would be good.

[MaxKellermann]

Rather than adding an initializer here, it should be on the declaration of the field to specify that this is the default value for all cases where none was given.
But I'm thinking we shouldn't have two constructors; only your new one should exist, and all old callers should be changed to pass the recursive flag (being true for them). When this feature exists, recursiveness should be an explicit decision with no default, because having an implicit default seems more confusing than not.

[thjbdvlt]

When you do fixups for unmerged commits, please don't add fixup commits on top; instead, amend the existing commit. This PR has 4 commits, but they do just one single feature, therefore should result in only one commit. A mention in NEWS would be good.

I'm sorry. May I rebase and push --force to have a single commit?