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.

Sign up for GitHub

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

Jump to bottom

Add UI Notification for Invalid Mod Installs #597

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

Add UI Notification for Invalid Mod Installs #597

wants to merge 26 commits into from

Conversation

uniboi
Copy link
Contributor

@uniboi uniboi commented Feb 26, 2023 •
edited
Loading

F1F7Y
F1F7Y reviewed Feb 26, 2023
View reviewed changes
Copy link
Member

@F1F7Y F1F7Y left a comment

Choose a reason for hiding this comment

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

Code looks good, might be worth putting the invalid mods at the start of the list so that users cant miss them

Northstar.Client/mod/resource/northstar_client_localisation_english.txt Show resolved Hide resolved
@BobTheBob9
Copy link
Member

haven't tested, but ui looks great!

Alystrasz
Alystrasz requested changes Feb 26, 2023
View reviewed changes
Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Code looks good, works along R2Northstar/NorthstarLauncher#415 with one or several fucked mods.
I however have some requests regarding UI.

Northstar.Client/mod/scripts/vscripts/ui/menu_ns_modmenu.nut Show resolved Hide resolved
Northstar.Client/mod/resource/northstar_client_localisation_english.txt Outdated Show resolved Hide resolved
Northstar.Client/mod/resource/ui/menus/panels/toast_popup.res Show resolved Hide resolved
Northstar.Client/mod/scripts/vscripts/ui/panel_mainmenu.nut Outdated Show resolved Hide resolved
@Alystrasz Alystrasz mentioned this pull request Feb 27, 2023
Open
1 task
@uniboi
Copy link
Contributor Author

uniboi commented Feb 27, 2023

The compile check fails because some exposed function signatures have been corrected in R2Northstar/NorthstarLauncher#415.

@GeckoEidechse
Copy link
Member

The compile check fails because some exposed function signatures have been corrected in R2Northstar/NorthstarLauncher#415.

In that case you can already update the signatures in .github/nativefuncs.json. The mods CI technically does not depend on launcher code (but should obviously be kept in sync :P)

@uniboi
Copy link
Contributor Author

uniboi commented Feb 28, 2023 •
edited
Loading

@GeckoEidechse panel_mainmenu.nut is a new vanilla file and nativefuncs are updated

@F1F7Y F1F7Y added the waiting on changes by author Waiting on PR author to implement the suggested changes label Apr 20, 2023
@uniboi uniboi requested review from F1F7Y and Alystrasz May 7, 2023 17:21
@uniboi uniboi added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code and removed waiting on changes by author Waiting on PR author to implement the suggested changes labels May 7, 2023
F1F7Y
F1F7Y approved these changes May 8, 2023
View reviewed changes
Copy link
Member

@F1F7Y F1F7Y left a comment

Choose a reason for hiding this comment

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

Code looks good, tested with Alystrasz/NorthstarLauncher#4

Alystrasz
Alystrasz requested changes May 9, 2023
View reviewed changes
Copy link
Contributor

@Alystrasz Alystrasz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks good, confirmed working on Ubuntu 23.04.

image

I'd like to see mod.json file mentioned in error messages though.

Northstar.Client/mod/resource/northstar_client_localisation_english.txt Outdated Show resolved Hide resolved
@uniboi uniboi requested a review from Alystrasz May 10, 2023 07:53
Alystrasz
Alystrasz approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Good feature, this will definitely help people understand how they can fix their Northstar install.

ASpoonPlaysGames
ASpoonPlaysGames approved these changes May 18, 2023
View reviewed changes
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

image
works in testing, notification appears and all works fine.

Code looks good, moving mod browser and stuff to use structs instead of loads of native functions really helps clean up the code

Northstar.Client/mod/resource/northstar_client_localisation_english.txt Show resolved Hide resolved
@uniboi uniboi added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels May 30, 2023
@uniboi uniboi added the needs testing Changes from the PR still need to be tested label Jul 14, 2023
@F1F7Y F1F7Y added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Jul 26, 2023
@ASpoonPlaysGames
Copy link
Contributor

@uniboi this is currently broken due to outside changes using native functions that are removed in this PR. (also merge conflicts)

@ASpoonPlaysGames ASpoonPlaysGames added waiting on changes by author Waiting on PR author to implement the suggested changes and removed almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Aug 25, 2023
@uniboi
Copy link
Contributor Author

uniboi commented Aug 26, 2023

I'm not sure if this is even needed anymore. I haven't seen someone manually installing mods incorrectly in a while. Maybe most people are using mod managers now?

@EnderBoy9217
Copy link
Contributor

I'm not sure if this is even needed anymore. I haven't seen someone manually installing mods incorrectly in a while. Maybe most people are using mod managers now?

A vast amount of people dont use mod managers and screw it up

Plus, this extra info doesnt hurt

@ASpoonPlaysGames ASpoonPlaysGames added the depends on another PR Blocked until the PR it depends on is merged label Sep 2, 2023
@Alystrasz
Copy link
Contributor

@uniboi I think your branch is outdated, the feature does not work anymore

@GeckoEidechse
Copy link
Member

@uniboi there's currently some merge conflicts.

@GeckoEidechse
Copy link
Member

@uniboi bump

@GeckoEidechse
Copy link
Member

bump

@Alystrasz
Copy link
Contributor

Alystrasz commented Nov 7, 2024 •
edited
Loading

@uniboi 'sup?

[EDIT]: Launcher counterpart PR (R2Northstar/NorthstarLauncher#415) is now up-to-date with main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alystrasz Alystrasz Alystrasz approved these changes

@ASpoonPlaysGames ASpoonPlaysGames ASpoonPlaysGames approved these changes

@F1F7Y F1F7Y F1F7Y approved these changes

Assignees
No one assigned
Labels
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 needs testing Changes from the PR still need to be tested waiting on changes by author Waiting on PR author to implement the suggested changes
Projects
Northstar Roadmap
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@uniboi @BobTheBob9 @GeckoEidechse @ASpoonPlaysGames @EnderBoy9217 @Alystrasz @F1F7Y