Bugzilla – Full Text Bug Listing
| Summary: | [RCP][Workbench] StatusLineManager API is not flexible enough for RCP applications | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Matthew Hatem <Matthew_Hatem> | ||||
| Component: | UI | Assignee: | Nick Edgar <n.a.edgar> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | ed.burnette, n.a.edgar | ||||
| Version: | 3.0 | ||||||
| Target Milestone: | 3.0 M8 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows 2000 | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Matthew Hatem
Sorry I could not create an attachment for some reason. The following post will be the content of a patch. Index: src/org/eclipse/jface/action/IStatusLineManager.java
===================================================================
retrieving revision 1.3
diff -u -r1.3 IStatusLineManager.java
--- src/org/eclipse/jface/action/IStatusLineManager.java 6 Nov 2003
00:41:45 -0000 1.3
+++ src/org/eclipse/jface/action/IStatusLineManager.java 4 Mar 2004
09:47:50 -0000
@@ -12,6 +12,8 @@
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.swt.graphics.Image;
+import org.eclipse.swt.widgets.Composite;
+import org.eclipse.swt.widgets.Control;
/**
* The <code>IStatusLineManager</code> interface provides protocol
@@ -98,4 +100,8 @@
* @param message the message, or <code>null</code> for no message
*/
public void setMessage(Image image, String message);
+
+
+public Control getControl();
+public Control createControl(Composite parent);
}
Index: src/org/eclipse/jface/action/StatusLineManager.java
===================================================================
retrieving revision 1.6
diff -u -r1.6 StatusLineManager.java
--- src/org/eclipse/jface/action/StatusLineManager.java 9 Oct 2003 17:08:16 -
0000 1.6
+++ src/org/eclipse/jface/action/StatusLineManager.java 4 Mar 2004 09:47:52 -
0000
@@ -50,7 +50,7 @@
* @param parent the parent control
* @return the status line control
*/
-public StatusLine createControl(Composite parent) {
+public Control createControl(Composite parent) {
if (!statusLineExist() && parent != null) {
statusLine= new StatusLine(parent);
update(false);
Index: src/org/eclipse/jface/action/SubStatusLineManager.java
===================================================================
retrieving revision 1.4
diff -u -r1.4 SubStatusLineManager.java
--- src/org/eclipse/jface/action/SubStatusLineManager.java 22 May 2003
19:14:34 -0000 1.4
+++ src/org/eclipse/jface/action/SubStatusLineManager.java 4 Mar 2004
09:47:53 -0000
@@ -13,6 +13,8 @@
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.swt.graphics.Image;
+import org.eclipse.swt.widgets.Composite;
+import org.eclipse.swt.widgets.Control;
/**
* A <code>SubStatusLineManager</code> is used to define a set of contribution
@@ -134,5 +136,17 @@
// call <code>setVisible</code> and then force an update. At
that
// point we need to update the parent.
getParentStatusLineManager().update(force);
+ }
+ /* (non-Javadoc)
+ * @see org.eclipse.jface.action.IStatusLineManager#createControl
(org.eclipse.swt.widgets.Composite)
+ */
+ public Control createControl(Composite parent) {
+ return getParentStatusLineManager().createControl(parent);
+ }
+ /* (non-Javadoc)
+ * @see org.eclipse.jface.action.IStatusLineManager#getControl()
+ */
+ public Control getControl() {
+ return getParentStatusLineManager().getControl();
}
}
Index: src/org/eclipse/jface/window/ApplicationWindow.java
===================================================================
retrieving revision 1.14
diff -u -r1.14 ApplicationWindow.java
--- src/org/eclipse/jface/window/ApplicationWindow.java 28 Feb 2004 00:06:39 -
0000 1.14
+++ src/org/eclipse/jface/window/ApplicationWindow.java 4 Mar 2004 09:47:59 -
0000
@@ -33,6 +33,7 @@
import org.eclipse.jface.action.CoolBarManager;
import org.eclipse.jface.action.MenuManager;
+import org.eclipse.jface.action.IStatusLineManager;
import org.eclipse.jface.action.StatusLineManager;
import org.eclipse.jface.action.ToolBarManager;
import org.eclipse.jface.operation.IRunnableContext;
@@ -86,7 +87,7 @@
*
* @see #addStatusLine
*/
- private StatusLineManager statusLineManager = null;
+ private IStatusLineManager statusLineManager = null;
/**
* Cool bar manager, or <code>null</null> if none (default).
@@ -137,8 +138,8 @@
hide = true;
result.y+= BAR_SIZE;
}
- }else if (statusLineManager != null &&
statusLineManager.getControl() == w) {
- } else if (i > 0) { /* we assume this window is
contents */
+ }
+ else if (i > 0) { /* we assume this window is
contents */
hide= false;
}
@@ -361,7 +362,7 @@
* </p>
* @return a status line manager
*/
-protected StatusLineManager createStatusLineManager() {
+protected IStatusLineManager createStatusLineManager() {
return new StatusLineManager();
}
/**
@@ -450,7 +451,7 @@
* this window does not have a status line
* @see #addStatusLine
*/
-protected StatusLineManager getStatusLineManager() {
+protected IStatusLineManager getStatusLineManager() {
return statusLineManager;
}
@@ -523,7 +524,7 @@
public void run(final boolean fork, boolean cancelable, final
IRunnableWithProgress runnable) throws InvocationTargetException,
InterruptedException {
try {
operationInProgress = true;
- final StatusLineManager mgr = getStatusLineManager();
+ final IStatusLineManager mgr = getStatusLineManager();
if (mgr == null) {
runnable.run(new NullProgressMonitor());
return;
Index: Eclipse UI/org/eclipse/ui/internal/WorkbenchWindow.java
===================================================================
retrieving revision 1.175
diff -u -r1.175 WorkbenchWindow.java
--- Eclipse UI/org/eclipse/ui/internal/WorkbenchWindow.java 1 Mar 2004
19:03:43 -0000 1.175
+++ Eclipse UI/org/eclipse/ui/internal/WorkbenchWindow.java 4 Mar 2004
09:46:01 -0000
@@ -28,9 +28,9 @@
import org.eclipse.jface.action.IContributionItem;
import org.eclipse.jface.action.IContributionManager;
import org.eclipse.jface.action.IMenuManager;
+import org.eclipse.jface.action.IStatusLineManager;
import org.eclipse.jface.action.MenuManager;
import org.eclipse.jface.action.Separator;
-import org.eclipse.jface.action.StatusLineManager;
import org.eclipse.jface.action.ToolBarContributionItem;
import org.eclipse.jface.action.ToolBarManager;
import org.eclipse.jface.operation.IRunnableWithProgress;
@@ -851,7 +851,7 @@
* this window does not have a status line
* @see #addStatusLine
*/
- public StatusLineManager getStatusLineManager() {
+ public IStatusLineManager getStatusLineManager() {
return super.getStatusLineManager();
}
This patch address issue 3, subclassing. I like this idea too. Somebody should nominate Matthew for committer, it would save a lot of time. Padwan must learn about breaking API changes <g>. Matt, sorry but we can't accept the patch as-is because it breaks several API methods by changing the return type. This breaks callers as well as overriders. Also, adding methods to interfaces that don't say "This interface is not intended to be implemented by clients" is also a breaking change to existing implementors. I'm OK with adding a creation method to the interface, but this will have to be done by introducing a new interface like IStatusLineManager2 or a mix-in interface like IContributionManagerControlCreation. Instead of passing the style bits into the constructor of the manager, I think these should get passed to the createControl method. This would also avoid conflicting with the existing createControl methods. I'm sorry Obi; I should have explained the patch in more detail. First off, I would never have expected something like this to get committed. It's more or less a preview of how we would like to see the statusline code evolve. And how it should have been done in the first place <g>. Second, yes this is an API breakage, but the API is already broken. The return type for createControl is package private. A public method that is API should never have a return type that is package private. And it's impossible to subclass this thing because of that. You might as well remove the comment from the javadoc. I'd be very surprised if anyone out there is subclassing this thing. I hear you on the additional methods but I had already broken (actually fixed) the API with createControl. Future patches will not have API breakages (fixes <g>). -m@ The force is strong in you. Your assessment of the problems with the current APIs is bang on. However, we still can't break existing clients. The only clients that should be affected by the ApplicationWindow and StatusLineManager changes are standalone JFace clients, not Workbench clients, but there are people out there using JFace standalone. I think it would make sense to align this with the work to separate out part presentations (bug 53673). For example, we could add the following to AbstractPresentationFactory: public IStatusLineManager createWindowStatusLineManager(); public Control createWindowStatusLineControl(IStatusLineManager); With these two methods, we give the app flexibility over which manager implementation it uses, and which controls it creates, without having to define new API for IStatusLineManager. Would it suffice just to change the return type of StatusLineManager.createControl to be Control? Since StatusLine is package- private, nobody other than JFace can be calling this now anyway so I wouldn't consider this to be a breaking API change. Changing the return type is a good start <g>. What do you think about an abstract StatusLine control? We could make all public methods on the existing StatusLine abstract. I don't think this breaks API. So what does this solve. Currently, if a StatusLineManager subclass refers to a status delegate that is not of type StatusLine, the subclass would need to override just about every method that referred to the StatusLine. This is not good, I'd hate to have to rewrite update(boolean) from scratch <g>. I'll provide a patch that will hopefully provide some clarity. The above does not completely address 1 and 2, but the presentation API can probably solve 4. -m@ For 1, I see this as the same as 3. We need a default implementation that fully implements the IStatusLineManager contract, including getProgressMonitor (). For 2, I like the suggestion and we should just add it to the base. Can you contribute this? For 3, if the statusLine field is changed to be Composite, and the return type of createControl changed to Control, then StatusLineManager becomes easily subclassable and you don't need to reimplement update. Having an abstract StatusLine control would allow for more code sharing though. I'm fine with that approach. For 4, we will add the ability to provide a different status line manager in the presentation factory. > For 2, I like the suggestion and we should just add it to the base. Can you
contribute this?
Yes. I'd be happy to.
And I'll take the abstract StatusLine approach and provide a separate patch for
that.
-m@
Created attachment 8512 [details]
Fixes subclassing issues
This patch should address all of the subclassing issues without breaking API.
Nick, you may not like the following.
1) getProgressMonitorDelegate - this was added to allow us to share as much
code as possible. A new IProgressMonitorDelegate may allow us to share even
more code. I'm okay with removing this new method, we'll just have to rewrite
the getProgressMonitor() method, sigh...
2) the naming convention for the default group markers. I'm fine with changing
the names to whatever you see fit.
I'm still looking into refactoring the StatusLine control. I'm investigating
how much code we can actually share from this class. It may be pointless.
The following are other problems that will need to be addressed to allow subclassing: - update assumes that there are 3 special controls being created by StatusLine - StatusLine uses StatusLineLayout as its layout. This expects StatusLineLayoutData (or null) from getLayoutData() on the controls in the status line (whether from StatusLine or ContributionItems). Some existing contribution items refer to StatusLineLayoutData, e.g. org.eclipse.ui.texteditor.StatusLineContributionItem. This prevents subclassers of StatusLineManager from using different layouts in their Composite. It would be better for StatusLineLayout to just use Control.computeSize on the child controls. Patch released. Leaving open to address 4. Note: the fix for bug 54436 has removed the magic number 3 from update(). Released enhancement for 4. See: - IWorkbenchWindowConfigurer.setPresentationFactory (AbstractPresentationFactory) AbstractPresentationFactory.createStatusLineManager() AbstractPresentationFactory.createStatusLineControl(IStatusLineManager, Composite) Matt, what are your thoughts about StatusLineLayoutData on comment #13? I'm hoping this is something we can defer. Re comment 17 and 13, yes I think this is something that we can defer. Seems like it will be up to the subclass to honor the LayoutData for contributions that use it. A simple computeSize() should work in most cases. I don't think we have fully addressed issue 1). This is my fault. I'm looking into pulling the 3 hard-wired controls out of the StatusLine and wrapping them into a SubStatusLine contribution. Subclasses can omit or replace this with their own implementation. What do you think? I would like to do this without forcing clients to reinvent StatusLine. Are you open to a new interface? This interface would provide the following: isCancelEnabled() setCancelEnabled(boolean) setErrorMessage(String) setErrorMessage(Image, String) setMessage(String) setMessage(Image, String) clearProgress() setProgressMessage(String) It could extend IProgressMonitor and be obtained through getProgressMonitorDelegate. Do you consider this an API breakage? getProgressMonitorDelegate hasn't been around for too long. <g> -m@ I was wondering, what's going on with the status line in recent I builds? There seems to be essentially two progress bars, one for foreground activities (native progress with a big red square stop button) and one for background activities (in an inset window, used to be floating but now in the status bar). Is there a plan to simplify this in the IDE and does that impact the new API being discussed? Using contribution items instead of a StatusLine makes sense. I think the real problem is that you may want to allow client contributions to get placed relative to the message control, progress control and cancel control independently (they don't need to be grouped together). I don't think SubContributionItem is the right approach though. That is a goofy interface intended to be used with SubContributionManagers for virtualizing contribution managers between the workbench and regular plugins. Perhaps the way the Workbench does it would work, where the app gets to contribute items first, then regular plugins get to contribution items relative to those. Except in this case, this would be done in StatusLineManager itself, but in a way that's subclassable. Re comment #19, various approaches have been tried, including overloading the main status line (there are some vestigial traces of this in IStatusLineWithProgressManager, which StatusLineManager implements, but this should be removed as it is not currently used by the background progress reporting mechanism). The current progress reporting mechanism is likely to be the final shape for 3.0. Even if it changes, the impact on IStatusLineManager and subclassing decisions would be minimal. If you have concerns about how progress reporting for background jobs is being done, please enter a separate bug report, or add your comments to one of the existing ones like bug 54748. Matt, given all the other loose ends that need to be tied up for M8, I suggest we stick with what we have now here, assuming that the patch you've contributed give you enough flexibility to change things in a subclass if needed. Please reopen if you disagree. No further work planned here for 3.0. Please open new PRs if there are other issues with the status line manager for RCP apps. |