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/