Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Recently Added moved to core #94

Merged
merged 7 commits into from Apr 17, 2011

Conversation

Projects
None yet
3 participants
Contributor

amet commented Apr 6, 2011

Hi all,

here is Recently Added moved to core with a lot of help from firnsy, script.recentlyadded is not needed anymore and all functions have been mirrored as they were. no skin changes are needed apart from removing script dependency and calls to it.

few things to note:

  1. we have hardcoded it to return 5 items
  2. it needed some changes to GetRecentlyAdded* in VideoDatabase/MusicDatabase to allow us to specify the limit of items returned
  3. firnsy added announcement for profile change so that we can clean items from RA screen when profile changes
  4. it listens to announcements and updates only if library has been touched, sqlite DB only. Mysql databases systems refresh on every homescreen init until remote announcements can be delivered

thx Jezz_X for Confluence code,Montellese for VS fix, JM and spiff for other help and ronie for testing for days :)

@ghost

ghost commented Apr 14, 2011

As far as the patches go, they look good. It looks like the limit type (or lack of bounds check) problem I'd mention on IRC was already handled. I still don't really understand why we need multiple jobs though? Should the totals job run with any other the DB access will effectively serialize their tasks. It seems like excessive thread creation with little to no gain and I'm guessing the blocking could be costly when using a remote DB. Couldn't we just pass the flags into the job ctor and use that to pick which stuff to query the DB for in the work function?

I do disagree with the inclusion of this patch on the principle that this feature is exactly of the type we are trying to move OUT of core. I can't help but figure it will just be moved right back out to an addon at some point in the (hopefully near) future. Is extending the addon interface to support the bits this adds really not an option?

@ghost

ghost commented Apr 15, 2011

ofc it can be done as an add-on. but what do we gain? core would still have to issue the refresh. and i can't think of a single other usecase for such a class of add-ons.

as for starting threads, they are much cheaper than spawning a python interpreter will ever be.

Contributor

amet commented Apr 15, 2011

multiple jobs are there to handle different cases. we dont want to update all when only watched status is changing or update music RA if only one new movie has been added

@amet amet closed this Apr 15, 2011

@amet amet reopened this Apr 15, 2011

@ghost

ghost commented Apr 15, 2011

@spiff
Gains would be the ability to choose different RA implementations which provide varying degrees of info. I for one don't desire anything more than title, path, date and thumb, where I'm sure there are others who want the whole mediainfo window. Additionally, I can see there being a fair number of settings associated with this. As an addon, we don't have to worry about yet more clutter in core settings.

@amet:
I understand what they're used for, just not why they're needed. It seems the job selection logic could be moved into a single RA::DoWork() and the relevant bits of the individual jobs DoWork() selected there.

Member

jmarshallnz commented Apr 15, 2011

Personally I agree that we could drop to a single job that you pass your flag to (i.e. AddRecentlyAddedJobs would just add a single job passing the flag to the constructor) - it would save on contention with the jobmanager as really you want all the jobs specified by the flag run before you update the UI anyway. In addition it'd save locks on the database during a scan.

You could still have separate classes to do the separate things if you think that's better, though IMO separate functions is fine as well - you'd simply call each of those classes/functions in the DoWork() routine of the one job.

However I don't see a huge benefit in providing less information - after all, it's up to the skin as to what they choose to display and as we don't know this in advance (necessarily at least) it's easier just to grab the lot. Especially given that grabbing everything versus grabbing a subset has very little impact on performance, even if the sql queries were optimised to the n-th degree.

In the future, the "listing" portion of RA would be done by simply retrieving a directory, so that portion of the jobs will be going away - the "totals" portion would then be all that remains, which would be general infolabels.

Member

jmarshallnz commented Apr 15, 2011

With that taken care of it can be slotted in.

Contributor

amet commented Apr 16, 2011

@althekiller & @jmarshallnz:
is this better?

@spiff:
you still happy with it like this?

Member

jmarshallnz commented Apr 16, 2011

Yup - better IMO. Only extra thing you need is checking the return codes of UpdateVideo etc. Something like:

bool ret = true;
if (m_flag & Video)
ret &= UpdateVideo();

return ret;

etc.

Contributor

amet commented Apr 16, 2011

@jmarshallnz: and now? :)

Member

jmarshallnz commented Apr 16, 2011

Yup, fine.

On Sat, Apr 16, 2011 at 7:58 PM, amet <
reply@reply.github.com>wrote:

@jmarshallnz: and now? :)

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

@ghost

ghost commented Apr 16, 2011

Looks better, yeah. I think the check for raflag != 0 oughta be moved into CGUIWindowHome::AddRecentlyAddedJobs(), though. As it stands, its not tested in CGUIWindowHome::OnInitWindow().

With that fixed I suppose I'm fine with this going in. If it ends up bothering me enough, I'll find time to readdonify it such that it keeps everyone happy.

@ghost

ghost commented Apr 16, 2011

Looks good now.

Contributor

amet commented Apr 16, 2011

squashed some commits together, will wait for spiffs comment before pushing

@ghost

ghost commented Apr 17, 2011

fine by me

@amet amet merged commit c53be3e into xbmc:master Apr 17, 2011

FernetMenta referenced this pull request in FernetMenta/xbmc May 15, 2011

Merge pull request #94 from FernetMenta/pvr
pvr: create objects in CPVRFile when opening recordings or live streams, fixes segfault when starting recordings

popcornmix pushed a commit to popcornmix/xbmc that referenced this pull request Jan 23, 2017

Merge pull request #94 from popcornmix/mmalprime
[MMAL] Fixes for stalled video
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment