Re: [xwiki-devs] [xwiki-notifications] r22900 - in platform: core/trunk/xwiki-core/src/main/resources xwiki-applications/trunk/watchlist/src/main/resources/XWiki xwiki-plugins/trunk/watchlist xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist - xwiki-devs@xwiki.org

8 Oct 2009

jvdrean (SVN) wrote:
...
  Author: jvdrean
 Date: 2009-08-25 15:03:08 +0200 (Tue, 25 Aug 2009)
 New Revision: 22900

 Added:

platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListEvent.java

platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListEventManager.java

platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListEventType.java

platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListJob.java

platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListJobManager.java

platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListNotifier.java

platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListStore.java
 Removed:

platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListJob.java
 Modified:
    platform/core/trunk/xwiki-core/src/main/resources/ApplicationResources.properties

platform/xwiki-applications/trunk/watchlist/src/main/resources/XWiki/WatchListManager.xml

platform/xwiki-applications/trunk/watchlist/src/main/resources/XWiki/WatchListMessage.xml
    platform/xwiki-plugins/trunk/watchlist/pom.xml

platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListPlugin.java

platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListPluginApi.java
 Log:
 XPWATCHLIST-55 : Make the watchlist plugin use the activitystream plugin to retrieve
events
 XPWATCHLIST-23 : Put the classes and objects diffs in the emails.
 XPWATCHLIST-34 : Exception in log for user not having watch list data
 XPWATCHLIST-35 : Watch list should support registering for full wikis
 XPWATCHLIST-40 : Watch list should support any interval for its notifications
 XPWATCHLIST-27 : WatchList send an exception by mail

 Modified:
platform/core/trunk/xwiki-core/src/main/resources/ApplicationResources.properties
 ===================================================================
 ---
platform/core/trunk/xwiki-core/src/main/resources/ApplicationResources.properties	2009-08-25
12:55:58 UTC (rev 22899)
 +++
platform/core/trunk/xwiki-core/src/main/resources/ApplicationResources.properties	2009-08-25
13:03:08 UTC (rev 22900)
 @@ -837,6 +837,10 @@
  watchlist.delete.ko=An error occurred while removing {0} from watchlist
  watchlist.create.object=Created WatchList storage object
  watchlist.save.object=Updated WatchList
 +watchlist.event.create=On {0}, the document has been created by {1}
 +watchlist.event.delete=On {0}, the document has been deleted by {1}
 +watchlist.event.update=On {0}, the document has been modified by {1}
 +watchlist.event.update.multiple=Between {0} and {1}, the document has been modified {2}
times, by {3} user(s): {4} 
/\ /\ /\
Replaced user(s) with: user{3,choice,0#s|1#|2#s}.

...
  Added:
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListEvent.java
 ===================================================================
 ---
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListEvent.java	
                       (rev 0)
 +++
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListEvent.java	2009-08-25
13:03:08 UTC (rev 22900)
 @@ -0,0 +1,504 @@ 
Why do we need both prefixedSpace and prefixedFullName? Can't we obtain 
the first from the latter?

...
  +    /**
 +     * Prefixed space in which the event happened.
 +     */
 +    private final String prefixedSpace;
 +
 +    /**
 +     * Prefixed document fullName in which the event happened.
 +     */
 +    private final String prefixedFullName; 
I prefer enums for this. Is there a good reason why this is a String?

...
  +    /**
 +     * Type of the event (example: "update").
 +     */
 +    private String type;
 + 
I don't like three lists that must be iterated in parallel. Why not a 
single lists with a custom data object?

...
  +
 +    /**
 +     * List of versions affected by this event. It will contain only one entry if the
event is not a composite event.
 +     */
 +    private List<String> versions;
 +
 +    /**
 +     * List of authors for this event. It will contain only one entry if the event is
not a composite event.
 +     */
 +    private List<String> authors;
 +
 +    /**
 +     * List of dates for this event. It will contain only one entry if the event is not
a composite event.
 +     */
 +    private List<Date> dates;
 +
 +    /**
 +     * Constructor.
 +     * 
 +     * @param activityEvent activity stream event to wrap
 +     */
 +    public WatchListEvent(ActivityEvent activityEvent)
 +    {
 +        this.activityEvents.add(activityEvent);
 +        type = activityEvent.getType();
 +        prefixedSpace = activityEvent.getWiki() + WatchListStore.WIKI_SPACE_SEP +
activityEvent.getSpace();
 +        prefixedFullName = activityEvent.getWiki() + WatchListStore.WIKI_SPACE_SEP +
activityEvent.getPage(); 
I'd move this piece of code in a separate method.

...
  +        int hash = 3;
 +        if (ActivityEventType.UPDATE.equals(activityEvent)) {
 +            hashCode = 42 * hash + prefixedFullName.hashCode() +
activityEvent.getType().hashCode();
 +        } else {
 +            hashCode =
 +                42 * hash + prefixedFullName.hashCode() +
activityEvent.getType().hashCode()
 +                    + activityEvent.getDate().hashCode();
 +        }
 +    }
 +
 +    /**
 +     * Add another event associated to this event.
 +     * 
 +     * @param event The event to add.
 +     */
 +    public void addEvent(WatchListEvent event)
 +    {
 +        if (ActivityEventType.DELETE.equals(event.getType())) {
 +            // If the document has been deleted, reset this event
 +            activityEvents.clear();
 +            type = event.getType();
 +            versions.clear();
 +            versions = null;
 +            authors.clear();
 +            authors = null;
 +            previousVersion = null;
 +            htmlDiff = null;
 +        } else if (ActivityEventType.UPDATE.equals(event.getType()) &&
ActivityEventType.DELETE.equals(getType())) {
 +            // If an update event had been fired before a delete, discard it
 +            return;
 +        }
 +
 +        activityEvents.add(event.getActivityEvent());
 +    } 
The following methods are not threadsafe at all. Should they be?

Why don't we build these lists as we add events? The current usage 
patterns probably don't add new events after retrieving the list of 
dates/authors, but this code has a dependency on the order in which 
methods are called, which is not good.

...
  +    /**
 +     * @return Get all the dates of a composite event, if this event is not a composite
this list will contain single
 +     *         entry.
 +     */
 +    public List<Date> getDates()
 +    {
 +        if (dates == null) {
 +            dates = new ArrayList<Date>(); 
I don't think this is needed, since isComposite simply checks the size 
of the list. It brings a marginal performance gain with the cost of 
increased code complexity.

...
  +            if (!isComposite()) {
 +                dates.add(getDate());
 +            } else {
 +                for (ActivityEvent event : activityEvents) {
 +                    dates.add(event.getDate());
 +                }
 +            }
 +        }
 +
 +        return dates;
 +    }
 +
 +    public List<String> getAuthors()
 +
 +    public List<String> getVersions()
 +
 +
 +    /**
 +     * @param classAttr The class of the div to create
 +     * @return a HTML div element
 +     */
 +    private Div createDiffDiv(String classAttr)
 +    { 
Oh no, please don't use jakarta-ecs. It's a dead project.

...
  +        Div div = new Div();
 +        div.setClass(classAttr);
 +        div.setStyle(HTML_STYLE_PLACEHOLDER_PREFIX + classAttr);
 +
 +        return div;
 +    }
 +
 +
 +    /**
 +     * Overriding of the default equals method.
 +     * 
 +     * @param obj the ActivityEvent to be compared with
 +     * @return True if the two events have been generated by the same document and are
equals or conflicting
 +     */
 +    @Override
 +    public boolean equals(Object obj)
 +    {
 +        if (this == obj) {
 +            return true;
 +        }
 +
 +        if (!(obj instanceof WatchListEvent)) {
 +            return false;
 +        } 
This is not correct, any implementation of equals must be commutative: 
this.equals(event) must return the same as event.equals(this).

...
  +        // At first this method was returning true
when the documents were the same and the events were the same type.
 +        // Since we don't want to keep update events for documents that have been
deleted this method has been modified
 +        // to a point were it performs something different from a equals(), it returns
true when obj is a delete event
 +        // and 'this' is an update event. See
WatchListEventManager#WatchListEventManager(Date, XWikiContext).
 +        // TODO: refactoring.
 +        WatchListEvent event = ((WatchListEvent) obj);
 +        return prefixedFullName.equals(event.getPrefixedFullName()) &&
WatchListEventType.UPDATE.equals(getType())
 +            && (WatchListEventType.UPDATE.equals(event.getType()) ||
WatchListEventType.DELETE.equals(event.getType()));
 +    }
 +} 
...
  Added:
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListJob.java
 ===================================================================
 ---
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListJob.java	
                       (rev 0)
 +++
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListJob.java	2009-08-25
13:03:08 UTC (rev 22900)
 @@ -0,0 +1,257 @@
 +
 +/** 
Why "abstract"? The documentation does not provide any information, the 
fact that it implements a Job can be seen from the list of implements.

...
  + * WatchList abstract implementation of Quartz's
Job.
 + * 
 + * @version $Id$
 + */
 +public class WatchListJob extends AbstractJob implements Job
 +{ 
These two methods are pretty generic, they should be moved somewhere 
else, in the core.

...
  +    /**
 +     * Initialize container context.
 +     * 
 +     * @param context The XWiki context.
 +     * @throws ServletException If the container initialization fails.
 +     */
 +    protected void initializeComponents(XWikiContext context) throws ServletException
 +
 +    /**
 +     * Clean the container context.
 +     */
 +    protected void cleanupComponents() 
...
  Modified:
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListPluginApi.java
 ===================================================================
 ---
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListPluginApi.java	2009-08-25
12:55:58 UTC (rev 22899)
 +++
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListPluginApi.java	2009-08-25
13:03:08 UTC (rev 22900)
 @@ -22,6 +22,7 @@ 
This change:

...
  -public class WatchListPluginApi extends PluginApi
 +public class WatchListPluginApi extends PluginApi<WatchListPlugin>
  { 
means that we don't need getWatchlistPlugin anymore, but getProtectedPlugin.

...
  -        return
getWatchListPlugin().getWatchedSpaces(getXWikiContext().getUser(),
 -            getXWikiContext()).contains(context.getDatabase() + ":" +
context.getDoc().getSpace());
 +        return getWatchListPlugin().getStore().getWatchedElements(context.getUser(),
ElementType.SPACE, context)
 +            .contains(context.getDatabase() + WatchListStore.WIKI_SPACE_SEP +
context.getDoc().getSpace());
      } 
...
  Added:
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListStore.java
 ===================================================================
 ---
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListStore.java	
                       (rev 0)
 +++
platform/xwiki-plugins/trunk/watchlist/src/main/java/com/xpn/xwiki/plugin/watchlist/WatchListStore.java	2009-08-25
13:03:08 UTC (rev 22900)
 @@ -0,0 +1,621 @@ 
@SuppressWarnings should only be used when there's no other way. It's 
better to fix the warning.

...
  +@SuppressWarnings("serial")
 +public class WatchListStore implements EventListener
 +{ 
These two are very generic, I think they should be defined in the XWiki 
Syntax.

...
  +    /**
 +     * Character used to separated wiki and space in XWiki model.
 +     */
 +    public static final String WIKI_SPACE_SEP = ":";
 +    
 +    /**
 +     * Character used to separated space and page in XWiki model.
 +     */
 +    public static final String SPACE_PAGE_SEP = "."; 

Enums are supposed to elliminate (or at least reduce) elseif 
programming. This could go in a method of the enum.

...
  +    private String
getWatchListClassPropertyForType(ElementType type)
 +    {
 +        if (ElementType.WIKI.equals(type)) {
 +            return WATCHLIST_CLASS_WIKIS_PROP;
 +        } else if (ElementType.SPACE.equals(type)) {
 +            return WATCHLIST_CLASS_SPACES_PROP;
 +        } else if (ElementType.DOCUMENT.equals(type)) {
 +            return WATCHLIST_CLASS_DOCUMENTS_PROP;
 +        } else {
 +            return StringUtils.EMPTY;
 +        }
 +    } 
-- 
Sergiu Dumitriu
http://purl.org/net/sergiu/