Merge branch 'v1.1' of stp:openslx-ng/tutor-module into v1.1

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;
+	}
+
+}