[xbmcfanboy0]

I'm tracking down a bug and came across this function in FileItem.cpp that I can't figure out its purpose. If I avoid the call to this function from GUIMediaWindow.OnMessage (line 380) then the bug I'm hunting appears fixed. According to VS2010 Call Hierarchy, UpdateItem is only called from GUIMediaWindow.OnMessage. Any thoughts?

Code:

bool CFileItemList::UpdateItem(const CFileItem *item)
{
  if (!item) return false;
  CFileItemPtr oldItem = Get(item->GetPath());
  if (oldItem)
    *oldItem = *item;
  return oldItem;
}

[jmarshall]

It updates the content of an existing CFileItem object in a CFileItemList. "item" contains the new info. "oldItem" is the matching item in the CFileItemList. If there's a match (same path) then we overwrite the item.

And yes, it's probably called only from the GUI_MSG_UPDATE_ITEM (sent from one of the background info loaders).

[xbmcfanboy0]

Thanks. Extremely helpful reply.
I'm attempting to solve http://trac.xbmc.org/ticket/12724

I suspect the match (same path) should also verify m_lStartOffset for things like flac album image with cue.

I found and fixed 3 related spots locally. I think this may be the last one and I'm not sure how to add the additional criteria.

Here is one example where I added an additional check for m_lStartOffset.

original:

Code:

bool CGUIInfoManager::OnMessage(CGUIMessage &message)
{
  if (message.GetMessage() == GUI_MSG_NOTIFY_ALL)
  {
    if (message.GetParam1() == GUI_MSG_UPDATE_ITEM && message.GetItem())
    {
      CFileItemPtr item = boost::static_pointer_cast<CFileItem>(message.GetItem());
      if (item && m_currentFile->GetPath().Equals(item->GetPath()))
        *m_currentFile = *item;
      return true;
    }
  }
  return false;
}

new:

Code:

bool CGUIInfoManager::OnMessage(CGUIMessage &message)
{
  if (message.GetMessage() == GUI_MSG_NOTIFY_ALL)
  {
    if (message.GetParam1() == GUI_MSG_UPDATE_ITEM && message.GetItem())
    {
      CFileItemPtr item = boost::static_pointer_cast<CFileItem>(message.GetItem());
      if (m_currentFile->GetPath() == item->GetPath())
        //Required for proper handling of flac album image with cuesheet.
        if (m_currentFile->m_lStartOffset == item->m_lStartOffset)
        {
          *m_currentFile = *item;
          return true;
        }
    }
  }
  return false;
}

[xbmcfanboy0]

Here is another one of my local changes.

Original:

Code:

void CPlayList::UpdateItem(const CFileItem *item)
{
  if (!item) return;

  for (ivecItems it = m_vecItems.begin(); it != m_vecItems.end(); ++it)
  {
    CFileItemPtr playlistItem = *it;
    if (playlistItem->GetPath() == item->GetPath())
    {
      *playlistItem = *item;
      break;
    }
  }
}

New: (EDIT: removed debugging counter, formatting)

Code:

void CPlayList::UpdateItem(const CFileItem *item)
{
  if (!item) return;

  for (ivecItems it = m_vecItems.begin(); it != m_vecItems.end(); ++it)
  {
    CFileItemPtr playlistItem = *it;
    if (playlistItem->GetPath() == item->GetPath())
      //Required for proper handling of flac album image with cuesheet.
      if (playlistItem->m_lStartOffset == item->m_lStartOffset)
      {
        *playlistItem = *item;
        break;
      }
  }
}

[jmarshall]

Nice investigation.

While your fixes are correct (there's nothing wrong with an && being used :p), unfortunately m_lStartOffset is also used for the resume position stuff for videos, so I suspect doing this in general *might* be troublesome.

A solution might be to split the joint use of this by having a resume-specific field. Cue sheets would set both.

Cheers,
Jonathan

[xbmcfanboy0]

I found a couple places where StartOffset is already being used for this purpose so I rolled with it. Here is an example.

Code:

bool CFileItem::IsSamePath(const CFileItem *item) const
{
  if (!item)
    return false;

  if (item->GetPath() == m_strPath && item->m_lStartOffset == m_lStartOffset) return true;

[DDDamian]

Hey - keep at it! When you're sure of your solution (well tested) do a pull-request for a review

[xbmcfanboy0]

I made a pull request. I was told I should add a loop to CFileItemList::UpdateItem so I'm working on that now.
Extremely new to c++ so it might take me some time. Took me almost 2 weeks to get this far.

[DDDamian]

Well, another dev has looked longer, so keep at it. I'll have a look at the PR - good job

[selinajone]

Because I am not learning this respect, so don't know very well, maybe you'll run into a proficient in code, I wish you good luck

[xbmcfanboy0]

What do you think of this rewrite of the function in OP?

Code:

bool CFileItemList::UpdateItem(const CFileItem *item)
{
  if (!item) return false;

  for (int i = 0; i < (int)m_items.size(); ++i)
  {
    CFileItemPtr pItem = m_items[i];
    if (pItem->IsSamePath(item))
    {
      *pItem = *item;
      return true;
      break;
    }
  }
  return false;
}

[jmarshall]

It's fine, subject to making it build (capital S for size) and dropping the unneeded break.

Fixing it to use just the path would be more efficient though, as Get() can be done using a map lookup potentially.

[xbmcfanboy0]

Fixing it to use just the path would be more efficient though, as Get() can be done using a map lookup potentially.

Is this related to implementing a cue URI scheme?

---

Changing "size" to "Size" results in this:
class "std::vector<CFileItemPtr, std::allocator<CFileItemPtr>>" has no member "Size"

[jmarshall]

Yes, implementing a cue URI scheme. That way you can potentially take advantage of the fast lookup map (assuming the itemlist has it set).

Sorry 'bout the .size vs .Size() - I missed you had m_items. there (you could just have used for (int i = 0; i < Size(); i++)) It's normally good practice to use unsigned integers for iteration of actual vectors (or iterators).

Cheers,
Jonathan

[xbmcfanboy0]

I copied that line straight out of CFileItemList::FillInDefaultIcons(). Do you prefer I use your syntax?