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

Add modl:// support #20

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

eddoursul
Copy link

Following the commit, adding support for https:// download links, adds support for a new link schema to nxmhandler.exe:
modl://MO_GAME_ID/?url=URL

URLs are expected to be encoded using RFC 3986 (for example, with rawurlencode in PHP).

Available game IDs can be found in HandlerStorage::knownGames().

Test:
start "" "modl://falloutnv/?url=https://eddoursul.win/Cyberware%20TTW%20Patch.7z"

@Silarn Silarn requested review from Holt59, Al12rs and Silarn October 20, 2023 17:48
@Silarn
Copy link
Member

Silarn commented Oct 20, 2023

My only comment is that we may be able to consolidate some of the code since there's a fair amount of repetition.

But I think this works for a v1.

@qudix
Copy link
Contributor

qudix commented Oct 20, 2023

Any reason to keep the chrome fix? I'd imagine that problem would have been fixed years ago at this point back when chrome versions were double digits.

@Silarn
Copy link
Member

Silarn commented Oct 20, 2023

True, I doubt that's still necessary, though not really within the scope of this change. I'm not even sure if I've seen that message pop up.

@eddoursul eddoursul requested a review from Holt59 October 22, 2023 19:52
@eddoursul
Copy link
Author

I reverted automatic hijacking of the nxm association, enabling it only for modl://
More flexible association with modl requires full rewrite of main.cpp.

@Silarn
Copy link
Member

Silarn commented Oct 22, 2023

I'm confused, isn't this change now preventing NXM links from being registered by the launch arg?

There's also one other issue, which is that we're assuming NXM and MODL links should be handled the same. But that's not a good assumption. If people have a custom setup that passes some games to Vortex, we probably don't want to pass those MODL downloads.

So, I'd recommend extending the assignment display and having a secondary game assignment list depending on the schema.

@eddoursul
Copy link
Author

eddoursul commented Oct 22, 2023

I'm confused, isn't this change now preventing NXM links from being registered by the launch arg?

No, it was me who added registerProxy to the autoReg block here 2077b5e#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R249
And I reverted this change, because MO2 manages nxm association through adding handlers to nxmhandler.ini and only if there are no handlers, it offers to create the association.

@Silarn
Copy link
Member

Silarn commented Oct 22, 2023

I see, thanks.

@eddoursul
Copy link
Author

eddoursul commented Oct 22, 2023

There's also one other issue, which is that we're assuming NXM and MODL links should be handled the same. But that's not a good assumption. If people have a custom setup that passes some games to Vortex, we probably don't want to pass those MODL downloads.

modl:// calls are not directed to a current nxm handler automatically, protocol associations do not depend on each other at all.

@Silarn
Copy link
Member

Silarn commented Oct 22, 2023

No, this is not what I mean at all.

          QStringList handlerVals = storage->getHandler(url.host());
          QString executable = handlerVals.front();

This gets the target executable based on the game slug. This can technically be any executable, including Vortex. As Vortex, AFAIK, can't handle direct download links this way this would be an invalid setup. (Now, people probably shouldn't click MODL links if they use Vortex, but you never know.)

What I'm saying is we should have two lists to associate game slugs with either scheme so that MODL links are not arbitrarily sent to a non-MO2 executable.

image

@eddoursul
Copy link
Author

I can't find how Vortex would appear in nxmhandler.ini, unless user added it manually.

@Silarn
Copy link
Member

Silarn commented Oct 22, 2023

I can't find how Vortex would appear in nxmhandler.ini, unless user added it manually.

I mean yes. They would have to do it intentionally. But I've done it before and it works. You can also just use the nxmhandler interface, you don't have to edit the INI.

@Silarn
Copy link
Member

Silarn commented Oct 22, 2023

There are a lot of games Vortex manages which MO2 doesn't so allowing users to associate those games with NXM Handler is something we should support.

@eddoursul
Copy link
Author

A proper solution to this is a separate tool, managing a new INI like modlhandler.ini.
Alternatively, I can add a check if the handler is ModOrganizer.exe and show a message box, notifying user that modl is supported only by MO2.

@Silarn
Copy link
Member

Silarn commented Oct 22, 2023

I think it's absolutely doable within NXMHandler with slightly more extensive changes. But a warning is fine as a stopgap solution.

@AnyOldName3
Copy link
Member

If the protocol's not a stupid idea, a message box saying it only works with MO2 is bad as other mod managers might want to support it (side note: it might be worth tagging people like mgamerz, who work on other mod managers and might want a stay in the matter so the protocol actually ends up good).

If the protocol is a stupid idea, then supporting it is a bad idea, so it having a popup is a bad idea.

Either way, the this only works with MO2 popup is a bad idea.

@eddoursul
Copy link
Author

I'm pretty sure Vortex or any other client would show a human-readable warning about unsupported link.

@Twinki14
Copy link

Bumping this.

While @Silarn has expressed their desire to do this differently, I think it's important to have modpub support as soon as possible. ModPub is quickly becoming quite popular, and some mods are even exclusive to modpub (TTW specifically)

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.

6 participants