[client] reworked def/custom permissions handling in ImageDetailsWindow

introduced PermsHelper.hasChanged(map1, map2) which returns whether two maps are equals (in their contents)
CAVE: the whole CustomPermManager stuff currently works by giving the ImagePermissionWindow direct references to the members of ImageDetailsWindow. Therefore, we are currently ignoring the return value of ImageCustomPermissionWindow as the refs are updated when the user changes stuff. As a result we are back to the "clean" check within reactToInput() to determine whether change occured or not. Should be discussed if giving copies to ImageCustomPermissionWindow/Manager would be even cleaner...

4 files changed, 129 insertions, 52 deletions

diff --git a/dozentenmodul/src/main/java/org/openslx/dozmod/gui/control/ImageCustomPermissionManager.java b/dozentenmodul/src/main/java/org/openslx/dozmod/gui/control/ImageCustomPermissionManager.java
index 76b6a94f..188f1891 100644
--- a/dozentenmodul/src/main/java/org/openslx/dozmod/gui/control/ImageCustomPermissionManager.java
+++ b/dozentenmodul/src/main/java/org/openslx/dozmod/gui/control/ImageCustomPermissionManager.java
@@ -3,6 +3,8 @@ package org.openslx.dozmod.gui.control;
 import java.awt.event.ActionEvent;
 import java.awt.event.ActionListener;
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -24,6 +26,8 @@ import org.openslx.dozmod.gui.control.table.ImagePermissionTable.UserImagePermis
 import org.openslx.dozmod.gui.helper.GridManager;
 import org.openslx.dozmod.gui.window.UserListWindow;
 import org.openslx.dozmod.gui.window.UserListWindow.UserAddedCallback;
+import org.openslx.dozmod.thrift.cache.UserCache;
+import org.openslx.dozmod.util.FormatHelper;
 
 
 /**
@@ -81,10 +85,8 @@ public class ImageCustomPermissionManager extends JPanel {
 			@Override
 			public void actionPerformed(ActionEvent e) {
 				UserListWindow.open(SwingUtilities.getWindowAncestor(me), new UserAddedCallback() {
-
 					@Override
 					public void userAdded(final UserInfo newUser, UserListWindow window) {
-
 						// check if we have this user already
 						for (UserImagePermissions current : permissionList) {
 							if (current.userId.equals(newUser.userId)) {
@@ -94,7 +96,6 @@ public class ImageCustomPermissionManager extends JPanel {
 						}
 						// add it to the list with default permissions
 						permissionList.add(new UserImagePermissions(newUser.userId, new ImagePermissions(defaultPermissions)));
-						LOGGER.debug("User added: " + newUser);
 						permissionTable.setData(permissionList, false);
 					}
 				}, "Hinzufügen", ownerId);
@@ -103,12 +104,10 @@ public class ImageCustomPermissionManager extends JPanel {
 
 		// delete user button listener
 		btnRemoveUser.addActionListener(new ActionListener() {
-
 			@Override
 			public void actionPerformed(ActionEvent e) {
 				final UserImagePermissions selected = permissionTable.getSelectedItem();
-				LOGGER.debug("Removing: " + selected);
-				if (!permissionList.remove(selected)) {
+				if (selected != null && !permissionList.remove(selected)) {
 					LOGGER.debug("Could not remove: " + selected);
 				}
 				permissionTable.setData(permissionList, false);
@@ -117,8 +116,9 @@ public class ImageCustomPermissionManager extends JPanel {
 	}
 
 	/**
-	 * Initialise the PermissionManager
-	 * @param permissionMap the old permission, to initialise the table with, null creates empty table.
+	 * Initialize the PermissionManager
+	 *
+	 * @param permissionMap the old permission, to initialize the table with, null creates empty table.
 	 * @param defaultPermissions the permissions for a newly added user
 	 * @param ownerId The user to exclude from the add user list. Can be null.
 	 */
@@ -131,7 +131,18 @@ public class ImageCustomPermissionManager extends JPanel {
 		for (Entry<String, ImagePermissions> e : newPermissionMap.entrySet()) {
 			permissionList.add(new UserImagePermissions(e.getKey(), e.getValue()));
 		}
-
+		// lexicographic sort on the last names
+		Collections.sort(permissionList, new Comparator<UserImagePermissions>() {
+			@Override
+			public int compare(UserImagePermissions o1, UserImagePermissions o2) {
+				UserInfo u1 = UserCache.find(o1.userId);
+				UserInfo u2 = UserCache.find(o2.userId);
+				if (u1 != null && u2 != null)
+					return FormatHelper.userName(u1).compareTo(FormatHelper.userName(u2));
+				else
+					return 0;
+			}
+		});
 		permissionTable.setData(permissionList, false);
 	}
 
diff --git a/dozentenmodul/src/main/java/org/openslx/dozmod/gui/window/ImageDetailsWindow.java b/dozentenmodul/src/main/java/org/openslx/dozmod/gui/window/ImageDetailsWindow.java
index c70842a8..d53daf95 100644
--- a/dozentenmodul/src/main/java/org/openslx/dozmod/gui/window/ImageDetailsWindow.java
+++ b/dozentenmodul/src/main/java/org/openslx/dozmod/gui/window/ImageDetailsWindow.java
@@ -11,8 +11,10 @@ import java.awt.event.WindowAdapter;
 import java.awt.event.WindowEvent;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 
 import javax.swing.JFrame;
 import javax.swing.JMenuItem;
@@ -40,8 +42,9 @@ import org.openslx.dozmod.gui.window.UserListWindow.UserAddedCallback;
 import org.openslx.dozmod.gui.window.layout.ImageDetailsWindowLayout;
 import org.openslx.dozmod.gui.wizard.ImageUpdateWizard;
 import org.openslx.dozmod.gui.wizard.LectureWizard;
-import org.openslx.dozmod.permissions.ImagePerms;
 import org.openslx.dozmod.permissions.DefaultCustomPerms;
+import org.openslx.dozmod.permissions.ImagePerms;
+import org.openslx.dozmod.permissions.PermsHelper;
 import org.openslx.dozmod.thrift.Session;
 import org.openslx.dozmod.thrift.ThriftActions;
 import org.openslx.dozmod.thrift.ThriftActions.DeleteCallback;
@@ -84,12 +87,9 @@ public class ImageDetailsWindow extends ImageDetailsWindowLayout implements UiFe
 	/**
 	 * The custom permissions of the image
 	 */
-	private Map<String, ImagePermissions> permissionMap;
-
-	/**
-	 * Whether the custom permission window has been used.
-	 */
-	private boolean permissionsChanged = false;
+	private Map<String, ImagePermissions> customPermissions;
+	private Map<String, ImagePermissions> originalCustomPermissions;
+	private ImagePermissions originalDefaultPermissions;
 
 	/**
 	 * Popup menu items
@@ -156,14 +156,15 @@ public class ImageDetailsWindow extends ImageDetailsWindowLayout implements UiFe
 		btnPermissions.addActionListener(new ActionListener() {
 			@Override
 			public void actionPerformed(ActionEvent arg0) {
-				DefaultCustomPerms<ImagePermissions> pl = ImagePermissionWindow.open(me,
-						me.permissionMap, image.defaultPermissions, image.ownerId);
-				if (pl != null && pl.defaultPermissions != null && pl.customPermissions != null) {
-					image.defaultPermissions = pl.defaultPermissions;
-					permissionMap = pl.customPermissions;
-					// TODO perms not changed for sure
-					permissionsChanged = true;
-				}
+				// open the custom permission window and save returned default/custom permissions
+				DefaultCustomPerms<ImagePermissions> combined = ImagePermissionWindow.open(me,
+						customPermissions, image.defaultPermissions, image.ownerId);
+				// since the window above gets references of the default/custom permission object
+				// there is no need to further save the return value ...
+				// THAT or we do work with copies in ImagePermissionWindow ...
+
+				// for now let's stay with refs and just reactToChange() where we check if
+				// the permissions stuff changed since the last call of fill()
 				reactToChange();
 			}
 		});
@@ -250,6 +251,9 @@ public class ImageDetailsWindow extends ImageDetailsWindowLayout implements UiFe
 	 * 
 	 ********************************************************************************/
 	/**
+	 * Sets the image to the given imageBaseId. This will also trigger fill() which 
+	 * will set the image details fields to the values represented by this image.
+	 *
 	 * @param imageBaseId the id of the image to be displayed
 	 */
 	public void setImage(final String imageBaseId) {
@@ -264,12 +268,12 @@ public class ImageDetailsWindow extends ImageDetailsWindowLayout implements UiFe
 					return;
 				}
 				if (permissions == null) {
-					// TODO
+					permissions = new HashMap<>();
 				}
 
 				synchronized (me) {
 					image = imageDetails;
-					permissionMap = permissions;
+					customPermissions = permissions;
 
 				}
 				fill();
@@ -294,7 +298,7 @@ public class ImageDetailsWindow extends ImageDetailsWindowLayout implements UiFe
 	}
 
 	/**
-	 * Push the changes of the image details to the satellite
+	 * Pushes the changes of the image details to the satellite
 	 */
 	private void saveChanges() {
 		// first build the ImageBaseWrite from the GUI fields
@@ -307,16 +311,16 @@ public class ImageDetailsWindow extends ImageDetailsWindowLayout implements UiFe
 		// now trigger the actual action
 		if (!ThriftActions.updateImageBase(JOptionPane.getFrameForComponent(me), image.getImageBaseId(), ibw))
 			return;
-		if (permissionsChanged) {
+		if (PermsHelper.hasChanged(originalCustomPermissions, customPermissions)) {
 			try {
 				ThriftManager.getSatClient().writeImagePermissions(Session.getSatelliteToken(),
-						image.imageBaseId, permissionMap);
-				permissionsChanged = false;
+						image.imageBaseId, customPermissions);
 			} catch (TException e) {
 				LOGGER.error("Fehler beim Übertragen der benutzerspezifischen Berechtigungen: ", e);
 			}
 		}
 		// success
+		LOGGER.info("Successfully saved changes to satellite.");
 		btnSaveChanges.setEnabled(false);
 		callback.updated();
 		dispose();
@@ -375,7 +379,22 @@ public class ImageDetailsWindow extends ImageDetailsWindowLayout implements UiFe
 	private void fill() {
 		if (image == null)
 			return;
-		LOGGER.debug(image);
+		// remember default permissions
+		if (image.defaultPermissions != null) {
+			originalDefaultPermissions = new ImagePermissions(image.defaultPermissions);
+		}
+		// remember custom permissions
+		if (customPermissions != null) {
+			// need a deep copy of the permission map to be able to check for changes after ImageCustomPermissionWindow
+			if (originalCustomPermissions == null)
+				originalCustomPermissions = new HashMap<String, ImagePermissions>();
+			else
+				originalCustomPermissions.clear();
+			// fill it
+			for (Entry<String, ImagePermissions> entry : customPermissions.entrySet()) {
+				originalCustomPermissions.put(entry.getKey(), new ImagePermissions(entry.getValue()));
+			}
+		}
 		txtTitle.setText(image.getImageName());
 		txtDescription.setText(image.getDescription());
 		lblOwner.setUser(UserCache.find(image.getOwnerId()));
@@ -430,6 +449,9 @@ public class ImageDetailsWindow extends ImageDetailsWindowLayout implements UiFe
 		setVisible(true);
 	}
 
+	/**
+	 * Creates listeners to register change in the fields of the details panel
+	 */
 	private void listenToChange() {
 		// final step, add listeners to react to change
 		final DocumentListener docListener = new DocumentListener() {
@@ -475,25 +497,13 @@ public class ImageDetailsWindow extends ImageDetailsWindowLayout implements UiFe
 	}
 
 	/**
-	 * Enables/disables the editable fields based on 'editable'
-	 * 
-	 * @param editable true to make fields editable, false otherwise.
-	 */
-	private void makeEditable(boolean editable) {
-		editable = editable && (ImagePerms.canEdit(image) || ImagePerms.canAdmin(image));
-		txtTitle.setEditable(editable);
-		txtDescription.setEditable(editable);
-		txtTags.setEditable(editable);
-		cboOperatingSystem.setEnabled(editable);
-		//		cboShareMode.setEnabled(editable);
-		btnPermissions.setEnabled(editable && ImagePerms.canAdmin(image));
-		btnChangeOwner.setEnabled(editable && ImagePerms.canAdmin(image));
-		btnUpdateImage.setEnabled(editable);
-	}
-
-	/**
 	 * Checks whether the user changed any fields of the image details and
-	 * enables the save button if so.
+	 * enables the save button if so
+	 * TODO: give user feedback why save isn't enabled if he changed something
+	 * but some other field is not valid....
+	 * e.g. no OS preset, change some checkbox, still can't save since OS is not selected
+	 * and the user is currently not notified that 1) we saw he changed something 2) what
+	 * field is invalid (in this example, OS)
 	 */
 	private boolean reactToChange() {
 		OperatingSystem newOs = cboOperatingSystem.getItemAt(cboOperatingSystem.getSelectedIndex());
@@ -511,7 +521,9 @@ public class ImageDetailsWindow extends ImageDetailsWindowLayout implements UiFe
 				changed = true;
 			} else if (chkIsTemplate.isSelected() != image.isTemplate) {
 				changed = true;
-			} else if (permissionsChanged) {
+			} else if (!image.defaultPermissions.equals(originalDefaultPermissions)) {
+				changed = true;
+			} else if (PermsHelper.hasChanged(originalCustomPermissions, customPermissions)) {
 				changed = true;
 			}
 		}
@@ -521,6 +533,23 @@ public class ImageDetailsWindow extends ImageDetailsWindowLayout implements UiFe
 	}
 
 	/**
+	 * Enables/disables the editable fields based on 'editable'
+	 * 
+	 * @param editable true to make fields editable, false otherwise.
+	 */
+	private void makeEditable(boolean editable) {
+		editable = editable && (ImagePerms.canEdit(image) || ImagePerms.canAdmin(image));
+		txtTitle.setEditable(editable);
+		txtDescription.setEditable(editable);
+		txtTags.setEditable(editable);
+		cboOperatingSystem.setEnabled(editable);
+		//		cboShareMode.setEnabled(editable);
+		btnPermissions.setEnabled(editable && ImagePerms.canAdmin(image));
+		btnChangeOwner.setEnabled(editable && ImagePerms.canAdmin(image));
+		btnUpdateImage.setEnabled(editable);
+	}
+
+	/**
 	 * Opens a new ImageDetailsWindow showing the details of the image with ID =
 	 * imageBaseId
 	 * 
diff --git a/dozentenmodul/src/main/java/org/openslx/dozmod/permissions/DefaultCustomPerms.java b/dozentenmodul/src/main/java/org/openslx/dozmod/permissions/DefaultCustomPerms.java
index bb05d182..e7b58a56 100644
--- a/dozentenmodul/src/main/java/org/openslx/dozmod/permissions/DefaultCustomPerms.java
+++ b/dozentenmodul/src/main/java/org/openslx/dozmod/permissions/DefaultCustomPerms.java
@@ -15,5 +15,3 @@ public class DefaultCustomPerms<T>{
 		this.defaultPermissions = defaultPermissions;
 	}
 }
-
-
diff --git a/dozentenmodul/src/main/java/org/openslx/dozmod/permissions/PermsHelper.java b/dozentenmodul/src/main/java/org/openslx/dozmod/permissions/PermsHelper.java
new file mode 100644
index 00000000..d4a864dd
--- /dev/null
+++ b/dozentenmodul/src/main/java/org/openslx/dozmod/permissions/PermsHelper.java
@@ -0,0 +1,39 @@
+package org.openslx.dozmod.permissions;
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+
+import org.apache.log4j.Logger;
+import org.openslx.bwlp.thrift.iface.ImagePermissions;
+
+public class PermsHelper {
+
+	private static final Logger LOGGER = Logger.getLogger(PermsHelper.class);
+
+	private PermsHelper() {}
+
+	public static boolean hasChanged(final Map<String, ImagePermissions> oldMap, final Map<String, ImagePermissions> newMap) {
+		// build list of users that were added, if any return true
+		Set<String> addedUsers = new HashSet<String>(newMap.keySet());
+		addedUsers.removeAll(oldMap.keySet());
+		if (!addedUsers.isEmpty())
+			return true;
+		// build list of users that were removed, if any return true
+		Set<String> removedUsers = new HashSet<String>(oldMap.keySet());
+		removedUsers.removeAll(newMap.keySet());
+		if (!removedUsers.isEmpty())
+			return true;
+		// no changes in the users, lets check for changes in each users permissions
+		for (Entry<String, ImagePermissions> entry : oldMap.entrySet()) {
+			ImagePermissions current = entry.getValue();
+			ImagePermissions toCheck = newMap.get(entry.getKey());
+			if (!current.equals(toCheck))
+				return true;
+		}
+		// everything was the same if we are still here
+		return false;
+	}
+
+}