mtkennerly / ludusavi-playnite

Playnite plugin for save backups via Ludusavi
MIT License
156 stars 10 forks source link

Support for playnite 9 #19

Closed sharkusmanch closed 2 years ago

sharkusmanch commented 3 years ago
darklinkpower commented 3 years ago

Hello, sorry for interfering in the PR but I gave it a quick read and have some advice:

sharkusmanch commented 3 years ago

For matching the pc platform, it's probably a good idea to use new SpecificationIdProperty property in platforms. It should be safer and more reliable because the Platform name can be changed and the extension will stop functioning correctly if it's changed, while the specification Id is always the same. The specification Id for PC games is pc_windows https://playnite.link/docs/devel/api/Playnite.SDK.Models.Platform.html#Playnite_SDK_Models_Platform_SpecificationId

Good catch, thanks. Done

I noticed that you are indexing in the platforms property, but this will cause issues if platforms is null. You can use the null conditional operator ? as it was before to prevent this

Good catch, thanks. Done

This one is up to preference and up to what @mtkennerly decides, but maybe saving the game to a game var would be simpler to prevent so many changes from game to arg.Game

Agreed, also gives a smaller diff in this case. Done.

The new plugin template comes with changes to the ...Settings class but it was not changed for this PR. I'm not sure if settings still works without changing this but I still highly recommend to change since it's easier to work with

The settings do work. I would like to keep the scope of this PR small. Given that the settings still work, I would prefer to leave as is for now.

mtkennerly commented 2 years ago

Sorry for the delay in looking at this! I've been a bit busy personally, but I'm going to make time to review this tonight or tomorrow.