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

Replace max KSP version empty string with "any" in GUI #2420

Merged
merged 2 commits into from
Apr 25, 2018
Merged

Replace max KSP version empty string with "any" in GUI #2420

merged 2 commits into from
Apr 25, 2018

Conversation

DasSkelett
Copy link
Member

@DasSkelett DasSkelett commented Apr 22, 2018

If there's no max KSP version defined in a mod's metadata, CKAN GUI displays it as an empty string.
See maxV1

It would be prettier and more informative if there is some text, so I changed the empty string to "N/D" "any".
This applies to the lower right metadata box and also the column in the modlist, as you can see here:
maxV2

Replace empty strings for max KSP version with N/D
@DasSkelett DasSkelett changed the title Replace empty string with N/D Replace max KSP version empty string with N/D Apr 22, 2018
@HebaruSan
Copy link
Member

I guess I don't find this clearer. Users are going to wonder what the N and D stand for.

@DasSkelett
Copy link
Member Author

Would N/A be better?

@DasSkelett
Copy link
Member Author

Or "Unlimited" ?

@HebaruSan HebaruSan added the GUI Issues affecting the interactive GUI label Apr 23, 2018
@HebaruSan
Copy link
Member

Unlimited is clearer, but kind of long.
What about something like "Any"?

@politas
Copy link
Member

politas commented Apr 23, 2018

Didn't we used to have "Any" and we stopped? Ah, yes #1866 and related #1882 and #1887

@DasSkelett
Copy link
Member Author

I might not get this right, but as I see #1866, the problem was with the mod-JSON parsing and handling "any" and NULL differently?
My PR would just change the displayed string in the GUI, so this shouldn't affect the core.
I got no errors during testing (-> sorting by max ver. column and install some mods), but please corrcet me, as this is a bit more complex for me.

@politas
Copy link
Member

politas commented Apr 25, 2018

Yes, I think in between we changed the display and sorting values for this column to different values.

@politas
Copy link
Member

politas commented Apr 25, 2018

I do think "any" would be preferable to "N/D" (which I admit I am not instantly parsing, myself). Not defined?

@DasSkelett
Copy link
Member Author

Yes, N/D stands for "Not Defined", but if we already have trouble understanding it, it's probably not a good idea. So, "any" now?

@politas
Copy link
Member

politas commented Apr 25, 2018

If you can push a new commit with that change in the next 12 hours, I'll include it in the 1.25.1 release I'm going to put out in the morning.

@DasSkelett DasSkelett changed the title Replace max KSP version empty string with N/D Replace max KSP version empty string with "any" Apr 25, 2018
@DasSkelett
Copy link
Member Author

Done!

@linuxgurugamer
Copy link
Contributor

Might I suggest that you make sure this works EXACTLY as the current KSP-AVC works (which I'm now maintaining)? Some bugs were fixed which might affect this

@linuxgurugamer
Copy link
Contributor

Not saying this is wrong, just want to review it together

@DasSkelett DasSkelett changed the title Replace max KSP version empty string with "any" Replace max KSP version empty string with "any" in GUI Apr 25, 2018
@DasSkelett
Copy link
Member Author

This shouldn't affect AVC at all, since it just changes just the representation of an empty maxVersion string in the GUI.

@politas
Copy link
Member

politas commented Apr 25, 2018

Might I suggest that you make sure this works EXACTLY as the current KSP-AVC works (which I'm now maintaining)? Some bugs were fixed which might affect this

That's a completely different bit of code (assuming you're referring to the parsing of max KSP from .version files)

@politas politas merged commit 7cba16c into KSP-CKAN:master Apr 25, 2018
@DasSkelett DasSkelett deleted the noTextWithNoMAXversion branch April 25, 2018 22:49
@yalov
Copy link
Contributor

yalov commented Apr 27, 2018

empty to "any"?
Then maybe 9.99.99, 99.99.99, 1.99.99 also to any?

KSP-CKAN/NetKAN#5356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issues affecting the interactive GUI Pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants