Conversation
|
Do not merge yet, still concluding my tests. |
|
@lostb1t Test Results
|
|
Do not merge yet got me wondering 😂 is he gonna read your PR? |
e18bcb6 to
6df8eb4
Compare
|
There's a reason collections items are cleared before inserting. To preserve the ordering of the catalog. |
6df8eb4 to
ca36a68
Compare
|
@lostb1t |
|
Any AI used? |
|
Only for research and boilerplate, every line was written by me. |
ca36a68 to
df77f62
Compare
|
@lostb1t I only did limited testing, but it seems to work so far. |
|
im a bit confused on what this Pr actually does different then the current code. Im aware if the empty collection folder but what other issues did you encounter that made you make this PR? |
|
There are several things mentioned in the first comment. Collections not getting created or updated: Collection getting recreated instead of updated Example: The collection never progressed which makes the global limit useless. Now it will only stop after importing the set amount of actual new items configured in the setting. Example: No safeguards for catalogs having less item than configured and constantly looping over the same items Example: Every catalog in the settings will get it's own collection (e.g. Netflix -> Netflix movies & Netflix series) Example: Collection providers can only contain one value for the same key. Now every catalog you see in the settings will get it's own collection. There were some other things, but this should give you an overview of the key changes. |
| cfg.MovieFolder = _manager.TryGetMovieFolder(cfg); | ||
| cfg.SeriesFolder = _manager.TryGetSeriesFolder(cfg); | ||
|
|
||
| cfg.MovieFolder = _manager.TryGetConfigFolder(cfg.MoviePath); |
There was a problem hiding this comment.
Whats the reason this has been moved?
There was a problem hiding this comment.
Passing the whole Plugin config just to extract a string property seems excessive.
| GelatoPlugin.Instance.SaveConfiguration(); | ||
| } | ||
|
|
||
| public CatalogConfig? GetCatalogConfig(string id, string type) |
There was a problem hiding this comment.
Whats the reason this has been moved?
There was a problem hiding this comment.
This just fetches something from the config and does not really belong in the service in my opinion. It can also be static and does not have any dependencies.
The helper can be called from anywhere without having to inject the CatalogService just for this.
| { | ||
| private const string ProviderKey = "Stremio"; | ||
|
|
||
| // TODO: Add property for "FullName" on CatalogConfig Object and use that instead of manually creating it here. |
Services/CatalogImportService.cs
Outdated
| existingCollectionIds = currentCollectionItems.ToHashSet(); | ||
| } | ||
|
|
||
| if (existingCollectionIds?.Count >= cfg.MaxCollectionItems) |
There was a problem hiding this comment.
Catalogs are dynamic, this prevents new items to shown up in collections.
There was a problem hiding this comment.
If a collection reaches the max items configured in the settings, it should not be expanded further, that seems fine to me.
Granted, the check in the beginning should be removed or at least tied to the above block as it would currently block new items from even getting imported in the library.
Proposal:
I will remove the check in the beginning.
When updating the collection I will check if the max size has been reached and then only update the collection with the newest items up to the max configured collection size.
Would that work for you?
There was a problem hiding this comment.
but that results in the exact same logic as there is now? Currently it purges all items in a collection and reinsterts.
Are you maybe talking about MaxItems instead of MaxCollectionItems?
There was a problem hiding this comment.
The MaxItems setting on the Catalogs themself are now being used to set the amount of new items (not currently present in the collection), while the MaxCollectionItems are used to limit how many items the collection will include in total (it's the upper cap).
The current logic does not use the MaxCollection items.
The MaxItems currently limits the size of the Collection instead of using the MaxCollectionItems.
If you set the MaxItems setting on the Catalog in the settings the resulting size will always be that value. If you set it to 50 and run the import twice the collection will contain 50 items.
With this change if you set it to 50 and run it twice it will contain 100.
There was a problem hiding this comment.
I changed it as proposed. The check is still there and will generate a warning, but it will continue with adding new items to the library and will update the collection with the newest amount of items configured in MaxCollectionItems.
So even if the max size has been reached the collection will be updated and contain newest items up to the configured cap.
|
|
||
| allSeenLibraryCatalogItems.Add(item); | ||
|
|
||
| if (existingCollectionIds?.Contains(item.Id) ?? true) |
There was a problem hiding this comment.
existing checks arent needed as its cleared
| var currentChildren = collection.GetLinkedChildren().Select(i => i.Id).ToArray(); | ||
| var currentChildrenSet = new HashSet<Guid>(currentChildren); | ||
|
|
||
| var newItems = allRetrievedCatalogItems |
There was a problem hiding this comment.
existing checks arent needed as its cleared
- Collections not getting created or updated - Collection getting recreated instead of updated - Collection overwriting themself - No safeguards for catalogs having less item than configured and constantly looping over the same items - No checks for existing items in the collection, leading to the same items getting imported and the collection not progressing - Import will now only stop after having imported reaching the configured amount for actual new items added to it - Every catalog in the settings will get it's own collection (e.g. Netflix -> Netflix movies & Netflix series) - Minor changes
df77f62 to
609c424
Compare
Fix catalog import: