Fixed 99.9% of the bugs and readd the mixin with clear warnings

src/hdskins/java/com/voxelmodpack/hdskins/gui/GLWindow.java

@@ -1,18 +1,21 @@
 package com.voxelmodpack.hdskins.gui;
 
 import com.google.common.collect.Lists;
+
 import net.minecraft.client.Minecraft;
 import net.minecraft.client.resources.DefaultResourcePack;
 import net.minecraft.util.ResourceLocation;
 import org.lwjgl.LWJGLException;
-import org.lwjgl.opengl.AWTGLCanvas;
 import org.lwjgl.opengl.Display;
-import org.lwjgl.opengl.PixelFormat;
+import org.lwjgl.opengl.DisplayMode;
 
 import java.awt.Canvas;
-import java.awt.dnd.DnDConstants;
 import java.awt.dnd.DropTarget;
 import java.awt.dnd.DropTargetListener;
+import java.awt.event.ComponentAdapter;
+import java.awt.event.ComponentEvent;
+import java.awt.event.WindowAdapter;
+import java.awt.event.WindowEvent;
 import java.io.IOException;
 import java.util.TooManyListenersException;
 import javax.annotation.Nullable;
@@ -22,82 +25,164 @@ import javax.swing.*;
 /**
  * Experimental window to control file drop. It kind of sucks.
  *
- * This has to be enabled using the {@code experimentalSkinDrop} config.
  */
 public class GLWindow extends DropTarget {
 
-    private static GLWindow instance = null;
+    // Serial version because someone decided to extend DropTarget
-
+    private static final long serialVersionUID = -8891327070920541481L;
-    public static void create() {
-        if (instance == null)
-            instance = new GLWindow();
-    }
 
     @Nullable
+    private static GLWindow instance = null;
+
+    /**
+     * Gets or creates the current GLWindow context.
+     */
     public static GLWindow current() {
+        if (instance == null) {
+            instance = new GLWindow();
+        }
         return instance;
     }
 
-    private final JFrame frame;
+    public static void refresh(boolean fullscreen) {
+        if (instance != null) {
+            instance.onRefresh(fullscreen);
+        }
+    }
 
-    private DropTargetListener saved = null;
+    /**
+     * Destroys the current GLWindow context and restores default behaviour.
+     */
+    public static void dispose() {
+        if (instance != null) {
+            instance.close();
+        }
+    }
 
-    // What's so special about these numbers? Are they the same on all systems?
+    private static int getScaledPixelUnit(int i) {
-    private final int frameX = 15;
+        return (int)Math.floor(i * Display.getPixelScaleFactor());
-    private final int frameY = 36;
+    }
 
     private final Minecraft mc = Minecraft.getMinecraft();
 
+    private JFrame frame;
+    private Canvas canvas;
+
+    private DropTargetListener dropListener = null;
+
+    private int windowState = 0;
+
+    private boolean isFullscreen;
+
     private GLWindow() {
-
-        setDefaultActions(DnDConstants.ACTION_LINK);
         try {
-
+            open();
-            int x = Display.getX();
-            int y = Display.getY();
-
-            int w = Display.getWidth() + frameX;
-
-            int h = Display.getHeight() + frameY;
-
-            Canvas canvas = new AWTGLCanvas(new PixelFormat().withDepthBits(24));
-
-            frame = new JFrame(Display.getTitle());
-            frame.setResizable(Display.isResizable());
-            frame.setLocation(x, y);
-            frame.setSize(w, h);
-
-            frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
-
-            // FIXME: icon is super small on the task bar
-            setIcons(frame);
-
-            frame.add(canvas);
-            frame.setVisible(true);
-
-            Display.setParent(canvas);
-
-            Display.setFullscreen(mc.isFullScreen());
-
         } catch (LWJGLException e) {
             throw new RuntimeException(e);
         }
-
-
     }
 
-    public void refresh() {
+    private void open() throws LWJGLException {
-        // trigger an update
+        // Dimensions from LWJGL may have a non 1:1 scale on high DPI monitors.
-        frame.setSize(frame.getWidth(), frame.getHeight()+1);
+        int x = getScaledPixelUnit(Display.getX());
-        frame.setSize(frame.getWidth(), frame.getHeight()-1);
+        int y = getScaledPixelUnit(Display.getY());
-//        frame.pack();
+
+        int w = getScaledPixelUnit(Display.getWidth());
+        int h = getScaledPixelUnit(Display.getHeight());
+
+        isFullscreen = mc.isFullScreen();
+
+        canvas = new Canvas();
+
+        frame = new JFrame(Display.getTitle());
+        frame.add(canvas);
+        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
+        frame.addWindowListener(new WindowAdapter() {
+            @Override
+            public void windowClosed(WindowEvent windowEvent) {
+                mc.shutdown();
+            }
+
+            @Override
+            public void windowStateChanged(WindowEvent event) {
+                windowState = event.getNewState();
+                onResize();
+            }
+
+            @Override
+            public void windowOpened(WindowEvent e) {
+                // Once the window has opened compare the content and window dimensions to get
+                // the OS's frame size then reassign adjusted dimensions to match LWJGL's window.
+                float frameFactorX = frame.getWidth() - frame.getContentPane().getWidth();
+                float frameFactorY = frame.getHeight() - frame.getContentPane().getHeight();
+
+                frame.setSize((int)Math.floor(w + frameFactorX), (int)Math.floor(h + frameFactorY));
+            }
+        });
+        frame.addComponentListener(new ComponentAdapter() {
+            @Override
+            public void componentResized(ComponentEvent componentEvent) {
+                onResize();
+            }
+        });
+
+        // TODO: (Unconfirmed) reports say the icon appears small on some OSs.
+        //       I've yet to reproduce this.
+        setIcons();
+
+        // Order here is important. Size is set _before_ displaying but
+        // after events to ensure the window and canvas both get correct dimensions.
+        frame.setResizable(Display.isResizable());
+        frame.setLocation(x, y);
+        frame.setSize(w, h);
+        frame.setVisible(true);
+
+        Display.setParent(canvas);
+        Display.setFullscreen(isFullscreen);
     }
 
-    private void setIcons(JFrame frame) {
+    private void close() {
         try {
-            // This should be using reflection. No need for this.
+            Display.setParent(null);
-            DefaultResourcePack pack = (DefaultResourcePack) mc.getResourcePackRepository().rprDefaultResourcePack;
+        } catch (LWJGLException e) {
+            e.printStackTrace();
+        }
 
+        try {
+            if (isFullscreen) {
+                Display.setFullscreen(true);
+            } else {
+                if ((windowState & JFrame.MAXIMIZED_BOTH) == JFrame.MAXIMIZED_BOTH) {
+                    Display.setLocation(0, 0);
+                    Display.setDisplayMode(Display.getDesktopDisplayMode());
+                } else {
+                    Display.setDisplayMode(new DisplayMode(frame.getContentPane().getWidth(), frame.getContentPane().getHeight()));
+                    Display.setLocation(frame.getX(), frame.getY());
+                }
+
+                // https://bugs.mojang.com/browse/MC-68754
+                Display.setResizable(false);
+                Display.setResizable(true);
+            }
+        } catch (LWJGLException e) {
+            e.printStackTrace();
+        }
+
+        frame.setVisible(false);
+        frame.dispose();
+
+        instance = null;
+    }
+
+    private void setIcons() {
+        // VanillaTweakInjector.loadIconsOnFrames();
+        try {
+            //
+            // The icons are stored in Display#cached_icons. However they're not the _original_ values.
+            // LWJGL copies the initial byte streams and then reverses them. The result is a stream that's not
+            // only already consumed, but somehow invalid when you try to parse it through ImageIO.read.
+            //
+            DefaultResourcePack pack = (DefaultResourcePack) mc.getResourcePackRepository().rprDefaultResourcePack;
             frame.setIconImages(Lists.newArrayList(
                     ImageIO.read(pack.getInputStreamAssets(new ResourceLocation("icons/icon_16x16.png"))),
                     ImageIO.read(pack.getInputStreamAssets(new ResourceLocation("icons/icon_32x32.png")))
@@ -107,18 +192,35 @@ public class GLWindow extends DropTarget {
         }
     }
 
-    void setDropTargetListener(@Nullable DropTargetListener dtl) {
+    private void onResize() {
-        if (saved != null) {
+        canvas.setBounds(0, 0, frame.getContentPane().getWidth(), frame.getContentPane().getHeight());
-            removeDropTargetListener(saved);
+    }
-        }
+
-        if (dtl == null)
+    private void onRefresh(boolean fullscreen) {
-            frame.setDropTarget(null);
+        if (fullscreen != isFullscreen) {
-        else {
+            // Repaint the canvas, not the window.
-            frame.setDropTarget(this);
+            // The former strips the window of its state. The latter fixes a viewport scaling bug.
-            try {
+            canvas.setBounds(0, 0, 0, 0);
-                addDropTargetListener(dtl);
+            onResize();
-            } catch (TooManyListenersException e) { }
+            isFullscreen = fullscreen;
-            saved = dtl;
         }
     }
+
+    public void clearDropTargetListener() {
+        if (dropListener != null) {
+            removeDropTargetListener(dropListener);
+            dropListener = null;
+            frame.setDropTarget(null);
+        }
+    }
+
+    public void setDropTargetListener(DropTargetListener dtl) {
+        clearDropTargetListener();
+        dropListener = dtl;
+
+        try {
+            frame.setDropTarget(this);
+            addDropTargetListener(dtl);
+        } catch (TooManyListenersException ignored) { }
+    }
 }

src/hdskins/java/com/voxelmodpack/hdskins/gui/GuiSkins.java

@@ -191,11 +191,9 @@ public class GuiSkins extends GuiScreen implements FutureCallback<SkinUploadResp
     }
 
     private void enableDnd() {
-        GLWindow window = GLWindow.current();
+        GLWindow.current().setDropTargetListener((FileDropListener) files -> {
-        if (window != null)
+            files.stream().findFirst().ifPresent(instance::loadLocalFile);
-            window.setDropTargetListener((FileDropListener) files -> {
+        });
-                files.stream().findFirst().ifPresent(instance::loadLocalFile);
-            });
     }
 
     private void initPanoramaRenderer() {
@@ -209,9 +207,7 @@ public class GuiSkins extends GuiScreen implements FutureCallback<SkinUploadResp
         remotePlayer.releaseTextures();
         HDSkinManager.clearSkinCache();
 
-        GLWindow window = GLWindow.current();
+        GLWindow.current().clearDropTargetListener();
-        if (window != null)
-            window.setDropTargetListener(null);
     }
 
     private void onFileOpenDialogClosed(JFileChooser fileDialog, int dialogResult) {

src/hdskins/java/com/voxelmodpack/hdskins/mixin/MixinMinecraft.java

@@ -0,0 +1,78 @@
+package com.voxelmodpack.hdskins.mixin;
+
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.injection.At;
+import org.spongepowered.asm.mixin.injection.Inject;
+import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;
+
+import com.voxelmodpack.hdskins.gui.GLWindow;
+
+import net.minecraft.client.Minecraft;
+import net.minecraft.crash.CrashReport;
+
+/**
+ * Mixin required to close the window after the game detects a crash.
+ *
+ * DO NOT REMOVE
+ *
+ * That means you, Killjoy.
+ *
+ */
+@Mixin(Minecraft.class)
+public abstract class MixinMinecraft {
+    //
+    // Due to how JFrame works the only way to know for sure when the game hash crashed
+    // is to have it call us explicitly.
+    //
+    // ShutdownListener.onShutDown is unlikely to be called as it depends on the
+    // Minecraft.running flag to be unset, which is unlikely to happen if the game crashes.
+    //
+    // Runtime.current().addShutdownHook won't be called it waits for all
+    // non-daemon threads to end, one of which is depending on the JFrame being
+    // disposed to tell it when to end.
+    //
+    // If you're thinking 'hey, what about Minecraft.isCrashed?'
+    // No, that's only set if the internal MinecraftServer crashes.
+    // Otherwise the value is always false and threads spinning to check any such value
+    // will only serve to hang up the VM.
+    //
+    // But senpai, what about a warden thread joined on the main? I'm sure as soon as
+    // the main thread closes that would-
+    //
+    // Nope. It never runs.
+    //
+    // @forge
+    //  Because the minecraft forge team are stupid, they call displayCrashReport on startup
+    //  regardless of whether the game has crashed or not. Thus the window may flicker an additional
+    //  time as the native window is forced back to the front.
+    //  This is a minor issue as the window will simply reassert itself when it's next referenced
+    //  (i.e. The skins GUI uses it for file drops) so I have no intention of fixing this.
+    //
+    //  This is their problem.
+    //
+    //  Update 09/06/2018:
+    //     After inspecting the forge source this was found to be nothing but pure lies.
+    //     The only place they call it is from FMLClientHandler#haltGame.
+    //     There is still the possible case where another mod tries to call it,
+    //     but that would be the very definition of 'misbehaving'.
+    //
+    //     Also note that the method unconditionally calls System.exit, so anyone
+    //     who does will be having a hard time.
+    //
+    // @killjoy
+    //  Don't be afraid to use a mixin when the situation calls for it.
+    //  There is no other way to do this.
+    //
+    //  Do not remove.
+    //
+    //  [!!!DO NOT REMOVE!!!]
+    //
+    //  I'm serious, do not remove.
+    //  I don't care how much of a vendeta you have aginst mixins.
+    //
+    //public void displayCrashReport(CrashReport crashReportIn)
+    @Inject(method = "displayCrashReport(Lnet/minecraft/crash/CrashReport;)V", at = @At("HEAD"))
+    private void onGameCrash(CrashReport report, CallbackInfo info) {
+        GLWindow.dispose();
+    }
+}

src/hdskins/java/com/voxelmodpack/hdskins/mod/LiteModHDSkinsMod.java

@@ -26,8 +26,6 @@ public class LiteModHDSkinsMod implements HDSkinsMod {
 
     @Expose
     public List<String> skin_servers = SkinServer.defaultServers;
-    @Expose
-    public boolean experimentalSkinDrop = false;
 
     @Override
     public String getName() {
@@ -59,7 +57,7 @@ public class LiteModHDSkinsMod implements HDSkinsMod {
         IReloadableResourceManager irrm = (IReloadableResourceManager) Minecraft.getMinecraft().getResourceManager();
         irrm.registerReloadListener(HDSkinManager.INSTANCE);
 
-        if (experimentalSkinDrop) GLWindow.create();
+        GLWindow.current();
     }
 
     @Override
@@ -94,8 +92,6 @@ public class LiteModHDSkinsMod implements HDSkinsMod {
 
     @Override
     public void onFullScreenToggled(boolean fullScreen) {
-        if (!fullScreen && GLWindow.current() != null) {
+        GLWindow.refresh(fullScreen);
-            GLWindow.current().refresh();
-        }
     }
 }

src/hdskins/resources/hdskins.mixin.json

@@ -4,6 +4,7 @@
   "package": "com.voxelmodpack.hdskins.mixin",
   "refmap": "hdskins.mixin.refmap.json",
   "mixins": [
+    "MixinMinecraft",
     "MixinGuiMainMenu",
     "MixinImageBufferDownload",
     "MixinPlayerInfo",