Repository: xbmc/xbmc
File: xbmc/interfaces/AnnouncementManager.cpp (C++)

Code before:
  int id = tag.GetDatabaseId();
  objItem["type"] = MediaTypeSong;

  //! @todo Can be removed once this is properly handled when starting playback of a file
  if (id <= 0 && !item.GetPath().empty() && ShouldLookup(item))
  {
    CMusicDatabase musicdatabase;
    if (musicdatabase.Open())
    {
      CSong song;
      if (musicdatabase.GetSongByFileName(item.GetPath(), song, item.GetStartOffset()))
      {
        tag.SetSong(song);
        id = tag.GetDatabaseId();
      }

      musicdatabase.Close();
    }
  }

  if (id <= 0)
  {
    //! @todo Can be removed once this is properly handled when starting playback of a file
    item.SetProperty(LOOKUP_PROPERTY, false);

    objItem["title"] = tag.GetTitle();
    if (objItem["title"].empty())
      objItem["title"] = item.GetLabel();

    if (tag.GetTrackNumber() > 0)
      objItem["track"] = tag.GetTrackNumber();
    if (!tag.GetAlbum().empty())
      objItem["album"] = tag.GetAlbum();
    if (!tag.GetArtist().empty())
      objItem["artist"] = tag.GetArtist();
  }
  else
  {
    objItem["id"] = id;
  }
}

CVariant CreateDataObjectFromItem(const CFileItem& item, const CVariant& data)
{
  CVariant object = data.isNull() || data.isObject() ? data : CVariant::VariantTypeObject;

  if (item.HasPVRChannelInfoTag())
  {
    auto copyPlayerId = data.isMember("player") && data["player"].isMember("playerid");
    CopyPVRTagInfoToObject(*item.GetPVRChannelInfoTag(), copyPlayerId, object);
  }

Review comment (suggestion):
As far as I can see `item`is a local (!) copy of the const item passed in. So, what sense does it make to set the property at an instance that will be destructed at the end of this function? I do not understand the local copy of the item at all, tbh. 

The original code sets the prop at a `shared_ptr` to a non-const item, which seems to make sense.

Code after:
  //! @todo Can be removed once this is properly handled when starting playback of a file
  if (id <= 0 && !item.GetPath().empty() && item.GetProperty(LOOKUP_PROPERTY).asBoolean(true))
  {
    CMusicDatabase musicdatabase;
    if (musicdatabase.Open())
    {
      CSong song;
      if (musicdatabase.GetSongByFileName(item.GetPath(), song, item.GetStartOffset()))
      {
        tag.SetSong(song);
        id = tag.GetDatabaseId();
      }

      musicdatabase.Close();
    }
    else
    {
      CLog::LogFC(LOGWARNING, LOGANNOUNCE,
                  "Unable to open music database. Can not load song tag for announcement!");
    }
  }

  if (id <= 0)
  {
    //! @todo Can be removed once this is properly handled when starting playback of a file
    item.SetProperty(LOOKUP_PROPERTY, false);

    objItem["title"] = tag.GetTitle();
    if (objItem["title"].empty())
      objItem["title"] = item.GetLabel();

    if (tag.GetTrackNumber() > 0)
      objItem["track"] = tag.GetTrackNumber();
    if (!tag.GetAlbum().empty())
      objItem["album"] = tag.GetAlbum();
    if (!tag.GetArtist().empty())
      objItem["artist"] = tag.GetArtist();
  }
  else
  {
    objItem["id"] = id;
  }
}

CVariant CreateDataObjectFromItem(CFileItem& item, const CVariant& data)
{
  CVariant object;
  if (data.isNull() || data.isObject())
    object = data;
  else
    object = CVariant::VariantTypeObject;