On Oct 26, 2010, at 9:19 PM, Sergiu Dumitriu wrote:

On 10/26/2010 07:43 PM, Vincent Massol wrote:

...

On Oct 26, 2010, at 6:12 PM, sdumitriu (SVN) wrote:

...

Author: sdumitriu Date: 2010-10-26 18:12:09 +0200 (Tue, 26 Oct 2010) New Revision: 32196

Modified: platform/xwiki-plugins/trunk/activitystream/pom.xml platform/xwiki-plugins/trunk/activitystream/src/main/java/com/xpn/xwiki/plugin/activitystream/api/ActivityEventType.java platform/xwiki-plugins/trunk/activitystream/src/main/java/com/xpn/xwiki/plugin/activitystream/impl/ActivityStreamImpl.java Log: XPAS-19: New event types for create, edit and delete comment XPAS-20: New event types for create, edit and delete attachment XPAS-21: New event type for create, edit and delete annotation Done. Patch from Stefan Abageru applied without changes.

[snip]

...

@@ -87,6 +96,15 @@ add(new DocumentSaveEvent()); add(new DocumentUpdateEvent()); add(new DocumentDeleteEvent()); + add(new CommentAddEvent()); + add(new CommentDeleteEvent()); + add(new CommentUpdateEvent()); + add(new AttachmentAddEvent()); + add(new AttachmentDeleteEvent()); + add(new AttachmentUpdateEvent()); + add(new AnnotationAddEvent()); + add(new AnnotationDeleteEvent()); + add(new AnnotationUpdateEvent());

hmm this doesn't look right. I don't think the activity stream should know about all modules. Also this won't work when you add a new module which offers new events. There's a design problem.

...

} };

@@ -875,21 +893,61 @@ if (!Utils.getComponent(RemoteObservationManagerContext.class).isRemoteState()) { String eventType; String displayTitle; - + String additionalIdentifier = null; + if (event instanceof DocumentSaveEvent) { eventType = ActivityEventType.CREATE; displayTitle = currentDoc.getDisplayTitle(context); } else if (event instanceof DocumentUpdateEvent) { eventType = ActivityEventType.UPDATE; displayTitle = originalDoc.getDisplayTitle(context); - } else { // event instanceof DocumentDeleteEvent + } else if (event instanceof DocumentDeleteEvent) { eventType = ActivityEventType.DELETE; displayTitle = originalDoc.getDisplayTitle(context); + } else if (event instanceof CommentAddEvent) { + eventType = ActivityEventType.ADD_COMMENT; + displayTitle = currentDoc.getDisplayTitle(context); + additionalIdentifier = ((CommentAddEvent) event).getComment(); + } else if (event instanceof CommentDeleteEvent) { + eventType = ActivityEventType.DELETE_COMMENT; + displayTitle = currentDoc.getDisplayTitle(context); + additionalIdentifier = ((CommentDeleteEvent) event).getComment(); + } else if (event instanceof CommentUpdateEvent){ + eventType = ActivityEventType.UPDATE_COMMENT; + displayTitle = currentDoc.getDisplayTitle(context); + additionalIdentifier = ((CommentUpdateEvent) event).getComment(); + } else if (event instanceof AttachmentAddEvent){ + eventType = ActivityEventType.ADD_ATTACHMENT; + displayTitle = currentDoc.getDisplayTitle(context); + additionalIdentifier = ((AttachmentAddEvent) event).getName(); + } else if (event instanceof AttachmentDeleteEvent){ + eventType = ActivityEventType.DELETE_ATTACHMENT; + displayTitle = currentDoc.getDisplayTitle(context); + additionalIdentifier = ((AttachmentDeleteEvent) event).getName(); + } else if (event instanceof AttachmentUpdateEvent){ + eventType = ActivityEventType.UPDATE_ATTACHMENT; + displayTitle = currentDoc.getDisplayTitle(context); + additionalIdentifier = ((AttachmentUpdateEvent) event).getName(); + } else if (event instanceof AnnotationAddEvent){ + eventType = ActivityEventType.ADD_ANNOTATION; + displayTitle = currentDoc.getDisplayTitle(context); + additionalIdentifier = ((AnnotationAddEvent) event).getIdentifier(); + } else if (event instanceof AnnotationDeleteEvent){ + eventType = ActivityEventType.DELETE_ANNOTATION; + displayTitle = currentDoc.getDisplayTitle(context); + additionalIdentifier = ((AnnotationDeleteEvent) event).getIdentifier(); + } else { // update annotation + eventType = ActivityEventType.UPDATE_ANNOTATION; + displayTitle = currentDoc.getDisplayTitle(context); + additionalIdentifier = ((AnnotationUpdateEvent) event).getIdentifier();

This also looks like a design issue. It's a lot of "if"s and duplicated code.

[snip]

In general this whole commit really smells like some code that was done without refactoring in mind. It shows problems but the time wasn't taken to refactor the issues IMO. Let's do that now since otherwise we're introducing technical debt.

Yes, both me and Ștefan disliked this a lot, but this is all we can do in a very busy day. It hurt me a lot to commit this many else-ifs. If there wasn't such a tight deadline, I'd take the time to refactor the whole activity stream into nice components, but the RC is in a few days and I still have a lot of things to do.

oh this is a prereq for the Recent Activity feature (rewrite of the old recent changes)? If so I wasn't aware (didn't realizer) and I understand the urgency. Let's just remember to fix this as soon as we can. Thanks -Vincent