viper icon indicating copy to clipboard operation
viper copied to clipboard

feat: support thunderstore ror2mm protocol for installing mods

Open Jan200101 opened this issue 1 year ago • 11 comments

Implements #124 (as the first launcher, afaik)

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

https://github.com/user-attachments/assets/f424ee0b-b4c1-4c05-979e-798da8485474

Jan200101 avatar Aug 04 '24 16:08 Jan200101

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 avatar Aug 13 '24 21:08 Jan200101

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?

Jan200101 avatar Aug 14 '24 10:08 Jan200101

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

0neGal avatar Aug 14 '24 11:08 0neGal

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.

Jan200101 avatar Aug 14 '24 12:08 Jan200101

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

0neGal avatar Aug 14 '24 12:08 0neGal

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.

Jan200101 avatar Aug 14 '24 14:08 Jan200101

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.

0neGal avatar Aug 14 '24 14:08 0neGal

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 avatar Aug 14 '24 18:08 Jan200101

Video of this in action

https://github.com/user-attachments/assets/8afc17c6-b170-4a04-b243-9a538803a590

Jan200101 avatar Aug 14 '24 18:08 Jan200101

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

0neGal avatar Aug 14 '24 22:08 0neGal

fine by me, less strings that needs translating

Jan200101 avatar Aug 15 '24 20:08 Jan200101

Just as a refresher, was the only thing missing in this PR the Spanish localizations? If so, I intend to make an issue dedicated to fixing that later on, and just merging this directly...

0neGal avatar Dec 20 '24 00:12 0neGal

as far as I am aware, only the spanish localization is missing.

Jan200101 avatar Dec 20 '24 07:12 Jan200101