Author: nbelaevski
Date: 2009-03-27 11:51:22 -0400 (Fri, 27 Mar 2009)
New Revision: 13267
Modified:
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/AlbumManager.java
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/Authenticator.java
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/Controller.java
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/DnDManager.java
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/ImageManager.java
Log:
Code review results committed
Modified:
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/AlbumManager.java
===================================================================
---
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/AlbumManager.java 2009-03-27
15:40:29 UTC (rev 13266)
+++
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/AlbumManager.java 2009-03-27
15:51:22 UTC (rev 13267)
@@ -57,10 +57,13 @@
public void addAlbum(Album album){
if(album.getShelf() == null){
+ //TODO nick - remove null argument
+ //TODO nick - externalize message
facesMessages.add("Shelf must be not-null", null);
return;
}
albumAction.addAlbum(album);
+ //TODO nick - addDirectory() uses this.album, do we need: this.album = album?
addDirectory();
model.resetModel(model.getMainArea(), model.getSelectedUser(),
model.getSelectedShelf(), album, null);
Events.instance().raiseEvent("albumAdded");
@@ -83,7 +86,9 @@
//Update domain model
String string = album.getId().toString();
albumAction.deleteAlbum(album);
+ //TODO nick - should be deleteDirectory(album). And call file manager directly
deleteDirectory(string);
+ //TODO nick - why not call just model.setAlbum(null). More: is deleted album the ones
currently selected in model?
model.resetModel(NavigationEnum.ALL_ALBUMS, model.getSelectedUser(),
model.getSelectedShelf(), null, null);
Events.instance().raiseEvent("albumDeleted");
Events.instance().raiseEvent("clearTree");
Modified:
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/Authenticator.java
===================================================================
---
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/Authenticator.java 2009-03-27
15:40:29 UTC (rev 13266)
+++
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/Authenticator.java 2009-03-27
15:51:22 UTC (rev 13267)
@@ -31,7 +31,6 @@
import org.jboss.seam.annotations.Out;
import org.jboss.seam.annotations.Scope;
import org.jboss.seam.annotations.web.RequestParameter;
-import org.jboss.seam.contexts.Contexts;
import org.jboss.seam.core.Conversation;
import org.jboss.seam.faces.FacesMessages;
import org.jboss.seam.log.Log;
@@ -79,6 +78,7 @@
public String login(){
if(identity.hasRole("admin")){
+ //TODO nick - return null
return "";
}
String password = credentials.getPassword();
@@ -88,9 +88,10 @@
credentials.setUsername(username);
try {
identity.authenticate();
+ //TODO nick - maybe clearShelves() before trying to aunthenticate?
shelfManager.clearShelfs();
} catch (LoginException e) {
- // TODO Auto-generated catch block
+ // TODO nick - Auto-generated catch block
e.printStackTrace();
}
return "main";
@@ -99,11 +100,13 @@
public boolean authenticate()
{
if (wantLoginAnonymous()) {
+ //TODO nick - remove another roles?
identity.addRole(Constants.GUEST_ROLE);
model.setMainArea(NavigationEnum.ANONYM);
return true;
}
try {
+ //TODO nick - move password holder into userAction
user = userAction.login(credentials.getUsername(),
passwordHolder.hash(credentials.getPassword()));
if (user != null) {
identity.addRole(Constants.ADMIN_ROLE);
@@ -151,6 +154,7 @@
}
public String startConversation(){
+ //TODO nick - rename to destroyConversation()?
identity.logout();
identity.unAuthenticate();
credentials.clear();
Modified:
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/Controller.java
===================================================================
---
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/Controller.java 2009-03-27
15:40:29 UTC (rev 13266)
+++
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/Controller.java 2009-03-27
15:51:22 UTC (rev 13267)
@@ -53,6 +53,7 @@
private User user;
public void selectShelfs(){
+ //TODO nick - i think model should reset selected albums, etc. internally
model.resetModel(NavigationEnum.ALL_SHELFS, user, null, null, null);
}
@@ -73,6 +74,7 @@
}
public void cancelEditShelf(Shelf shelf){
+ //TODO nick - unused "shelf" argument
model.setMainArea(NavigationEnum.SHELF_PREVIEW);
}
@@ -90,6 +92,7 @@
return;
}
model.resetModel(NavigationEnum.ALBUM_IMAGE_PREVIEW, image.getAlbum().getOwner(),
image.getAlbum().getShelf(), image.getAlbum(), image);
+ //TODO nick - model should already has enough information to set up image index
model.setSelectedImageIndex(model.getSelectedAlbum().getIndex(model.getSelectedImage())+1);
image.getAlbum().getShelf().visitImage(image);
image.getAlbum().visitImage(image);
@@ -101,6 +104,7 @@
Image image = model.getSelectedAlbum().getImages().get(index - 1);
model.resetModel(NavigationEnum.ALBUM_IMAGE_PREVIEW, image.getAlbum().getOwner(),
image.getAlbum().getShelf(), image.getAlbum(), image);
image.getAlbum().visitImage(image);
+ //TODO nick - album can notify shelf
image.getAlbum().getShelf().visitImage(image);
}
@@ -109,10 +113,12 @@
pushEvent(Constants.ADD_ERROR_EVENT, new Exception(HAVENT_ACCESS));
return;
}
+ //TODO nick - resetModel?
model.setMainArea(NavigationEnum.ALBUM_IMAGE_EDIT);
}
public void cancelEditImage(Image image){
+ //TODO nick - unused image argument
model.setMainArea(NavigationEnum.ALBUM_IMAGE_PREVIEW);
}
@@ -124,6 +130,7 @@
if(image == null || image.getAlbum() == null || image.getAlbum().getOwner() == null){
return false;
}
+ //TODO nick - compare User objects?
return image.getAlbum().getOwner().getLogin().equals(user.getLogin());
}
@@ -132,11 +139,13 @@
pushEvent(Constants.ADD_ERROR_EVENT, new Exception(HAVENT_ACCESS));
return;
}
+ //TODO nick - resetModel?
model.setSelectedAlbum(album);
model.setMainArea(NavigationEnum.ALBUM_EDIT);
}
public void cancelEditAlbum(Album album){
+ //TODO nick - unused "album" argument
model.setMainArea(NavigationEnum.ALBUM_PREVIEW);
}
@@ -158,6 +167,7 @@
}
public boolean isUserShelf(Shelf shelf){
+ //TODO nick - compare User objects?
return shelf.getOwner()!=null &&
shelf.getOwner().getLogin().equals(user.getLogin());
}
@@ -165,6 +175,7 @@
if(album == null || album.getOwner() == null){
return false;
}
+ //TODO nick - compare User objects?
return album.getOwner().getLogin().equals(user.getLogin());
}
@@ -181,6 +192,7 @@
}
public void showTag(MetaTag metatag){
+ //TODO nick - model.setMainArea()?
model.resetModel(NavigationEnum.TAGS, model.getSelectedUser(),
model.getSelectedShelf(), model.getSelectedAlbum(), model.getSelectedImage());
model.setSelectedTag(metatag);
}
@@ -205,6 +217,7 @@
if(selectedUser == null){
return false;
}
+ //TODO nick - equals()?
if(selectedUser == user){
return true;
}
Modified:
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/DnDManager.java
===================================================================
---
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/DnDManager.java 2009-03-27
15:40:29 UTC (rev 13266)
+++
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/DnDManager.java 2009-03-27
15:51:22 UTC (rev 13267)
@@ -52,11 +52,13 @@
}
handleImage((Image)dragValue, (Album)dropValue);
}else if(dragValue instanceof Album){
+ //TODO nick - check user
handleAlbum((Album)dragValue, (Shelf)dropValue);
}
}
private void handleAlbum(Album dragValue, Shelf dropValue) {
+ //TODO nick - add check for the current shelf
dropValue.addAlbum(dragValue);
albumAction.editAlbum(dragValue);
Events.instance().raiseEvent("clearTree");
@@ -74,8 +76,10 @@
private String getNewPathOfImage(Image dragValue, Album dropValue) {
String fileNameOld = dragValue.getPath();
+ //TODO nick - is "/" correct for windows?
int lastIndexOf = dragValue.getPath().lastIndexOf("/");
String prevPathEnd = dragValue.getPath().substring(lastIndexOf);
+ //TODO nick - file manager should handle naming!!!
String fileNameNew =
user.getLogin()+"/"+dropValue.getId()+"/"+prevPathEnd;
fileManager.renameImage(fileNameOld, fileNameNew);
return fileNameNew;
Modified:
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/ImageManager.java
===================================================================
---
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/ImageManager.java 2009-03-27
15:40:29 UTC (rev 13266)
+++
trunk/test-applications/realworld2/web/src/main/java/org/richfaces/realworld/manager/ImageManager.java 2009-03-27
15:51:22 UTC (rev 13267)
@@ -58,8 +58,11 @@
private User user;
public void deleteImage(Image image) {
+ //TODO nick - should we check image owner?
String imagePath = image.getPath();
model.resetModel(NavigationEnum.ALBUM_PREVIEW, image.getAlbum().getOwner(),
image.getAlbum().getShelf(), image.getAlbum(), null);
+
+ //TODO nick - imageAction can call file manager itself
imageAction.deleteImage(image);
fileManager.deleteImage(imagePath);
Events.instance().raiseEvent("imageDeleted");
@@ -71,6 +74,7 @@
Events.instance().raiseEvent("imageEdited");
}
+ //TODO nick - Constants.ADD_IMAGE_EVENT is not used anywhere else
@Observer(Constants.ADD_IMAGE_EVENT)
public void addImage(Image image) {
imageAction.addImage(image);