Getdirectory: Use flags rather than a bunch of bools #658

Merged
merged 3 commits into from Apr 6, 2012

Conversation

Projects
None yet
2 participants
Member

jmarshallnz commented Jan 26, 2012

This does exactly what it says: implements all the bools (except the "allow threading" one) in CDirectory::GetDirectory as flags instead.

The defaults for the flags are the same as the previous defaults, so this is mostly code-shuffle. I've carefully reviewed each change, but still might have missed one - just running through and comparing the previous defaults with what is there now.

The second commit is a change in behaviour. Basically I reviewed cases where we were setting allowThreads to true. Anything that was either already threaded, or should be threaded through different means (eg throw off a job to do the whole function) was removed. The remaining cases are UI dir fetches, slideshow dir fetches, and smartplaylist dir fetches.

The last commit is removing a clearing of the cache during subtitle fetching which was a left over from the refactor that caused a bunch of confusion in the beginning.

One final thing for consideration: Are the defaults sane? Primarily, should we be fetching filedirectories by default with every CDirectory::GetDirectory() call? This is currently the largest flag that we specify (turning off that feature) and I suspect there's a bunch of places where we're needlessly doing this.

Not for Eden.

Member

elupus commented Jan 26, 2012

I already had something very similar in my hdmv blueray branch cued up for
after dharma. Structure instead of flags since I needed more options.

Member

jmarshallnz commented Jan 26, 2012

Could easily do a combination thereof by making the hints structure take a flag in it's constructor to keep most calls reasonable:

CDirectory::GetDirectory(foo, items, mask, hints(flags, content), allowThreads)

Or, given you're only using it as a bool switch atm, you could just add another flag :)

Member

elupus commented Jan 26, 2012

Well the point was to pass along the content type directly without need to if(this) if(that).. I don't mind the flags approach per say, just the struct idea give us more options in the future.

Member

jmarshallnz commented Jan 26, 2012

Yeah, perhaps a combination might be the way to go where the hints struct has a constructor that takes the flags.

Member

jmarshallnz commented Apr 6, 2012

@elupus: are you happy if I switch to using a struct of flags combination? That way we can easily add to the hints struct later should more than just a flag be required.

Member

elupus commented Apr 6, 2012

My code need the scanning content type, so the flags wont do. Would be
nicer to get the structure in directly. Can't you combine the two.
Structure in the lower layers at least. Then I don't have to rework same
code, just add the content type.

Member

jmarshallnz commented Apr 6, 2012

Yup - I wasn't clear. What I meant was:
{{{
struct Hints
{
Hints(int f) : flags(f);
int flags;
};

bool GetDirectory(const CStdString &path, const CStdString &mask, const Hints &hints);
}}}

Member

elupus commented Apr 6, 2012

Mask into that too IMHO. But yea that will be fine.
On Apr 6, 2012 12:18 PM, "jmarshallnz" <
reply@reply.github.com>
wrote:

Yup - I wasn't clear. What I meant was:
{{{
struct Hints
{
Hints(int f) : flags(f);
int flags;
};

bool GetDirectory(const CStdString &path, const CStdString &mask, const
Hints &hints);
}}}


Reply to this email directly or view it on GitHub:
#658 (comment)

Member

jmarshallnz commented Apr 6, 2012

Ok, rebased and now using a class - left the flags method for ease of access, and left allowThreads out of the hints as it's only used at CDirectory level rather than at IDirectory.

@ghost ghost assigned jmarshallnz Apr 6, 2012

@@ -282,7 +282,6 @@ bool CGUIWindowVideoNav::GetDirectory(const CStdString &strDirectory, CFileItemL
if (m_thumbLoader.IsLoading())
m_thumbLoader.StopThread();
- m_rootDir.SetCacheDirectory(DIR_CACHE_ONCE);
@elupus

elupus Apr 6, 2012

Member

Unrelated?

@jmarshallnz

jmarshallnz Apr 6, 2012

Member

It defaults to DIR_CACHE_ONE (and the function is now gone), so nope :)

Member

elupus commented Apr 6, 2012

Seems good to me.

jmarshallnz added a commit that referenced this pull request Apr 6, 2012

Merge pull request #658 from jmarshallnz/getdirectory_flags
Getdirectory: Use flags rather than a bunch of bools

@jmarshallnz jmarshallnz merged commit 426c1cf into xbmc:master Apr 6, 2012

tru added a commit to plexinc/plex-home-theater-public that referenced this pull request Nov 12, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment