Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for playnite 9 #19

Merged
merged 5 commits into from
Oct 8, 2021

Conversation

sharkusmanch
Copy link
Sponsor Contributor

@sharkusmanch sharkusmanch commented Sep 7, 2021

  • Updated for SDK changes in Playnite 9 (SDK 6.0.0)
  • A game can now have multiple platforms. For now, just use the first platform and don't backup if there is more than one platform. The current platform backup system will likely need to be reworked, but seemed out of scope for this PR.
  • Added manifest file for new built-int add-on browser/installer https://github.com/JosefNemec/PlayniteAddonDatabase. Will submit a PR to add on repo when this is merged.

@darklinkpower
Copy link

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

  • 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
  • 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
  • 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
  • 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

@sharkusmanch
Copy link
Sponsor Contributor Author

sharkusmanch commented Sep 15, 2021

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
Copy link
Owner

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.

Copy link
Owner

@mtkennerly mtkennerly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor issue, but I can take care of it after the merge. Thanks for the PR! I'll get a release out ASAP for this.

return game.Tags != null && game.Tags.Any(x => x.Name == "ludusavi-skip");
return game.Tags != null
&& game.Tags.Any(x => x.Name == "ludusavi-skip")
&& game.Platforms.Count > 1;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be || because we always want to skip if it has the tag, even if it only has one platform.

@mtkennerly mtkennerly merged commit 33294a6 into mtkennerly:master Oct 8, 2021
@mtkennerly mtkennerly added the enhancement New feature or request label Oct 8, 2021
@mtkennerly mtkennerly added this to the v0.6.0 milestone Oct 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants