-
Notifications
You must be signed in to change notification settings - Fork 400
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 warning when trying to run Fabric API as an executable #1196
Conversation
I don’t believe we are allowed to release this onto curseforge. I’m 99% sure that executable jars are against the TOS. Please correct me if I’m wrong. |
I have asked around, and it seems it is in the clear. BetterFPS does this as well and is allowed on curseforge. |
|
||
public final class APIWarning { | ||
public static void main(String[] args) { | ||
String st = "Fabric API is not meant to be run.\nPlease place this file in your mods folder."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferably this should be handled by a resource bundle
build.gradle
Outdated
attributes 'Implementation-Title': 'FabricAPI', | ||
'Implementation-Version': project.version, | ||
'Main-Class': 'net.fabricmc.fabric.impl.launch.APIWarning' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should move this block into tasks.withType(Jar)
so this implementation info and main class is available for any jar produced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've fixed both these, you mind reviewing again?
Signed-off-by: IMS <31803019+IMS212@users.noreply.github.com>
Wait, what? I didn't remove the request for API developers |
public static void main(String[] args) { | ||
Locale defaultLocale = Locale.getDefault(); | ||
ResourceBundle WarningAPI = ResourceBundle.getBundle("lang/WarningAPI", defaultLocale); | ||
JOptionPane.showMessageDialog(null, WarningAPI.getString("api.warning")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fail if fabric api is run in a headless environment. Just check GraphicsEnvironment
to validate that otherwise print the message in sys out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would system error be better? Since it's already an error, and this way it will be highlighted red (in consoles/terminals that treat error differently to out).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has been fixed with Alex's PR
String st = "Fabric API is not meant to be run.\nPlease place this file in your mods folder."; | ||
JOptionPane.showMessageDialog(null, st); | ||
Locale defaultLocale = Locale.getDefault(); | ||
ResourceBundle WarningAPI = ResourceBundle.getBundle("lang/WarningAPI", defaultLocale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope we can have a bundle setup like https://github.com/FabricMC/fabric-installer/blob/e76d587e22ae9767c0f97fc782c991e461881b1c/src/main/java/net/fabricmc/installer/util/Utils.java#L36-L50 so that we don't need to use iso encoding and ugly unicode escapes for the translation properties
* Fix headless exception. (This seems to work for me locally?) * Spaces -> Tabs. * msg -> message. * Fix uppercase variable name (by inlining it).
…hanks to @AlexIIL Signed-off-by: IMS <31803019+IMS212@users.noreply.github.com>
It's been updated with both ResourceBundles and headless support, you mind re-reviewing? |
I would also add something to link to the fabric installer, so that people can install the loader if they don't have it yet |
Signed-off-by: IMS <31803019+IMS212@users.noreply.github.com>
Done. |
|
||
public final class APIWarning { | ||
public static void main(String[] args) { | ||
final ResourceBundle WarningAPI = ResourceBundle.getBundle("lang/WarningAPI", Locale.getDefault(), new ResourceBundle.Control() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we prefer name the local var in lower camel like warningApi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
e.printStackTrace(); | ||
} | ||
|
||
return super.newBundle(baseName, locale, format, loader, reload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return super.newBundle(baseName, locale, format, loader, reload); | |
return super.newBundle(baseName, locale, format, loader, reload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good? Unless you mean a different area
Co-authored-by: liach <7806504+liach@users.noreply.github.com>
Buddy, this made me laugh 😆 Is this really necessary? I believe 95%+ of players know this should be dragged into If players don't know Fabric API is a mod instead of an executable library, you can add a tip in the installer, noticing them download Fabric API in CurseForge/Modrinth and put it into |
Accessibility features make you laugh? sad While, yes, a large user base should know this should be put into the This comment honestly comes off as a rather elitist take and hostile towards new users. I would rather not have the same issue as Linux here in Fabric or its alternatives. |
This feature looks good, but I believe it should belong in Loom so that everyone can use it (with optout ofc). Also the branch is 1.16, which is way outdated. |
Great idea! However, I think the class should be shrunk as small as possible. Using proguard and package-less class name can shrink it in a specific way. Also, add a boolean toggle in To make it short:
|
This was made as my first PR ever. I am now a developer for many mods, and can likely recreate this much better, so as such, I'll close this. |
How would it be included in Loom? Would it just embed itself into the jar? |
To store the class file in Loom, and dumps it into |
This PR shows this when the jar is run as an executable. This would fix a lot of the confusion around the API, and the current code does not seem to have problems with anything else in the API.