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: support thunderstore ror2mm protocol for installing mods #248

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

Conversation

Jan200101
Copy link
Contributor

@Jan200101 Jan200101 commented Aug 4, 2024

Implements #124 (as the first launcher, afaik)

details are up for discussion, this is a minimal implementation into the existing system.

2024-08-04_18-41-12.mp4

src/index.js Show resolved Hide resolved
src/index.js Show resolved Hide resolved
if (version.northstar() == "unknown")
return;

const args = process.argv.slice(app.isPackaged ? 1 : 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this for the sake of sanity
when packaged the first argument is the bundled electron app
when not packaged the first argument is electron and the second the script.
Not really needed, can remove.

Copy link
Owner

Choose a reason for hiding this comment

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

Ternary operators are the devil and not as readable imo, something like this seems better:

let args = process.argv;
if (app.isPackaged) {args.shift()}

src/modules/protocol.js Outdated Show resolved Hide resolved
This is achieved by sending an IPC event to the renderer and waiting for a reply once
Since this is async we return from the function after sending the event and recursively invoke it once the reply arrives

The package data is returned a a JSON String because Electron is unable to copy the Object over IPC
@Jan200101
Copy link
Contributor Author

the only thing I think is missing here is preventing multiple instances of viper from being open using requestSingleInstanceLock and forwarding any information to the already running process

@Jan200101
Copy link
Contributor Author

Viper now requests an instance lock, preventing multiple instances of running at once and forwarding process info to the main process.

made protocol accept an optional argv argument that defaults to process.argv and remove the slicing since it isn't really a problem. Tested it locally and it appears to work just fine with the Mod List updating aswell.

Would it be good to add a toast message when a mod is installed via the ror2mm protocol?

@0neGal
Copy link
Owner

0neGal commented Aug 14, 2024

Viper now requests an instance lock, preventing multiple instances of running at once and forwarding process info to the main process.

It may be worth also either forwarding regular CLI commands so that they get run, or displaying a message akin to "Existing Viper process found, exiting...", as from what I can tell (not able to test at this very moment), new instances simply exit without forwarding this or printing an error, personally I think the error would be better, as we'd otherwise also have to have the output of the CLI command be forwarded etc, and it would overall be a pain.

Would it be good to add a toast message when a mod is installed via the ror2mm protocol?

I would say so, there's no other hints it's been installed without that, right? If not then it would be a good idea

@Jan200101
Copy link
Contributor Author

It may be worth also either forwarding regular CLI commands so that they get run

The lock is acquired after the CLI checks, so they shouldn't be affected.
I think it only makes sense to prevent multiple instances of the GUI running, since we cannot know if the other process with the lock has any console output.

or displaying a message akin to "Existing Viper process found, exiting...", as from what I can tell (not able to test at this very moment), new instances simply exit without forwarding this or printing an error, personally I think the error would be better, as we'd otherwise also have to have the output of the CLI command be forwarded etc, and it would overall be a pain.

Alternatively, the active Viper window could be put into focus

I would say so, there's no other hints it's been installed without that, right? If not then it would be a good idea

There is a Toast after a mod was installed, which is good when the user presses the "Install" button in Viper, but when using the protocol there is nothing that states what its doing.

src/modules/protocol.js Outdated Show resolved Hide resolved
src/modules/packages.js Outdated Show resolved Hide resolved
src/app/js/mods.js Outdated Show resolved Hide resolved
src/app/js/mods.js Outdated Show resolved Hide resolved
@0neGal
Copy link
Owner

0neGal commented Aug 14, 2024

The lock is acquired after the CLI checks, so they shouldn't be affected.
I think it only makes sense to prevent multiple instances of the GUI running, since we cannot know if the other process with the lock has any console output.
...
Alternatively, the active Viper window could be put into focus

I see, yeah I think it should perhaps just do as it's doing now with the CLI args, but if it's being launched normally/with a GUI, then show a message that it's exiting and why, then focus the existing window.

There is a Toast after a mod was installed, which is good when the user presses the "Install" button in Viper, but when using the protocol there is nothing that states what its doing.

I think adding a toast for when the download is started would suffice just fine, on top of the normal "mod installed" toasts

@Jan200101
Copy link
Contributor Author

if it's being launched normally/with a GUI, then show a message that it's exiting and why, then focus the existing window.

okay uhh
what if I have Viper open and I press the "Install with Mod Manager" button? It would invoke the scheme handler which would start a second instance of viper.
The best for UX would be to focus the main instance and pass everything over.

@0neGal
Copy link
Owner

0neGal commented Aug 14, 2024

By "launched normally" I mostly meant when launching Viper by itself, and not through the scheme handler, so yeah, handle CLI args if there are any, then if no instance exists simply start normally and do what needs to be done, otherwise pass over the URLs to the existing instance and exit with some message.

@Jan200101
Copy link
Contributor Author

Changes:

  • open a dialog with the message "Viper is already running" when it was launched with no arguments (the way the user is expected to launch viper)
  • remove some pointless stuff
  • disable alert when a requested mod or mod version could not be found. This makes use ot util.format since I was unaware if there is any established way of formatting locale, please advise if there is a preferred way
  • show a toast when installing a mod through ror2mm protocol

@Jan200101
Copy link
Contributor Author

Video of this in action

2024-08-14_20-09-10.mp4

@0neGal
Copy link
Owner

0neGal commented Aug 14, 2024

disable alert when a requested mod or mod version could not be found. This makes use ot util.format since I was unaware if there is any established way of formatting locale, please advise if there is a preferred way

Currently there's no preferred way, mostly because the only place we use %s strings are in console.log() calls which do it for you, I've been meaning to add this straight to lang() but haven't yet, due to not needing it.

It may however be an idea to entirely omit the "Installing" part of the description i.e the toast would be:

Installing mod...
<package name>

Instead of:

Installing mod...
Installing <package name>

But that's open for discussion...

@Jan200101
Copy link
Contributor Author

fine by me, less strings that needs translating

Jan200101 added a commit to Jan200101/com.github._0negal.Viper that referenced this pull request Aug 17, 2024
Needed for when 0neGal/viper#248 is merged
will allow mods to be installed through the browser
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants