[video][music] Hide play-related context menu items for party mode pl…

[ksooo]

…aylists if not existing.

In video or music window, the node "Playlists" contains always an entry "Party mode playlist" (see screen shot below). What happens when clicking on this entry depends on whether there already was a party mode playlist created. If yes, the content of that list will be displayed in the respective window, if not, a dialog to create the playlist will be opened. If no playlist exists (yet) and you open the context menu of the "Party mode playlist" item, it should not contain any playback related items, because there is nothing to play. This is, what this PR does: hiding those context menu entries in case there is no party mode playlist yet.

Runtime-tested on macOS and Android, latest Kodi master.

@enen92 a quick code review would be nice.

[enen92]

Sorry to be the usual pain in the ass but IMO I think both of the new checks you added for video and music could be useful elsewhere in the future or at least make sense in a method of is own (reused then in IsItemPlayable). Mind creating a new method in each class HasPartyModePlaylist(item) or similar for now to make it easier to move elsewhere (if needed) in the future?

[ksooo]

Mind creating a new method in each class HasPartyModePlaylist(item) or similar for now to make it easier to move elsewhere (if needed) in the future?

@enen92 thanks for asking, but my answer is no. :-)

Let my explain: I have a multi-step strategy here, mainly for getting complexity down to something managable for me.

What I'm doing is to refactor all the playback/queue related functionality for video and music from currently lots of different source code units in a first step to two implementations, one for music, one for video. This already reduces code copies a lot.

Only if I'm done with this first step I can judge on what exactly needs to be done to do the final step, to get down from two implementation, still containing code duplicates to one base carrying the common things for video and music + two (small) specializations, one for music one for video. Until then, I will not start to factor out some of the duplicates, because this might be something that needs to be reverted later.

[enen92]

Mind creating a new method in each class HasPartyModePlaylist(item) or similar for now to make it easier to move elsewhere (if needed) in the future?

@enen92 thanks for asking, but my answer is no. :-)

Let my explain: I have a multi-step strategy here, mainly for getting complexity down to something managable for me.

What I'm doing is to refactor all the playback/queue related functionality for video and music from currently lots of different source code units in a first step to two implementations, one for music, one for video. This already reduces code copies a lot.

Only if I'm done with this first step I can judge on what exactly needs to be done to do the final step, to get down from two implementation, still containing code duplicates to one base carrying the common things for video and music + two (small) specializations, one for music one for video. Until then, I will not start to factor out some of the duplicates, because this might be something that needs to be reverted later.

Maybe I've explained myself wrong. The request is not to avoid duplicates or concentrate these checks elsewhere is just to reduce the complexity of the already existing checks. Also it doesn't go against your refactoring strategy.
The simple fact you have to add a comment on each code block pretty much makes it implicit the code itself is not easy to understand:

// Check for the partymode playlist item, nothing to play if it does not exist

^^ ok, so the 5 lines below are checking if a music party mode playlist exists...

I don't really understand why you refuse to create a simple private method (or even on an anonymous namespace) on each class where you simple name it according to what you're checking and dump those code blocks (the actual logic) inside....
I prefer to have a simple line HasPartyModePlaylist(item) (or some better naming) inside the IsPlayable check than the actual code there. It's a question of scope and manageable code units.

I.e., if I am interesting in knowing why something is not playable I am interested in knowing it is not playable because the item is an empty party mode playlist. I am not interested in knowing the business logic around what makes/means an empty party mode playlist (even more if that involves the usage of multiple subsystems).

[ksooo]

Maybe I've explained myself wrong. The request is not to avoid duplicates or concentrate these checks elsewhere

@enen92 then I indeed misread your initial comment.

I don't really understand why you refuse to create a simple private method (or even on an anonymous namespace) on each class where you simple name it according to what you're checking

Now that I understand what you mean, yes, that ofc. makes perfect sense. Will change this (anon namespace for now) and keep in mind as a pattern for the future.

[ksooo]

@enen92 I have my difficulties for this particular case to find a method name reflecting the actual functionality. We must not invert the semantics here. I need something I can early return on return false. A positive check will not work (without rewriting even more code.). A "positive check" will not be of help here. Do you have an idea?

[enen92]

@enen92 I have my difficulties for this particular case to find a method name reflecting the actual functionality. We must not invert the semantics here. I need something I can early return on return false. A positive check will not work (without rewriting even more code.). A "positive check" will not be of help here. Do you have an idea?

The reason I ask for this is because I also have difficulties understanding what you're doing. Care to explain what you are actually trying to check?

  // Check for the partymode playlist item, nothing to play if it does not exist
  if (item.IsSmartPlayList() && !CFileUtils::Exists(item.GetPath()))
  {
    const auto profileManager = CServiceBroker::GetSettingsComponent()->GetProfileManager();
    if (item.GetPath() == profileManager->GetUserDataItem("PartyMode.xsp"))
      return false;
  }

[ksooo]

The reason I ask for this is because I also have difficulties understanding what you're doing.

Well, it is exactly what that comment says: Check for the partymode playlist item, nothing to play if it does not exist. Rephrased: Only if a party mode playlist exists it can be played.

The problem is the general code flow here, which cannot easily changed. I cannot work with something like IsPartymodePlaylistExisting() return true; Because there will be other checks below that will return true after this method returned false.

Yes, maybe all bad design, but really, compared to what we had before I started this refactoring (I must be crazy), we are way better now.

Really, this is so hard to explain, and all this just for the sake of something that will be rewritten at a later point of time - for me more a theoretical thing not to understand the code like it is than something real, tbh, for this particular case. I slightly tend to just close this PR. I personally never use those playlists, so will never stumble about the problem this PR fixes. So much time for something so little - theory meets practice I'd say and really no offence intended with this. Just tired.

[ksooo]

I have a suggestion @enen92: Come up with a diff as I have no clue how to name the block of code you want me to factor out and I will happily apply the change. It is not a
"if CheckIfPlaylistExists then return true"
what is needed here. What we need is
"if item is a smart playlist and in particular it is the one and only user's party mode smart playlist and this playlist does not exists, then return false (by means of not playable).

[ksooo]

Maybe:

bool IsNonExistingUserPartyModePlaylist(const CFileItem& item)
{
  if (!item.IsSmartPlayList())
    return false;

  const std::string path{item.GetPath()};
  const auto profileManager = CServiceBroker::GetSettingsComponent()->GetProfileManager();
  return ((profileManager->GetUserDataItem("PartyMod-Video.xsp") == path) &&
          !CFileUtils::Exists(path));
}

if (IsNonExistingUserPartyModePlaylist(item)
  return false;

Fine for you?

[ksooo]

Updated PR with IsNonExistingUserPartyModePlaylist. Good to go now?