added MovieSets to Movie submenu #719

Merged
merged 1 commit into from Mar 29, 2012

6 participants

@mad-max

Hey Guys,

as MovieSets are now supported by scraper and XBMC it might be worth thinking about a sets entry in movie submenu...
Attached are the changes for this...
Merging this would close #10364...

Maybe hold this until eden is released?

cheers,
mad-max

@jmarshallnz
Team Kodi member

You should probably make it conditional on whether sets exist?

@mad-max

Hey jmarshall,
wow, fast answer...and yes, sounds reasonable...
Will change this accordingly

mad-max

@mad-max

Think the visible statement should be sufficient...
Alright now?

@jmarshallnz
Team Kodi member

Looks fine - mayaswell squash it down. Will be up to Jezz_X as to whether it goes in.

@JezzX
Team Kodi member

While on basic principle I have nothing against this from a design standard I wonder if the placement of it is good ? do we really want it in the middle of Genre and Years ? seems to me a better location would be after Recently added? since the main button takes you to Title anyway

Anyone else agree ?

@mad-max

The placement was orientated on my typical usage behaviour...
So recently added, and genres are more in use than sets...
For me, it's no problem to close this and submit another PR with the new placement...
Leave it up to the team guys ;-)

@MartijnKaijser
Team Kodi member

Between years and genre isn't that obvious. Think like JezzX suggested is a good way.
(note: No need to close PR I think. Just do a PR update)

@mad-max
@mad-max

Ok, just updated the commit range...Hope it's fine, now...

@jmarshallnz
Team Kodi member

Now rebase it down to a single commit.

@mad-max

damn...this seems to be the wrong way to rebase...
Will get this sorted

@jmarshallnz
Team Kodi member

Heh. I'm not sure what you've done there, but while in your moviesets branch, try

git pull --rebase origin master

That should pull origin/master into your branch and put your commits on top. Push it back to your github repo and you'll be back to your 3 commits on top. Then:

git rebase -i HEAD~3

and squash down the last two. Then check the log (git log) to make sure it all looks good and push it back up.

@mad-max

I won't get rid of the old commits...
Log shows me only my commit that I rebased...after pushing there are 28 more in history...

BTW: Thanks for your patience:-)

@mad-max mad-max added MovieSets to Movie submenu
make submenu entry for sets conditional
(now only shows up if Library.HasContent(Sets))

rearranged the position of the submenu
15179c7
@mad-max

Hah...
git pull --rebase "upstream" master
That did the trick...

@jmarshallnz
Team Kodi member

looks good. Will be pulled once Eden is done.

@mad-max

Ok, cool...
We keep my fingers off this branch until pulled grin

@JezzX JezzX merged commit 1e56203 into xbmc:master Mar 29, 2012
@JezzX
Team Kodi member

and done thanks

@JezzX
Team Kodi member

Hi Max looks like you duped me a little here :( "Library.HasContent(Sets)" does not even exist in the xbmc code so the button is always hidden

My fault really for not testing it (or maybe I can blame github auto merge :) but anyway end result is merging the code did nothing :P so I guess I'll just remove that visible condition and have people go to a blank list if they click it. but hopefully if they use TheMovieDB scraper it wont be empty

@mad-max

Hey Jezz,
that is quite strange as there is a library node for sets and the visible condition worked fine when I tested it.
Might there something be broken?
Of course I will take a look again why the hell it worked out...maybe some strange coincidence.
But sorry for this issue. Hopefully we get this sorted soon
cheers,
Max

@JezzX
Team Kodi member

Its nothing to do with the nodes its all to do with the code in GUIInfoManager.cpp and all it has is

void CGUIInfoManager::SetLibraryBool(int condition, bool value)
{
switch (condition)
{
case LIBRARY_HAS_MUSIC:
m_libraryHasMusic = value ? 1 : 0;
break;
case LIBRARY_HAS_MOVIES:
m_libraryHasMovies = value ? 1 : 0;
break;
case LIBRARY_HAS_TVSHOWS:
m_libraryHasTVShows = value ? 1 : 0;
break;
case LIBRARY_HAS_MUSICVIDEOS:
m_libraryHasMusicVideos = value ? 1 : 0;
break;
default:
break;
}
}

@mad-max

Hmm..ok. This is reasonable.
Currenty I'm trying to reproduce the fact, that the movie sets had this working visible condition.
Is there a good way to have a new bool in GUIInfoManager.cpp to have a Library.HasContent(Sets) or any other workaround? I know always enabling the button is possible, but this is something jmarshall tried to avoid?!
Can we use the option for "group movies in sets" as a visible condition?
I know this also needs to be prepared...

@JezzX
Team Kodi member

I already just made it always visible because I want to use it but not actually group my sets in the main title node. :)
Worst case is people get a empty window with a ".." item in it no real biggy like it used to be before "Always show parent item when a list is empty regardless of setting" was added and you lost navigation

@mad-max

As I said before, it must have been some coincidence why this had worked out before.
Can't get it to work again, for the obvious reason :)
Really really sorry man!
Now I have to shutdown my machine, as I'm going to marry in 4 hours and if my future wife sees my sitting at the table doing some XBMC stuff, I'm sure she will not be very amused 😁

@Memphiz
Team Kodi member

Grats for the marriage. Was nice to have you contribute your last PR here :D.

JezzX mhhh i'm not quiet sure if i like to have a submenu entry which might end in an empty list tbh. I still don't get why we don't fix this condition in core and make the submenu entry dependend on it?

@JezzX
Team Kodi member

Well I guess it just comes down too where to we draw the line?
Library.HasContent(smartplaylists)
Library.HasContent(playlists)
Library.HasContent(addons)
Library.HasContent(tvheadend)
Library.HasContent(genre,scifi)
Library.HasContent(genre,Action)

All of these are just as valid IMO more so for the first few as they actually show up in the root list of the videos window unlike movie sets

The list can just go on and on and then it starts going slower and slower, of course I'm not against adding the option just saying it may not really be needed any more than others, other than its the "in thing" right now

@mad-max

hey guys,

based on the disussion above, I think I need to follow up a bit...
jmarshall has commited Library.HasContent(Movie_Sets) here:
d38a14c

Then it's possible to make a visible condition for the sets submenu...

Will take care of it later on...

cheers,
mad-max

@mad-max mad-max deleted the mad-max:moviesets branch Mar 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment