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

Mods can specify required Highlander version in config (reupload) #1007

Merged

Conversation

Iridar
Copy link
Contributor

@Iridar Iridar commented Jun 8, 2021

Closes #909

Replaces #955

Here's how the popup looks:

https://i.imgur.com/Mrll6e9.jpg

A slight issue with this approach is that we don't display Highlander version on the workshop, so most people who see this popup will have trouble figuring out what are they supposed to do to update the Highlander.

@Iridar Iridar added the ready-to-review A pull request is ready to be reviewed label Jun 8, 2021
@robojumper
Copy link
Member

A slight issue with this approach is that we don't display Highlander version on the workshop

Good call. We should do that.

Copy link
Member

@robojumper robojumper left a comment

Choose a reason for hiding this comment

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

Approach LGTM. This PR has some formatting issues -- much code uses spaces instead of tabs (and there is some trailing whitespace in ModDependencies.uc(198) and a double space in the signature in ModDependencies.uc(207)).

@Iridar
Copy link
Contributor Author

Iridar commented Jun 8, 2021

Approach LGTM. This PR has some formatting issues -- much code uses spaces instead of tabs (and there is some trailing whitespace in ModDependencies.uc(198) and a double space in the signature in ModDependencies.uc(207)).

Ah, probably because I copied it off Source Tree interface from the old PR rather than code itself. My bad, will get that fixed.

@Iridar Iridar force-pushed the 909-CHL-version-check-reupload branch 2 times, most recently from 6ccd37e to c810447 Compare June 9, 2021 06:14
@Iridar
Copy link
Contributor Author

Iridar commented Jun 9, 2021

Hopefully should be all fixed now, thanks for your vigilance and details.

@Xymanek
Copy link
Member

Xymanek commented Jun 9, 2021

I'm not sure about the overall value/use case of this system. The popup states:

Mod requires newer Highlander version
Please update X2WOTCCommunityHighlander

However, 99% of CHL users are using it by subscribing to it on workshop, which handles the updates on its own. In fact, they simply have no ability to affect the process, so the call to action is likely to just create confusion.

Perhaps add some sort of reference to the beta version?

@Iridar
Copy link
Contributor Author

Iridar commented Jun 9, 2021

I'm not sure about the overall value/use case of this system. The popup states:

Mod requires newer Highlander version
Please update X2WOTCCommunityHighlander

However, 99% of CHL users are using it by subscribing to it on workshop, which handles the updates on its own. In fact, they simply have no ability to affect the process, so the call to action is likely to just create confusion.

Perhaps add some sort of reference to the beta version?

Good point. The general use for this popup is to let the user know they need to switch to beta, yeah. It could also be potentially used by projects that need custom highlander (which is a practice I personally disagree with), but it looks like both of them will be released after 1.22 hits the workshop anyway, so the point is moot.

Perhaps, the popup should direct the user to the problematic mod's description, which would refer the required highlander version?

Also, not everyone uses steam workshop, people might be modding non-steam version of the game, or load highlander locally for whatever reason, so the note about needing to update the chl seems valid to me.

@Iridar Iridar added this to the 1.23.0 milestone Jul 13, 2021
@Xymanek Xymanek modified the milestones: 1.23.0, 1.24.0 Nov 23, 2021
@Iridar Iridar force-pushed the 909-CHL-version-check-reupload branch from c810447 to 2878215 Compare February 27, 2022 06:14
@Iridar
Copy link
Contributor Author

Iridar commented Feb 27, 2022

I rebased the PR to the main branch and confirmed it still works fine.

@Iridar Iridar self-assigned this Feb 27, 2022
@Xymanek
Copy link
Member

Xymanek commented Mar 2, 2022

I'll take Iridar's word on this

@Xymanek Xymanek merged commit 2f393f9 into X2CommunityCore:master Mar 2, 2022
@Iridar Iridar deleted the 909-CHL-version-check-reupload branch October 29, 2023 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-review A pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand CHModDependency functionality to check the required Highlander version for each mod individually
3 participants