Bug 531792: [Tips] API cleanup

* Moved TipManager to internal
* Fixed all arising issues
* Added save read tips to preferences

Change-Id: Id557ab165f63ecabf87460671d1ec8e7ba5de522
Signed-off-by: Wim Jongman <wim.jongman@remainsoftware.com>

4 files changed, 89 insertions, 68 deletions

diff --git a/org.eclipse.tips.core/src/org/eclipse/tips/core/ITipManager.java b/org.eclipse.tips.core/src/org/eclipse/tips/core/ITipManager.java
index a722914b5..52fbe3434 100644
--- a/org.eclipse.tips.core/src/org/eclipse/tips/core/ITipManager.java
+++ b/org.eclipse.tips.core/src/org/eclipse/tips/core/ITipManager.java
@@ -11,9 +11,10 @@
 package org.eclipse.tips.core;
 
 import org.eclipse.core.runtime.IStatus;
+import org.eclipse.tips.core.internal.TipManager;
 
 /**
- * The model maintained by the TipManager.
+ * The ITipManager interface.
  *
  */
 public interface ITipManager {
@@ -29,8 +30,7 @@ public interface ITipManager {
 	/**
 	 * Consults TipManager to determine the Tip's read status.
 	 *
-	 * @param tip
-	 *            the tip to query for its read status
+	 * @param tip the tip to query for its read status
 	 * @return true if the tip is read, false otherwise.
 	 */
 	public abstract boolean isRead(Tip tip);
@@ -38,8 +38,7 @@ public interface ITipManager {
 	/**
 	 * Instructs the TipManager to mark this tip as read.
 	 *
-	 * @param tip
-	 *            the tip to set as read.
+	 * @param tip the tip to set as read.
 	 * @return this
 	 */
 	public abstract ITipManager setAsRead(Tip tip);
@@ -47,8 +46,7 @@ public interface ITipManager {
 	/**
 	 * Central place of logging for the Tip Framework.
 	 *
-	 * @param status
-	 *            the {@link IStatus} which may not be null
+	 * @param status the {@link IStatus} which may not be null
 	 * @return this
 	 */
 	public ITipManager log(IStatus status);
@@ -59,16 +57,29 @@ public interface ITipManager {
 	 * {@link TipProvider#loadNewTips(org.eclipse.core.runtime.IProgressMonitor)}
 	 * method.
 	 *
-	 * @param provider
-	 *            the {@link TipProvider} to register which may not be null.
+	 * @param provider the {@link TipProvider} to register which may not be null.
 	 * @return this
 	 */
 	public ITipManager register(TipProvider provider);
+	
+	/**
+	 * Opens the Tip of the Day dialog.
+	 *
+	 * @param startUp When called from a startup situation, true must be passed for
+	 *                <code>startup</code>. If in a manual starting situation,
+	 *                false must be passed. This enables the manager to decide to
+	 *                skip opening the dialog at startup (e.g., no new tip items).
+	 *
+	 * @return this
+	 *
+	 * @see #isOpen()
+	 */
+	public ITipManager open(boolean startUp);
 
 	/**
-	 * Returns the disposed stated.
+	 * Returns the open state.
 	 *
-	 * @return true if this manager is disposed, false otherwise.
+	 * @return true if this manager is open, false otherwise.
 	 */
-	public boolean isDisposed();
+	public boolean isOpen();
 }
\ No newline at end of file
diff --git a/org.eclipse.tips.core/src/org/eclipse/tips/core/TipProvider.java b/org.eclipse.tips.core/src/org/eclipse/tips/core/TipProvider.java
index 4bc72b118..e5f364716 100644
--- a/org.eclipse.tips.core/src/org/eclipse/tips/core/TipProvider.java
+++ b/org.eclipse.tips.core/src/org/eclipse/tips/core/TipProvider.java
@@ -15,6 +15,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
 import org.eclipse.core.runtime.IProgressMonitor;
@@ -55,7 +56,7 @@ public abstract class TipProvider {
 
 	private ITipManager fTipManager;
 	private int fTipIndex;
-	protected List<Tip> fTips = new ArrayList<>();
+	private List<Tip> fTips = new ArrayList<>();
 	private Tip fCurrentTip;
 	private boolean fReady;
 	private PropertyChangeSupport fChangeSupport = new PropertyChangeSupport(this);
@@ -63,6 +64,22 @@ public abstract class TipProvider {
 	private String fExpression;
 
 	/**
+	 * A predicate that tests if a tip is valid based on its read state and the
+	 * requirement to only serve read tips. Subclasses may replace this predicate if
+	 * they want to add some additional tests.
+	 */
+	private Predicate<Tip> fUnreadTipPredicate = new Predicate<Tip>() {
+
+		@Override
+		public boolean test(Tip pTip) {
+			if (getManager().mustServeReadTips()) {
+				return true;
+			}
+			return !getManager().isRead(pTip);
+		}
+	};
+
+	/**
 	 * The zero argument constructor must be able to instantiate the TipProvider.
 	 * This method may also be used to quickly set the available tips by calling the
 	 * {@link #setTips(List)} method. The constructor may not be used to load tips
@@ -98,34 +115,6 @@ public abstract class TipProvider {
 	public abstract TipImage getImage();
 
 	/**
-	 * Get a list of tips. The default implementation returns tips based on the
-	 * following conditions: <br>
-	 * <dl>
-	 * <dt><code>pFilter</code> is false</dt>
-	 * <dd>Return all read and unread tips.</dd>
-	 * <dt><code>pFilter</code> is true</dt>
-	 * <dd>Return read and unread tips if the tipManager may serve unread tips,
-	 * otherwise return only unread tips.</dd>
-	 * </dl>
-	 * <p>
-	 * Subclasses may override (calling super(false) to fetch the list) if they want
-	 * to serve or sort the list of tips in a different way.
-	 *
-	 * @param filter false or true, see description above.
-	 * @return an unmodifiable list of tips.
-	 */
-	public synchronized List<Tip> getTips(boolean filter) {
-		if (filter) {
-			return Collections.unmodifiableList(fTips //
-					.stream() //
-					.filter(tip -> getManager().mustServeReadTips() || !getManager().isRead(tip)) //
-					.sorted(Comparator.comparing(Tip::getCreationDate).reversed()) //
-					.collect(Collectors.toList()));
-		}
-		return Collections.unmodifiableList(fTips);
-	}
-
-	/**
 	 * @return the {@link Tip} that was last returned by {@link #getNextTip()} or
 	 *         {@link #getPreviousTip()}
 	 */
@@ -146,12 +135,11 @@ public abstract class TipProvider {
 	 * @see #getCurrentTip()
 	 */
 	public synchronized Tip getNextTip() {
-		boolean unreadOnly = !getManager().mustServeReadTips();
-		List<Tip> list = getTips(unreadOnly);
+		List<Tip> list = getTips(fUnreadTipPredicate);
 		if (list.isEmpty()) {
 			return setCurrentTip(fFinalTip);
 		}
-		if (!unreadOnly && fCurrentTip != null) {
+		if (getManager().mustServeReadTips() && fCurrentTip != null) {
 			fTipIndex++;
 		} else if (fCurrentTip != null && getManager().isRead(fCurrentTip)) {
 			fTipIndex++;
@@ -168,7 +156,7 @@ public abstract class TipProvider {
 	 * @see #getCurrentTip()
 	 */
 	public Tip getPreviousTip() {
-		List<Tip> list = getTips(!getManager().mustServeReadTips());
+		List<Tip> list = getTips(fUnreadTipPredicate);
 		if (list.isEmpty()) {
 			return setCurrentTip(fFinalTip);
 		}
@@ -239,6 +227,35 @@ public abstract class TipProvider {
 	}
 
 	/**
+	 * A convenience method to get the list of tips based on the read status of the
+	 * tip and the requirement to serve unread or all tips.
+	 * 
+	 * @return the list of tips based on the description above
+	 * @see ITipManager#mustServeReadTips()
+	 * @see ITipManager#isRead(Tip)
+	 */
+	public List<Tip> getTips() {
+		return getTips(fUnreadTipPredicate);
+	}
+
+	/**
+	 * Get a list of tips filtered by the passed predicate.
+	 *
+	 * @param a {@link Predicate} targeting a Tip object or null to return all tips
+	 * @return an unmodifiable list of tips.
+	 */
+	public synchronized List<Tip> getTips(Predicate<Tip> predicate) {
+		if (predicate != null) {
+			return Collections.unmodifiableList(fTips //
+					.stream() //
+					.filter(tip -> predicate.test(tip)) //
+					.sorted(Comparator.comparing(Tip::getCreationDate).reversed()) //
+					.collect(Collectors.toList()));
+		}
+		return Collections.unmodifiableList(fTips);
+	}
+
+	/**
 	 * Sets the tips for this provider, replacing the current set of tips, and sets
 	 * the <code>ready</code> flag to true. This method is typically called from the
 	 * constructor of the {@link TipProvider} but may also be called from the
@@ -251,7 +268,7 @@ public abstract class TipProvider {
 	 * @see #loadNewTips(IProgressMonitor)
 	 */
 	public TipProvider setTips(List<Tip> tips) {
-		if (getManager().isDisposed()) {
+		if (!getManager().isOpen()) {
 			return this;
 		}
 		doSetTips(tips, true);
diff --git a/org.eclipse.tips.core/src/org/eclipse/tips/core/TipManager.java b/org.eclipse.tips.core/src/org/eclipse/tips/core/internal/TipManager.java
index c09da82a8..4d2a8ace4 100644
--- a/org.eclipse.tips.core/src/org/eclipse/tips/core/TipManager.java
+++ b/org.eclipse.tips.core/src/org/eclipse/tips/core/internal/TipManager.java
@@ -8,7 +8,7 @@
  * Contributors:
  *    Wim Jongman <wim.jongman@remainsoftware.com> - initial API and implementation
  *****************************************************************************/
-package org.eclipse.tips.core;
+package org.eclipse.tips.core.internal;
 
 import java.beans.PropertyChangeEvent;
 import java.beans.PropertyChangeSupport;
@@ -19,7 +19,8 @@ import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
 
-import org.eclipse.tips.core.internal.LogUtil;
+import org.eclipse.tips.core.ITipManager;
+import org.eclipse.tips.core.TipProvider;
 
 /**
  * An abstract implementation of ITipManager with additional control API. While
@@ -32,7 +33,7 @@ public abstract class TipManager implements ITipManager {
 
 	private Map<String, TipProvider> fProviders = new HashMap<>();
 	private Map<Integer, List<String>> fProviderPrio = new TreeMap<>();
-	protected boolean fOpen;
+	private boolean fOpen;
 	private boolean fServeReadTips = false;
 	private boolean fIsDiposed;
 	private PropertyChangeSupport fChangeSupport = new PropertyChangeSupport(this);
@@ -74,7 +75,7 @@ public abstract class TipManager implements ITipManager {
 		checkDisposed();
 		log(LogUtil.info("Registering provider: " + provider.getID() + " : " + provider.getDescription()));
 		provider.setManager(this);
-		addToMaps(provider, new Integer(getPriority(provider)));
+		addToMaps(provider, Integer.valueOf(getPriority(provider)));
 		provider.getChangeSupport().addPropertyChangeListener(event -> {
 			if (event.getPropertyName().equals(TipProvider.PROP_READY)) {
 				PropertyChangeEvent newEvent = new PropertyChangeEvent(this, event.getPropertyName(), null, provider);
@@ -184,20 +185,6 @@ public abstract class TipManager implements ITipManager {
 	public abstract TipManager setRunAtStartup(boolean shouldRun);
 
 	/**
-	 * Opens the Tip of the Day dialog.
-	 *
-	 * @param startUp When called from a startup situation, true must be passed for
-	 *                <code>pStartup</code>. If in a manual starting situation,
-	 *                false must be passed. This enables the manager to decide to
-	 *                skip opening the dialog at startup (e.g., no new tip items).
-	 *
-	 * @return this
-	 *
-	 * @see #isOpen()
-	 */
-	public abstract TipManager open(boolean startUp);
-
-	/**
 	 * The default implementation disposes of this manager and all the TipProviders
 	 * when the dialog is disposed. Subclasses may override but must call super.
 	 */
@@ -219,13 +206,20 @@ public abstract class TipManager implements ITipManager {
 	}
 
 	/**
-	 * @return returns true if the tips are currently being displayed in some way.
+	 * @return true if this manager is currently open.
 	 */
+	@Override
 	public boolean isOpen() {
-		checkDisposed();
+		// checkDisposed();
+		if (isDisposed())
+			return false;
 		return fOpen;
 	}
 
+	protected void setOpen(boolean open) {
+		fOpen = open;
+	}
+
 	/**
 	 * Indicates whether read tips must be served or not. Subclasses could override,
 	 * to save the state somewhere, but must call super.
@@ -252,7 +246,6 @@ public abstract class TipManager implements ITipManager {
 		return fServeReadTips;
 	}
 
-	@Override
 	public boolean isDisposed() {
 		return fIsDiposed;
 	}
diff --git a/org.eclipse.tips.core/src/org/eclipse/tips/core/package-info.java b/org.eclipse.tips.core/src/org/eclipse/tips/core/package-info.java
index f63aef9af..f98e446bd 100644
--- a/org.eclipse.tips.core/src/org/eclipse/tips/core/package-info.java
+++ b/org.eclipse.tips.core/src/org/eclipse/tips/core/package-info.java
@@ -14,7 +14,7 @@
  * IDE you may use the tips extension point to register a new
  * {@link org.eclipse.tips.core.TipProvider}.If you want to run the Tip
  * framework outside of the IDE (e.g. in an RCP application) then you should
- * also implement your own {@link org.eclipse.tips.core.TipManager} and start
+ * also implement your own {@link org.eclipse.tips.core.internal.TipManager} and start
  * that with some action or at startup of your application.
  */
 package org.eclipse.tips.core;
\ No newline at end of file