videolibrary: support manually setting dateadded through NFO and JSON-RPC

[Montellese]

This adds the long requested possibility to manually specify the dateadded property through NFOs and JSON-RPC. We have already been exporting the value to NFOs and we've also been reading it during import but we never passed it on to the database.
If nothing is specified it will still use the existing dateadded logic (based on file modification time etc.).

[razzeee]

So addons will start messing with some of my date fields and I will have modification time from normal imported mixed with my own custom timestamps?
Not a fan :/

[Montellese]

They can mess with any of your other details as well. Not sure why dateadded should be any different. Simply don't install any addons that mess with your data.

[Tolriq]

I suppose this field is handled as all other, meaning if one update only this there's still a delete insert and movieid change ?

[Montellese]

@Tolriq: This is not refresh but VideoLibrary.SetFooDetails which should not change the item's ID.

[razzeee]

yes, but this is causing inconsistencys. I would prefer our database to have columns where you know what's in there. that's why I'm working on separating dvd order from absolute order and air order. And also want to improve on the "external ids".
This is fine for now and won't make it worse, but we should really separate those fields in the database.

[Montellese]

There are a lot of people (and tools) out there which want to be able to change any property to their liking. Moving things into different tables won't change that fact. The only data that must never be touched by anyone but us our our IDs and references to other IDs. Everything else can be manually specified through NFOs, JSON-RPC etc. We don't offer them full control over what is written into the database. In this case we still make sure that we only store valid dates into the database and in the valid format.

[razzeee]

Yes, absolutely correct. But a third party addon might need to know what's set in core? Should I save the modified date or the created date? What's the setting saying?
If it can do that now, everything is good.
I have the same problem in the trakt addon, where I never know which id is in the id field. Is it a imdb id? Is it tvdb id? Is it a trakt id? Has a scraper saved something else? (same for season orders for e.g.)

[Montellese]

Rebased. jenkins build this please

[DaveTBlake]

I can see @razzeee concerns with this. We end up with a field called "dateadded" that actually contains the file timestamp unless something as modified it to any arbitrary time it wants. Not good data design.

I suspect that many people that want to modify "dateadded" because it does not actually hold the date added! The better improvement would be to rename this field "filetimestamp", only change it when the file timestamp changes, and then find out what other date these tools really want stored (and allowed to be edited).

Concerned that once we add something to JSON we aren't allowed to take it away or rename it (for backward compatibility) so would want to get this right. And of course apply the same for music.

[Tolriq]

There's dateadded to artists albums, tvshows, ... all not really related to 1 file / folder.

And date added can / is imported from nfo so also not related to files datetime.

Dateadded field is useful for importing / exporting data from instance to another while keeping this date added ordering for sorting.

From report I have too ;) Updating dateadded when timestamp change is not always wanted, specially for lot's of users that download fastsub tvshows (yes download is bad :p) then replace the files with propers later or BR / dvd rips.

About usage tons of users depend on this field for sorting, because other ordering would be item id, but they change on rescrape so mess this order.

[razzeee]

Still bad data design. If you can't know as a third party dev what's in the field. Is it modified by date, created by date, my mothers birthday? Anyway not caused or blocking this PR.

The thing I would have expected here happening would be a new field like "userdateadded", which is nullable. We should still store "dateadded" normally and just use "userdateadded" whenever it is set in place of "dateadded".

[Tolriq]

Well this is the exact same problem with playcount and lastplayed, anyone can modify those fields to mean anything. (And some tools do)

What you want is a way bigger change to architecture :) But I'd too love to see more user vs kodi datas like discussed in the DB change thread and I think @phate89 have lot's to say about that.

I'd love to see not a play count and lastplayed, but a table with all watch / played times to have an history and even could handle profiles and everything :) that would simplify offline watching / sync too :)

Edit : But I do think that GUI could auto choose to display one date or the user, but the API should expose both so that API consumer can make decision based on the data and have the choice.

[tamland]

Fyi 'dateadded' used to be the date it was added to the db, but it was changed because people wanted to 'import' their times from files. What's one more way of importing? Too late to change back to store any one thing.

[Montellese]

Yup @tamland is right. The original idea of the field was to store when the file was added to the library. But then our own (and very stupid) remove-and-add-upon-update logic kicked us in the n***s because it resulted in the value of dateadded changing and not being usable. Therefore people asked for a way to have an option for a more consistent value. The options we provide by default are

• use the same logic as before
• use the file's ctime
• use the file's mtime

With this we add an additional option that allows the user to have full control over the value. One use case is e.g. moving files around which on cetain OSes messes with the ctime and/or mtime of the file and therefore results in unwanted behaviour. For those cases users can now manually set whatever value they think makes more sense.

Assuming anything about the value in dateadded in an addon or remote is wrong. The value is mainly meant for sorting. It's not displayed anywhere in Kodi itself for that matter.

So actually it's not bad data design. You assume a definition for the value which is not valid. If you want to have access to a value like ctime or mtime consistently that needs to be added as a separate property in the database.

[phate89]

It's not better instead of providing another option to a single field to separate them in separate fields? It's the same for imdb/tvdb ids.. Kodi doesn't care where they are from but addons might use that information and they can't (or they have to do complicated stuff) because they don't know where the id comes fom..

[Montellese]

IMO it depends on the data being represented. I can fully understand the argument for the scraper IDs because Kodi uses it for internal logic and addons can use it to also retrieve additional data.
dateadded is meant to be used as a sorting value and nothing else. Splitting that information across multiple fields will make it harder to implement the sorting.
I can also see that differentiating between ratings from a scraper and user ratings makes total sense because any user will probably rate a movie differently than what has been scraped from whatever website. But having a dateadded (which is already ambiguous) and a userdateadded (for which I'm not even sure what it should mean/represent in general) just doesn't make sense to me.

Either way the main issue I have with the current situation for dateadded is that we provide NFOs as a means to completely control the information stored in the database for an item. But we violate that promise/functionality by ignoring the dateadded value we explicitly exported for the user. That is just plain wrong.

[phate89]

I agree that users should be able to set the imported date field.. But if there is request to sort from ctime/mtime it should be a separate field...

[Tolriq]

The xtime are more here to fix the delete / insert stuff that prevent any ordering based on that :(

Fixed ids in 17 or 18 would allow so many things after :)

[DaveTBlake]

The value is mainly meant for sorting. It's not displayed anywhere in Kodi itself for that matter.

FWIW there is an info label for it, and when we sort by it the "date added" value is displayed.

But our remove-and-add-upon-update logic does make it impossible for Kodi to maintain a meaningful actual "date added" to library field. So our approach is to have such a named field, default it to file time stamp (because we can't know how else to set it) but let external things set it to whatever they think a "date added" value should be? While other times having the file timestamp in the library could be useful, and it might be in "date added" or it might not....

If you want to have access to a value like ctime or mtime consistently that needs to be added as a separate property in the database.

I think we need that separate field, not modifiable by scraping etc.

So actually it's not bad data design. You assume a definition for the value which is not valid.

Well it is another example of why it would help if Kodi had a documented data design that describes what tables, fields and relationships actually are. All we have are table and field names, and clearly that just isn't enough.

Seems that we would be best to add file time stamp field(s), and have Kodi set the "date added" field to null, leaving others to use it as they will.

[Tolriq]

Nulling dateadded is not backward compatibility at all :)

About new filetime field as seen at https://github.com/xbmc/xbmc/pull/8872/files#diff-dbf978b6cac348a2af93d3beec420e43R905 current field allows multiple different values depending on settings. (Including none of the timestamp)

And such name for things not related to a single file / directory would not be very explicit. (What value for a show with multiple source directory ?)

And of course Kodi does support tons of different source type and AFAIK not all do support those values.

IMO removing dateadded as it is without a correct replacement would be bad.

[DaveTBlake]

Valid points @Tolriq, maybe nulling was a bad idea. ;)
But I would like a big sign somewhere that tells anyone coming to that data table that "date added" does not reliably contain the date the item was initially added to the library, but also does not reliably give the file time stamp because it can be set externally.

[Tolriq]

Well all points towards the main current problem that I reported to each of you guys quite a few time and individually :)

Let's try to get rid of the delete / insert nonsense :) (Not related to this PR, but the number of problem it would solve is quite big).

[razzeee]

@Montellese
I think you can go ahead and merge this (or I will if your okay with that)

[Montellese]

I'll rebase ASAP.

[Montellese]

jenkins build and merge

[Montellese]

Win32 build had a jenkins internal problem. jenkins build and merge