Changes proposed by @totalvoidness in code review

main/ui/src/main/java/org/cryptomator/ui/Main.java

@@ -1,10 +1,10 @@
 /*******************************************************************************
- * Copyright (c) 2014 Sebastian Stenzel
+ * Copyright (c) 2014 cryptomator.org
  * This file is licensed under the terms of the MIT license.
  * See the LICENSE.txt file for more info.
  * 
  * Contributors:
- *     Sebastian Stenzel - initial API and implementation
+ *     Tillmann Gaida - initial implementation
  ******************************************************************************/
 package org.cryptomator.ui;
 
@@ -15,7 +15,7 @@ import java.util.function.Consumer;
 
 import javafx.application.Application;
 
-import org.controlsfx.tools.Platform;
+import org.apache.commons.lang3.SystemUtils;
 import org.cryptomator.ui.util.SingleInstanceManager;
 import org.cryptomator.ui.util.SingleInstanceManager.RemoteInstance;
 import org.slf4j.Logger;
@@ -26,10 +26,10 @@ import com.github.axet.desktop.os.mac.AppleHandlers;
 public class Main {
 	public static final Logger LOG = LoggerFactory.getLogger(MainApplication.class);
 
-	public static final CompletableFuture<Consumer<File>> openFileHandler = new CompletableFuture<>();
+	public static final CompletableFuture<Consumer<File>> OPEN_FILE_HANDLER = new CompletableFuture<>();
 
 	public static void main(String[] args) {
-		if (Platform.getCurrent().equals(org.controlsfx.tools.Platform.OSX)) {
+		if (SystemUtils.IS_OS_MAC_OSX) {
 			/*
 			 * On OSX we're in an awkward position. We need to register a
 			 * handler in the main thread of this application. However, we can't
@@ -38,15 +38,15 @@ public class Main {
 			 * the file in the application.
 			 */
 			try {
-				AppleHandlers.getAppleHandlers().addOpenFileListener(list -> {
+				AppleHandlers.getAppleHandlers().addOpenFileListener(file -> {
 					try {
-						openFileHandler.get().accept(list);
+						OPEN_FILE_HANDLER.get().accept(file);
 					} catch (Exception e) {
 						LOG.error("exception handling file open event", e);
 						throw new RuntimeException(e);
 					}
 				});
-			} catch (Throwable e) {
+			} catch (RuntimeException e) {
 				// Since we're trying to call OS-specific code, we'll just have
 				// to hope for the best.
 				LOG.error("exception adding OSX file open handler", e);

main/ui/src/main/java/org/cryptomator/ui/MainApplication.java

@@ -8,8 +8,10 @@
  ******************************************************************************/
 package org.cryptomator.ui;
 
-import java.io.File;
 import java.io.IOException;
+import java.nio.file.FileSystems;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.ResourceBundle;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
@@ -22,6 +24,7 @@ import javafx.scene.Parent;
 import javafx.scene.Scene;
 import javafx.stage.Stage;
 
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.SystemUtils;
 import org.cryptomator.crypto.aes256.Aes256Cryptor;
 import org.cryptomator.ui.settings.Settings;
@@ -43,7 +46,7 @@ public class MainApplication extends Application {
 
 	private static final Logger LOG = LoggerFactory.getLogger(MainApplication.class);
 
-	ExecutorService executorService;
+	private ExecutorService executorService;
 
 	@Override
 	public void start(final Stage primaryStage) throws IOException {
@@ -77,7 +80,7 @@ public class MainApplication extends Application {
 		}
 
 		if (org.controlsfx.tools.Platform.getCurrent().equals(org.controlsfx.tools.Platform.OSX)) {
-			Main.openFileHandler.complete(file -> handleCommandLineArg(ctrl, file.getAbsolutePath()));
+			Main.OPEN_FILE_HANDLER.complete(file -> handleCommandLineArg(ctrl, file.getAbsolutePath()));
 		}
 
 		LocalInstance cryptomatorGuiInstance = SingleInstanceManager.startLocalInstance(APPLICATION_KEY, executorService);
@@ -89,21 +92,25 @@ public class MainApplication extends Application {
 	}
 
 	void handleCommandLineArg(final MainController ctrl, String arg) {
-		File file = new File(arg);
-		if (!file.exists()) {
-			if (!file.mkdirs()) {
+		Path file = FileSystems.getDefault().getPath(arg);
+		if (!Files.exists(file)) {
+			try {
+				if (!Files.isDirectory(Files.createDirectories(file))) {
+					return;
+				}
+			} catch (IOException e) {
 				return;
 			}
 			// directory created.
-		} else if (file.isFile()) {
-			if (file.getName().toLowerCase().endsWith(Aes256Cryptor.MASTERKEY_FILE_EXT.toLowerCase())) {
-				file = file.getParentFile();
+		} else if (Files.isRegularFile(file)) {
+			if (StringUtils.endsWithIgnoreCase(file.getFileName().toString(), Aes256Cryptor.MASTERKEY_FILE_EXT)) {
+				file = file.getParent();
 			} else {
 				// is a file, but not a masterkey file
 				return;
 			}
 		}
-		File f = file;
+		Path f = file;
 		Platform.runLater(() -> {
 			ctrl.addDirectory(f);
 			ctrl.toFront();
@@ -150,7 +157,7 @@ public class MainApplication extends Application {
 			SHUTDOWN_TASKS.forEach(r -> {
 				try {
 					r.run();
-				} catch (Throwable e) {
+				} catch (RuntimeException e) {
 					LOG.error("exception while shutting down", e);
 				}
 			});

main/ui/src/main/java/org/cryptomator/ui/MainController.java

@@ -11,6 +11,8 @@ package org.cryptomator.ui;
 import java.io.File;
 import java.io.IOException;
 import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.Collection;
 import java.util.ResourceBundle;
 import java.util.stream.Collectors;
@@ -75,7 +77,7 @@ public class MainController implements Initializable, InitializationListener, Un
 	private void didClickAddDirectory(ActionEvent event) {
 		final DirectoryChooser dirChooser = new DirectoryChooser();
 		final File file = dirChooser.showDialog(stage);
-		addDirectory(file);
+		addDirectory(file.toPath());
 	}
 
 	/**
@@ -85,9 +87,9 @@ public class MainController implements Initializable, InitializationListener, Un
 	 * @param file
 	 *            non-null, writable, existing directory
 	 */
-	void addDirectory(final File file) {
-		if (file != null && file.canWrite()) {
-			final Directory dir = new Directory(file.toPath());
+	void addDirectory(final Path file) {
+		if (file != null && Files.isWritable(file)) {
+			final Directory dir = new Directory(file);
 			if (!directoryList.getItems().contains(dir)) {
 				directoryList.getItems().add(dir);
 			}

main/ui/src/main/java/org/cryptomator/ui/model/Directory.java

@@ -8,6 +8,7 @@ import java.nio.file.Path;
 import javafx.beans.property.ObjectProperty;
 import javafx.beans.property.SimpleObjectProperty;
 
+import org.apache.commons.lang3.StringUtils;
 import org.cryptomator.crypto.Cryptor;
 import org.cryptomator.crypto.SamplingDecorator;
 import org.cryptomator.crypto.aes256.Aes256Cryptor;
@@ -117,7 +118,7 @@ public class Directory implements Serializable {
 	 */
 	public String getName() {
 		String name = path.getFileName().toString();
-		if (name.toLowerCase().endsWith(Aes256Cryptor.FOLDER_EXTENSION.toLowerCase())) {
+		if (StringUtils.endsWithIgnoreCase(name, Aes256Cryptor.FOLDER_EXTENSION)) {
 			name = name.substring(0, name.length() - Aes256Cryptor.FOLDER_EXTENSION.length());
 		}
 		return name;

main/ui/src/main/java/org/cryptomator/ui/util/ListenerRegistry.java

@@ -1,10 +1,10 @@
 /*******************************************************************************
- * Copyright (c) 2014 Sebastian Stenzel
+ * Copyright (c) 2014 cryptomator.org
  * This file is licensed under the terms of the MIT license.
  * See the LICENSE.txt file for more info.
  * 
  * Contributors:
- *     Sebastian Stenzel - initial API and implementation
+ *     Tillmann Gaida - initial implementation
  ******************************************************************************/
 package org.cryptomator.ui.util;
 

main/ui/src/main/java/org/cryptomator/ui/util/SingleInstanceManager.java

@@ -29,6 +29,7 @@ import java.util.Set;
 import java.util.concurrent.ExecutorService;
 import java.util.prefs.Preferences;
 
+import org.apache.commons.io.IOUtils;
 import org.cryptomator.ui.Main;
 import org.cryptomator.ui.util.ListenerRegistry.ListenerRegistration;
 import org.slf4j.Logger;
@@ -104,7 +105,7 @@ public class SingleInstanceManager {
 	/**
 	 * Represents a socket making this the main instance of the application.
 	 */
-	public static abstract class LocalInstance implements Closeable {
+	public static class LocalInstance implements Closeable {
 		private class ChannelState {
 			ByteBuffer write = ByteBuffer.wrap(applicationKey.getBytes());
 			ByteBuffer readLength = ByteBuffer.allocate(2);
@@ -113,10 +114,15 @@ public class SingleInstanceManager {
 
 		final ListenerRegistry<MessageListener, String> registry = new ListenerRegistry<>(MessageListener::handleMessage);
 		final String applicationKey;
+		final ServerSocketChannel channel;
+		final Selector selector;
+		int port = 0;
 
-		public LocalInstance(String applicationKey) {
+		public LocalInstance(String applicationKey, ServerSocketChannel channel, Selector selector) {
 			Objects.requireNonNull(applicationKey);
 			this.applicationKey = applicationKey;
+			this.channel = channel;
+			this.selector = selector;
 		}
 
 		/**
@@ -130,7 +136,16 @@ public class SingleInstanceManager {
 			return registry.registerListener(listener);
 		}
 
-		void handle(SelectionKey key) throws IOException {
+		void handleSelection(SelectionKey key) throws IOException {
+			if (key.isAcceptable()) {
+				final SocketChannel accepted = channel.accept();
+				if (accepted != null) {
+					LOG.info("accepted incoming connection");
+					accepted.configureBlocking(false);
+					accepted.register(selector, SelectionKey.OP_READ | SelectionKey.OP_WRITE);
+				}
+			}
+
 			if (key.attachment() == null) {
 				key.attach(new ChannelState());
 			}
@@ -174,7 +189,39 @@ public class SingleInstanceManager {
 			}
 		}
 
-		public abstract void close();
+		public void close() {
+			IOUtils.closeQuietly(selector);
+			IOUtils.closeQuietly(channel);
+			if (getSavedPort(applicationKey).orElse(-1).equals(port)) {
+				Preferences.userNodeForPackage(Main.class).remove(applicationKey);
+			}
+		}
+
+		void selectionLoop() {
+			try {
+				final Set<SelectionKey> keysToRemove = new HashSet<>();
+				while (selector.select() > 0) {
+					final Set<SelectionKey> keys = selector.selectedKeys();
+					for (SelectionKey key : keys) {
+						if (Thread.interrupted()) {
+							return;
+						}
+						try {
+							handleSelection(key);
+						} catch (IOException | IllegalStateException e) {
+							LOG.error("exception in selector", e);
+						} finally {
+							keysToRemove.add(key);
+						}
+					}
+					keys.removeAll(keysToRemove);
+				}
+			} catch (ClosedSelectorException e) {
+				return;
+			} catch (Exception e) {
+				LOG.error("error while selecting", e);
+			}
+		}
 	}
 
 	/**
@@ -230,12 +277,8 @@ public class SingleInstanceManager {
 		} catch (Exception e) {
 			return Optional.empty();
 		} finally {
-			try {
-				if (channel != null && close && channel.isOpen()) {
-					channel.close();
-				}
-			} catch (Exception e) {
-
+			if (close) {
+				IOUtils.closeQuietly(channel);
 			}
 		}
 	}
@@ -263,7 +306,6 @@ public class SingleInstanceManager {
 	 * @return
 	 * @throws IOException
 	 */
-	@SuppressWarnings("resource")
 	public static LocalInstance startLocalInstance(String applicationKey, ExecutorService exec) throws IOException {
 		final ServerSocketChannel channel = ServerSocketChannel.open();
 		channel.configureBlocking(false);
@@ -276,59 +318,15 @@ public class SingleInstanceManager {
 		Selector selector = Selector.open();
 		channel.register(selector, SelectionKey.OP_ACCEPT);
 
-		LocalInstance instance = new LocalInstance(applicationKey) {
-			@Override
-			public void close() {
-				try {
-					selector.close();
-				} catch (IOException e) {
-					LOG.error("error closing socket", e);
-				}
-				try {
-					channel.close();
-				} catch (IOException e) {
-					LOG.error("error closing selector", e);
-				}
-				if (getSavedPort(applicationKey).orElse(-1).equals(port)) {
-					Preferences.userNodeForPackage(Main.class).remove(applicationKey);
-				}
-			}
-		};
+		LocalInstance instance = new LocalInstance(applicationKey, channel, selector);
 
 		exec.submit(() -> {
 			try {
-				final Set<SelectionKey> keysToRemove = new HashSet<>();
-				while (selector.select() > 0) {
-					final Set<SelectionKey> keys = selector.selectedKeys();
-					for (SelectionKey key : keys) {
-						if (Thread.interrupted()) {
-							return;
-						}
-						try {
-							// LOG.debug("selected {} {}", key.channel(),
-							// key.readyOps());
-							if (key.isAcceptable()) {
-								final SocketChannel accepted = channel.accept();
-								if (accepted != null) {
-									LOG.info("accepted incoming connection");
-									accepted.configureBlocking(false);
-									accepted.register(selector, SelectionKey.OP_READ | SelectionKey.OP_WRITE);
-								}
-							}
-							instance.handle(key);
-						} catch (IOException | IllegalStateException e) {
-							LOG.error("exception in selector", e);
-						} finally {
-							keysToRemove.add(key);
-						}
-					}
-					keys.removeAll(keysToRemove);
-				}
-			} catch (ClosedSelectorException e) {
-				return;
-			} catch (Throwable e) {
-				LOG.error("error while selecting", e);
+				instance.port = ((InetSocketAddress) channel.getLocalAddress()).getPort();
+			} catch (IOException e) {
+
 			}
+			instance.selectionLoop();
 		});
 
 		return instance;

main/ui/src/main/java/org/cryptomator/ui/util/TimeoutTask.java

@@ -1,10 +1,10 @@
 /*******************************************************************************
- * Copyright (c) 2014 Sebastian Stenzel
+ * Copyright (c) 2014 cryptomator.org
  * This file is licensed under the terms of the MIT license.
  * See the LICENSE.txt file for more info.
  * 
  * Contributors:
- *     Sebastian Stenzel - initial API and implementation
+ *     Tillmann Gaida - initial implementation
  ******************************************************************************/
 package org.cryptomator.ui.util;
 

main/ui/src/test/java/org/cryptomator/ui/util/ListenerRegistryTest.java

@@ -1,3 +1,11 @@
+/*******************************************************************************
+ * Copyright (c) 2014 cryptomator.org
+ * This file is licensed under the terms of the MIT license.
+ * See the LICENSE.txt file for more info.
+ * 
+ * Contributors:
+ *     Tillmann Gaida - initial implementation
+ ******************************************************************************/
 package org.cryptomator.ui.util;
 
 import static org.junit.Assert.*;

main/ui/src/test/java/org/cryptomator/ui/util/SingleInstanceManagerTest.java

@@ -1,3 +1,11 @@
+/*******************************************************************************
+ * Copyright (c) 2014 cryptomator.org
+ * This file is licensed under the terms of the MIT license.
+ * See the LICENSE.txt file for more info.
+ * 
+ * Contributors:
+ *     Tillmann Gaida - initial implementation
+ ******************************************************************************/
 package org.cryptomator.ui.util;
 
 import static org.junit.Assert.*;
@@ -10,7 +18,6 @@ import java.nio.ByteBuffer;
 import java.nio.channels.SocketChannel;
 import java.util.Collections;
 import java.util.HashSet;
-import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.UUID;
@@ -27,7 +34,7 @@ import org.cryptomator.ui.util.SingleInstanceManager.RemoteInstance;
 import org.junit.Test;
 
 public class SingleInstanceManagerTest {
-	@Test(timeout = 1000)
+	@Test(timeout = 10000)
 	public void testTryFillTimeout() throws Exception {
 		try (final ServerSocket socket = new ServerSocket(0)) {
 			// we need to asynchronously accept the connection
@@ -44,9 +51,9 @@ public class SingleInstanceManagerTest {
 			try (SocketChannel channel = SocketChannel.open()) {
 				channel.configureBlocking(false);
 				channel.connect(new InetSocketAddress(InetAddress.getLoopbackAddress(), socket.getLocalPort()));
-				TimeoutTask.attempt(t -> channel.finishConnect(), 100, 1);
+				TimeoutTask.attempt(t -> channel.finishConnect(), 1000, 1);
 				final ByteBuffer buffer = ByteBuffer.allocate(1);
-				SingleInstanceManager.tryFill(channel, buffer, 100);
+				SingleInstanceManager.tryFill(channel, buffer, 1000);
 				assertTrue(buffer.hasRemaining());
 			}
 
@@ -54,7 +61,7 @@ public class SingleInstanceManagerTest {
 		}
 	}
 
-	@Test(timeout = 1000)
+	@Test(timeout = 10000)
 	public void testTryFill() throws Exception {
 		try (final ServerSocket socket = new ServerSocket(0)) {
 			// we need to asynchronously accept the connection
@@ -71,9 +78,9 @@ public class SingleInstanceManagerTest {
 			try (SocketChannel channel = SocketChannel.open()) {
 				channel.configureBlocking(false);
 				channel.connect(new InetSocketAddress(InetAddress.getLoopbackAddress(), socket.getLocalPort()));
-				TimeoutTask.attempt(t -> channel.finishConnect(), 100, 1);
+				TimeoutTask.attempt(t -> channel.finishConnect(), 1000, 1);
 				final ByteBuffer buffer = ByteBuffer.allocate(1);
-				SingleInstanceManager.tryFill(channel, buffer, 100);
+				SingleInstanceManager.tryFill(channel, buffer, 1000);
 				assertFalse(buffer.hasRemaining());
 			}
 
@@ -104,7 +111,7 @@ public class SingleInstanceManagerTest {
 			assertTrue(r.isPresent());
 
 			String message = "Is this thing on?";
-			assertTrue(r.get().sendMessage(message, 100));
+			assertTrue(r.get().sendMessage(message, 1000));
 			System.out.println("wrote message");
 
 			latch.await(10, TimeUnit.SECONDS);
@@ -153,7 +160,7 @@ public class SingleInstanceManagerTest {
 										if (!sentMessages.add(message)) {
 											continue;
 										}
-										r.get().sendMessage(message, 100);
+										r.get().sendMessage(message, 1000);
 										break;
 									}
 								} catch (Exception e) {