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
_______________________________________________
devs mailing list
devs@xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
