-
Notifications
You must be signed in to change notification settings - Fork 28
fix /importfile erroring & update char limit #74
Conversation
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.
Thanks for the PR; I have a couple questions.
@@ -6,6 +6,8 @@ | |||
public final class Main implements ClientModInitializer { | |||
@Override | |||
public void onInitializeClient() { | |||
System.setProperty("java.awt.headless", "false"); // Enable AWT features |
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 line used to be in the mod, but we removed it because Minecraft runs in headless mode for a reason. Unless I am mistaken, /importfile
should use native code, not AWT.
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.
Seems like FileDialog is in fact AWT, also from my testing this works perfectly fine, don't see why its an issue
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.
FileDialog is AWT, but we shouldn't be using FIleDialog. It works for you because you are not on a headless machine. For more reference, see how Minecraft implements the "Open Resource Pack Folder" button.
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.
Minecraft uses their own Util class for getting the operating system where they open a folder using a URI command, I don't see how opening a folder is related to picking a file. If it is in fact possible to use a URI command to open a file picker I would love for someone else to work on fixing /importfile since I cannot figure this one out and it's a pretty useful command.
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'm not entirely sure it matters; I'm just going off of what I've looked up and heard in the Fabric discord. I think the idea is that native code can be extended to do more than AWT (since Minecraft used to run on the Raspberry Pi, etc), but for 99.9% of users this isn't an issue. For now I'm just working on other things so I'll leave it to other contributors to debate.
@@ -58,8 +58,8 @@ public void register(Minecraft mc, CommandDispatcher<FabricClientCommandSource> | |||
|
|||
while (sc.hasNextLine()) { | |||
String line = sc.nextLine(); | |||
if (line.length() > 2000) { | |||
ChatUtil.sendMessage("Line " + (lines.size() + 1) + " is too long! (" + line.length() + " > 2000)", ChatType.FAIL); | |||
if (line.length() > 10_000) { |
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.
Is there a reason this should be raised to 10,000 specifically? How did you come up with this?
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.
That's the new string limit on DiamondFire
I feel like this is a needed fix, N_Enders has been asking for this for a long time now. If possible, could you build it and post a version on your repo? |
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.
Per our more recent discord conversation, this is good as is. Thanks :)
Wait, nevermind. I searched some more on the Fabric discord and remember why this is a bad idea.
I will get back to this tomorrow |
as discussed on discord, it's better to use a native (c) library to open a file picker instead of setting headless to false |
No description provided.