Better caching and updating for dynamic list provider #4243

Merged
merged 2 commits into from Feb 28, 2014

Projects

None yet

4 participants

@Black09
Team Kodi member

This updates the way the items of the new dynamic directory provider are getting updated.

  • Do not reset and refresh on window close/open all the time
  • Update only on database changes

Some exceptions where no update takes place:

  • Library is currently updating
  • No corresponding items to the library update (e.g. video library update and only audio items in the list)
  • Window of the container is not active (update on next activation)
  • A plugin provides the content, it can handle updates itself
@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Feb 21, 2014
@jmarshallnz
Team Kodi member

The basic idea is sound, but I think it needs a little more care. Primarily the dynamic nature is dependent on the list you're fetching rather than the container itself, so ideally the container shouldn't need to know about whether it's dynamic or not - it should get that from it's list provider. i.e. the Announce stuff shouldn't be needed in GUIBaseContainer at all, rather it should just handle it in it's UpdateItems() routine which should already work.

The DirectoryProvider can then hold it's content over a Reset() if the content is OK to hold (obviously dependent on whether or not it's being destructed or not, so the immediately flag needs to carry over). It can register itself (if necessary) to library updates - only do so if you actually have a library folder (library:// or videodb://, musicdb:// paths), so this will need to be done based on path.

Purely from a style perspective, try and keep includes out of headers. e.g. the implementation of the new function you've added that uses StringUtils should be in the .cpp. Otherwise everyone that includes DirectoryProvider.h is also including StringUtils.h (and anything it includes).

@Black09
Team Kodi member

If the announce stuff goes into the list provider, we would need a callback to the container if an update occurs, right? Do you have some preferred way to do that? Common way would be a message to the parent window I guess.

@jmarshallnz
Team Kodi member

As things are, we won't need a callback I think, as I'm pretty sure we already check the listprovider in UpdateItems() as things are (this is how we can update the items once the background fetching is complete).

@Black09
Team Kodi member

Here's my understanding but I might be wrong so please enlighten me if I'm wrong. :)

Call UpdateListProvider() in CGUIBaseContainer:

  • Call m_listProvider->Update() which starts a new job if needed
    • Job calls OnJobComplete() which sets m_items of the list provider
  • If there was any change, call m_listProvider->Fetch() with m_items of the container
    • Adds m_items of the list provider to m_items of the container

There is some synchronization going on because Fetch() is always called immediately after OnJobComplete() (otherwise it wouldn't work) so I guess this is due to the CSingleLock?

Now I can't see how it's going to work without having the Fetch(m_items) call from the container.

@jmarshallnz
Team Kodi member

UpdateListProvider() is always done is the key (i.e. it's polled), so that ListProvider::Update() is called every frame. Thus, you need only return true once you have more content.

So basically you take care of the appropriate announcements in the directory provider class, and fire off the directory update job. Then, once that returns you set invalid as per usual and the next time through Update() the container will know it needs to update and re-fetch the items.

@Black09
Team Kodi member

All right, thanks for the info. Did an update and moved everything to the directory provider.

@jmarshallnz jmarshallnz commented on the diff Feb 22, 2014
xbmc/guilib/GUIBaseContainer.cpp
SelectItem(m_listProvider->GetDefaultItem());
+ }
@jmarshallnz
jmarshallnz Feb 22, 2014

Unrelated to changes, but no problem having them there (i.e. if you want 'em, split out to a separate cosmetic commit).

@jmarshallnz jmarshallnz commented on an outdated diff Feb 22, 2014
xbmc/guilib/GUIBaseContainer.cpp
@@ -810,8 +812,10 @@ void CGUIBaseContainer::FreeResources(bool immediately)
CGUIControl::FreeResources(immediately);
if (m_listProvider)
{
- Reset();
- m_listProvider->Reset();
+ if (immediately)
+ Reset();
+
@jmarshallnz
jmarshallnz Feb 22, 2014

Watch whitespace at EOL :)

@jmarshallnz jmarshallnz commented on the diff Feb 22, 2014
xbmc/guilib/GUIBaseContainer.h
@@ -122,7 +122,7 @@ class CGUIBaseContainer : public IGUIContainer
virtual int GetCurrentPage() const;
bool InsideLayout(const CGUIListItemLayout *layout, const CPoint &point) const;
virtual void OnFocus();
- void UpdateListProvider(bool refreshItems = false);
+ void UpdateListProvider(bool forceRefresh = false);
@jmarshallnz
jmarshallnz Feb 22, 2014

I'd tend to split this out into the cosmetic commit - saves having to analyse the commit when bisecting later on to figure out when something broke.

@jmarshallnz jmarshallnz commented on the diff Feb 22, 2014
xbmc/listproviders/DirectoryProvider.cpp
@@ -105,7 +101,7 @@ class CDirectoryJob : public CJob
initThumbLoader<CProgramThumbLoader>(PROGRAM);
return m_thumbloaders[PROGRAM];
}
-
+
@jmarshallnz
jmarshallnz Feb 22, 2014

this one into separate cosmetic commit.

@jmarshallnz jmarshallnz commented on an outdated diff Feb 22, 2014
xbmc/listproviders/DirectoryProvider.cpp
@@ -114,16 +110,19 @@ class CDirectoryJob : public CJob
boost::shared_ptr<CThumbLoader> thumbLoader = boost::make_shared<CThumbLoaderClass>();
thumbLoader->OnLoaderStart();
m_thumbloaders.insert(make_pair(type, thumbLoader));
+ m_itemTypes.push_back(type);
@jmarshallnz
jmarshallnz Feb 22, 2014

Don't we have this from m_thumbloaders ? You could iterate through the map and construct the vector in GetItemTypes() probably to save having another data member.

@jmarshallnz jmarshallnz commented on the diff Feb 22, 2014
xbmc/listproviders/DirectoryProvider.cpp
m_jobID(0)
{
assert(element);
-
@jmarshallnz
jmarshallnz Feb 22, 2014

cosmetic -> separate commit

@jmarshallnz jmarshallnz commented on an outdated diff Feb 22, 2014
xbmc/listproviders/DirectoryProvider.cpp
+
+bool CDirectoryProvider::UpdateUrl()
+{
+ CStdString value(m_url.GetLabel(m_parentID, false));
+ if (value == m_currentUrl)
+ return false;
+
+ m_currentUrl = value;
+
+ // Register this provider only if we have library content
+ RegisterListProvider(HasLibraryContent());
+
+ return true;
+}
+
+bool CDirectoryProvider::HasLibraryContent() const
@jmarshallnz
jmarshallnz Feb 22, 2014

As a matter of style, I prefer these type of functions to be static when only used locally. i.e. pass in the URL.

@jmarshallnz jmarshallnz commented on an outdated diff Feb 22, 2014
xbmc/listproviders/DirectoryProvider.cpp
+
+ return false;
+}
+
+void CDirectoryProvider::FireJob()
+{
+ CSingleLock lock(m_section);
+ if (m_jobID)
+ CJobManager::GetInstance().CancelJob(m_jobID);
+ m_jobID = CJobManager::GetInstance().AddJob(new CDirectoryJob(m_currentUrl, m_parentID), this);
+}
+
+void CDirectoryProvider::RegisterListProvider(bool hasLibraryContent)
+{
+ if (hasLibraryContent && !m_isAnnounced) {
+ m_isAnnounced = true;
@jmarshallnz
jmarshallnz Feb 22, 2014

separate line for braces

@jmarshallnz jmarshallnz commented on an outdated diff Feb 22, 2014
xbmc/listproviders/DirectoryProvider.cpp
+bool CDirectoryProvider::NeedsRefresh(AnnouncementFlag flag, const char *message)
+{
+ // we are only interested in library changes
+ if ((flag & (VideoLibrary | AudioLibrary)) == 0)
+ return false;
+
+ // we don't need to refresh anything if there are no fitting
+ // items in this list provider for the announcement flag
+ if ((((flag & VideoLibrary) != 0) &&
+ (std::find(m_itemTypes.begin(), m_itemTypes.end(), VIDEO) == m_itemTypes.end())) ||
+ (((flag & AudioLibrary) != 0) &&
+ (std::find(m_itemTypes.begin(), m_itemTypes.end(), AUDIO) == m_itemTypes.end())))
+ return false;
+
+ if (strcmp(message, "OnScanStarted") == 0 ||
+ strcmp(message, "OnCleanStarted") == 0) {
@jmarshallnz jmarshallnz commented on an outdated diff Feb 22, 2014
xbmc/listproviders/DirectoryProvider.cpp
+ (((flag & AudioLibrary) != 0) &&
+ (std::find(m_itemTypes.begin(), m_itemTypes.end(), AUDIO) == m_itemTypes.end())))
+ return false;
+
+ if (strcmp(message, "OnScanStarted") == 0 ||
+ strcmp(message, "OnCleanStarted") == 0) {
+ m_dbUpdating = true;
+ return false;
+ }
+
+ if (strcmp(message, "OnScanFinished") == 0 ||
+ strcmp(message, "OnCleanFinished") == 0 ||
+ ((strcmp(message, "OnUpdate") == 0 ||
+ strcmp(message, "OnRemove") == 0) && !m_dbUpdating))
+ {
+ m_dbUpdating = false;
@jmarshallnz
jmarshallnz Feb 22, 2014

You're going to need to reset m_dbUpdating when you remove the listener.

@jmarshallnz jmarshallnz commented on the diff Feb 22, 2014
xbmc/listproviders/StaticProvider.cpp
{
- bool changed = refresh;
+ bool changed = forceRefresh;
@jmarshallnz
jmarshallnz Feb 22, 2014

Separate commit for these

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Feb 22, 2014
xbmc/listproviders/DirectoryProvider.cpp
for (vector<CGUIStaticItemPtr>::iterator i = m_items.begin(); i != m_items.end(); ++i)
changed |= (*i)->UpdateVisibility(m_parentID);
return changed; // TODO: Also returned changed if properties are changed (if so, need to update scroll to letter).
}
+void CDirectoryProvider::Announce(AnnouncementFlag flag, const char *sender, const char *message, const CVariant &data)
+{
+ CLog::Log(LOGDEBUG, "GOT ANNOUNCEMENT, type: %i, from %s, message %s", (int)flag, sender, message);
+
+ // Fire off a new job if needed
+ if (NeedsRefresh(flag, message))
+ FireJob();
@jmarshallnz
jmarshallnz Feb 22, 2014

I wonder if instead you should flag here and trigger the job as per usual in Update(). You could switch m_invalid to a 3-way enum for example. You'll need to make sure you guard access to it in Announce ofc.

Sidenote: The NeedsRefresh() function could probably be dropped and absorbed into here, as it's all this routine is going to do. Also, drop the debug logging once you've finished debugging :)

@Black09
Black09 Feb 22, 2014

Got the log from the home window implementation so I thought it was intended to have it. :)

@Black09
Black09 Feb 22, 2014

So I did try to change m_invalid to an enum but do you really think it's worth the effort? It makes everything more complicated imo because of the bool <> enum mapping.

@jmarshallnz
jmarshallnz Feb 22, 2014
@jmarshallnz
Team Kodi member

Looking really good. I've gone through quite thoroughly, but in general keep cosmetic changes separate where you can as it reduces the scope for review and also for the inevitable re-review when we're bisecting later on for breakage :)

@Black09
Team Kodi member

Did the updates.

@t-nelson

What does this give us besides complexity?

@jmarshallnz
Team Kodi member

It makes the lists only update when they require, which basically makes then far more useful than they currently are. ATM every time you enter a window it'll re-fetch, regardless of whether that is required. Further, they don't re-fetch until you enter a window. Thus, they aren't being used currently for the homescreen widgets and the like, which are instead being done by scripts that fill in a bunch of properties (inefficient).

@t-nelson

Ok once the cosmetics are in.

I'm ripping it out of Gotham branch if we get issues. ;)

@jmarshallnz
Team Kodi member

jenkins build this please

@jmarshallnz jmarshallnz merged commit 3ada898 into xbmc:master Feb 28, 2014

1 check passed

Details default Merged build #292 succeeded in 1 hr 25 min
@Black09 Black09 deleted the Black09:directoryprovider-extensions branch Mar 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment