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

feat: Bad mod formatting warnings #415

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

Alystrasz
Copy link
Contributor

@Alystrasz Alystrasz commented Feb 4, 2023

While loading mods, this checks if a mod.json file is present in mods directories; if not, this will display a warning message, telling user mod hasn't been installed properly.

This does not analyze hidden directories (with a filename beginning with a .).

image

Depends on (cyclic dependency):

@Alystrasz Alystrasz mentioned this pull request Feb 4, 2023
8 tasks
@Alystrasz
Copy link
Contributor Author

@GeckoEidechse if you want to keep doing your shenanigans, this will ignore your git folders 😁

@GeckoEidechse
Copy link
Member

@GeckoEidechse if you want to keep doing your shenanigans, this will ignore your git folders grin

Depending on how fast VTOL switches to using enabledmods.json we might also have to ignore the disabled folder it uses to hide the mod.json.
@BigSpice is there an ETA on the fix in VTOL? ^^

@GeckoEidechse
Copy link
Member

Actually I just realised that this might also cause issues for players where there is a leftover disabled folder in the mods dir. Not sure if Viper ever cleaned that up after switching to enabledmods.json.

It might make more sense to check folder recursively for mod.json and only warn if there is actually one in a subdirectory.

@Alystrasz
Copy link
Contributor Author

It might make more sense to check folder recursively for mod.json and only warn if there is actually one in a subdirectory.

What about mod folders with no mod.json file? (quite unlikely, I know)

@GeckoEidechse
Copy link
Member

What about mod folders with no mod.json file? (quite unlikely, I know)

For that to occur, the player must have manually deleted that file, given that a mod author wouldn't release a mod with missing mod.json cause that mod would have never loaded on their end in the first place. I'd argue that this would be a rarer occurrence then a player accidentally creating an empty folder. ^^

@Alystrasz
Copy link
Contributor Author

It might make more sense to check folder recursively for mod.json and only warn if there is actually one in a subdirectory.

Addressed in c0e44c9.

image

@BobTheBob9
Copy link
Member

concept is good, but would be nicer if this was exposed to script and shown in ui, rather than than shown as a messagebox? messagebox would be annoying in cases where mod formatting isn't an issue since it'd block execution

@Alystrasz
Copy link
Contributor Author

concept is good, but would be nicer if this was exposed to script and shown in ui, rather than than shown as a messagebox?

What method would you use to show error message in UI? All cool UI methods I know are callable from Squirrel VM.

messagebox would be annoying in cases where mod formatting isn't an issue since it'd block execution

Yeah that's also cool, because this way we're sure user knows some of their mods are fucked (in comparison with a notification that closes automatically without user noticing it)

@BobTheBob9
Copy link
Member

What method would you use to show error message in UI? All cool UI methods I know are callable from Squirrel VM.

something on the title screen to display warnings would work, would need to expose all messages to squirrel on reload and do some ui stuff in script to display them

Yeah that's also cool, because this way we're sure user knows some of their mods are fucked (in comparison with a notification that closes automatically without user noticing it)

it'd be very annoying for developers in situations where they know they'll get the warning and don't want to wait for a blocking textbox every startup, is what i'm thinking

@Alystrasz
Copy link
Contributor Author

Marking this as draft while I refactor to address comments.

@Alystrasz Alystrasz marked this pull request as draft February 7, 2023 22:47
@uniboi
Copy link
Contributor

uniboi commented Feb 23, 2023

What method would you use to show error message in UI? All cool UI methods I know are callable from Squirrel VM.

Define a global function in the UI that displays a dialog / menu / submenu that explains what's going on and call that function from native after the UI VM is initialized and mods that are formatted wrong are detected

Alystrasz and others added 2 commits February 26, 2023 22:35
expose incorrect installs to UI and fix typing
@Alystrasz Alystrasz marked this pull request as ready for review February 26, 2023 22:05
@Alystrasz
Copy link
Contributor Author

Reviews have been addressed, @uniboi implemented the UI part in R2Northstar/NorthstarMods#597.
I'm marking this as ready for reviews.

@uniboi
Copy link
Contributor

uniboi commented Feb 27, 2023

Do note that this changes the function signatures of some script methods as they are currently incorrect. R2Northstar/NorthstarMods#597 fixes those issues as well.

@ASpoonPlaysGames ASpoonPlaysGames added the depends on another PR Blocked until the PR it depends on is merged label Sep 2, 2023
@ASpoonPlaysGames ASpoonPlaysGames added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge merge conflicts Blocked by merge conflicts, waiting on the author to resolve and removed READY TO MERGE This mergeable right now labels Sep 16, 2023
@GeckoEidechse
Copy link
Member

@Alystrasz just a heads-up that this PR sadly has merge conflicts now ^^"

@Alystrasz
Copy link
Contributor Author

@Alystrasz just a heads-up that this PR sadly has merge conflicts now ^^"

Done :)

@ASpoonPlaysGames ASpoonPlaysGames added READY TO MERGE This mergeable right now and removed merge conflicts Blocked by merge conflicts, waiting on the author to resolve almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Oct 20, 2023
@ASpoonPlaysGames
Copy link
Contributor

@Alystrasz would this need a rework to check packages as well? We already have basic format requirements for packages already so maybe not but want your opinion

@Alystrasz Alystrasz added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed READY TO MERGE This mergeable right now labels Oct 22, 2023
@Alystrasz
Copy link
Contributor Author

@Alystrasz would this need a rework to check packages as well? We already have basic format requirements for packages already so maybe not but want your opinion

About packages, currently we only check if directory names match the AUTHOR-MOD-VERSION pattern, not checking if the mod manifest is at the right place; this has been added in e398b5c and 0d6b624.

I removed the "READY TO MERGE" label since the diff shows this is deleting functions such as NSGetModNames (probably some merge operation failed), I need to address that.

@Alystrasz
Copy link
Contributor Author

Waiting for @uniboi to re-add the NSGetInvalidMods function with new syntax since they've done it in the first place :)

@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Dec 20, 2023
@Alystrasz Alystrasz removed the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Nov 7, 2024
@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge depends on another PR Blocked until the PR it depends on is merged merge conflicts Blocked by merge conflicts, waiting on the author to resolve
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

7 participants