Community
Participate
Working Groups
I20030129++ The type hierarchy is currently computed in the UI thread. We should do it in the background thread and should make it cancelable.
Not for 2.1
If fixed please remove work around TypeHierarchyViewPart.processOutstandingEvents
*** Bug 68542 has been marked as a duplicate of this bug. ***
We should look at this again in 3.4.
*** Bug 61833 has been marked as a duplicate of this bug. ***
The type hierarchy is not computed in the UI thread, but in the modal context thread. It is also cancellable. What it could be done is to make it non-blocking.
Created attachment 146846 [details] Patch As of now I have used the empty viewer and kept the empty message for the canceled viewer too. Markus, pls let me know if we need a separate message after cancel, Ill modify it accordingly.
Created attachment 146848 [details] Patch for IJavaHelpContextIds Pls add this to the original patch .. forgot to add this earlier.
When playing around a bit with your patch, I realized that the 'Cancel' button in the Type Hierarchy view toolbar is too prominent. In contrast to Search, Call Hierarchy, etc., the computation of a type hierarchy is usually so fast that people should rarely need to cancel. Please remove the button again. However, the view must implicitly cancel a computation when the user opens a new type hierarchy. With your patch, this blocks the UI until the old computation is done (synchronized block in TypeHierarchyLifeCycle#ensureRefreshedTypeHierarchy()). You have to detect this case, cleanly cancel the ongoing job, and start computation for the fresh target type. The view part title must be italic while the view is busy (computing hierarchy in the background). See /org.eclipse.platform.doc.isv/guide/workbench_jobs.htm in the Help ("Showing that a part is busy") for how to do this. Even with that, I think it's confusing that the view still shows the old contents during the computation. The user can still interact with the view, but when the computation is done, the content suddenly gets replaced. It would be better if the view became empty (white, not gray) immediately after the user action and stays empty until the hierarchy is available. I guess the easiest way to do that is to just clear the views (all three, since the user can toggle modes). The fRefreshHierarchyJob should be a user Job. This gives a nice progress dialog after a short delay (unless the user enabled "Always run in background"). And you cannot just catch all InvocationTargetExceptions and then do nothing in TypeHierarchyViewPart#updateInput(IJavaElement).
Created attachment 148399 [details] Patch (In reply to comment #9) > When playing around a bit with your patch, I realized that the 'Cancel' button > in the Type Hierarchy view toolbar is too prominent. In contrast to Search, > Call Hierarchy, etc., the computation of a type hierarchy is usually so fast > that people should rarely need to cancel. Please remove the button again. Removed the cancel button from the view. > > However, the view must implicitly cancel a computation when the user opens a > new type hierarchy. With your patch, this blocks the UI until the old > computation is done (synchronized block in > TypeHierarchyLifeCycle#ensureRefreshedTypeHierarchy()). You have to detect this > case, cleanly cancel the ongoing job, and start computation for the fresh > target type. > > The view part title must be italic while the view is busy (computing hierarchy > in the background). See /org.eclipse.platform.doc.isv/guide/workbench_jobs.htm > in the Help ("Showing that a part is busy") for how to do this. > > The fRefreshHierarchyJob should be a user Job. This gives a nice progress > dialog after a short delay (unless the user enabled "Always run in > background"). > Done. > > Even with that, I think it's confusing that the view still shows the old > contents during the computation. The user can still interact with the view, but > when the computation is done, the content suddenly gets replaced. It would be > better if the view became empty (white, not gray) immediately after the user > action and stays empty until the hierarchy is available. I guess the easiest > way to do that is to just clear the views (all three, since the user can toggle > modes). I have changed the behaviour now. Now the viewers are cleared as soon as the input is updated(F4 is pressed on a new selection) and then updated once computation is complete. > And you cannot just catch all InvocationTargetExceptions and then do nothing in > TypeHierarchyViewPart#updateInput(IJavaElement). The catch blocks are only required since we throw the exceptions from ensureRefreshedTypeHierarchy(...) and handle them in HierarchyInformationControl.setInput(..) only in the case of ctrl+T where the view part is null... which does not hold when type hierarchy is created in the view. Anyhow, I have added the exception handling code.
Created attachment 148408 [details] Updated Patch Pls take this one. The restore functionality was broken. Fixed it.
> > However, the view must implicitly cancel a computation when the user opens a > > new type hierarchy. With your patch, this blocks the UI until the old > > computation is done (synchronized block in > > TypeHierarchyLifeCycle#ensureRefreshedTypeHierarchy()). You have to detect this > > case, cleanly cancel the ongoing job, and start computation for the fresh > > target type. > Done. It still blocks with the latest patch. E.g. open Hierarchy of java.lang.Object and then try to open another hierarchy.
Created attachment 150907 [details] Patch
Created attachment 150908 [details] Patch (In reply to comment #13) > Created an attachment (id=150907) [details] > Patch (Pls ignore this one.. bugzilla timed out) (In reply to comment #12) > It still blocks with the latest patch. E.g. open Hierarchy of java.lang.Object > and then try to open another hierarchy. Fixed the block, does implicit cancel when users opens a new type hierarchy and shows empty viewer till new contents are replaced.
Canceling works fine now, but the view looks broken when a hierarchy computation is canceled. We shouldn't even show another state in the view when the computation has been canceled. The 'canceled' message is unnecessary, because the user already knows that she canceled the computation. The new state after canceling is more interesting: If the computation has been canceled because a new input type has been set, the view should just be updated with the new input element (and the empty tree viewer kept empty until the computation is done. If the computation has been explicitly canceled by the user, the view should go back to the default empty state that is e.g. shown after the user chose 'Clear History' from the toolbar button menu.
Created attachment 151768 [details] Updated Patch (In reply to comment #15) > Canceling works fine now, but the view looks broken when a hierarchy > computation is canceled. We shouldn't even show another state in the view when > the computation has been canceled. > The canceled message was to differentiate between the empty and the canceled states. But I think showing the default empty grey viewer on user canceled computations and showing empty viewer till hierarchy is updated looks more neat. I have modified the patch accordingly.
The UI transitions look OK now, but I found an NPE: While a long-running hierarchy computation is going on, open the history drop-down and choose Clear History. When the computation is done, this NPE is thrown: java.lang.NullPointerException at org.eclipse.jdt.internal.ui.typehierarchy.TypeHierarchyViewPart.getSelectableType(TypeHierarchyViewPart.java:1139) at org.eclipse.jdt.internal.ui.typehierarchy.TypeHierarchyViewPart.updateViewers(TypeHierarchyViewPart.java:589) at org.eclipse.jdt.internal.ui.typehierarchy.TypeHierarchyLifeCycle$3.run(TypeHierarchyLifeCycle.java:247) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:134) ... 23 more * TypeHierarchyViewPart: - It's very hard to review code that is not self-explanatory. You need to be more careful when choosing names, and you need to make sure that the doc really describes their current meaning: /** * Indicates whether the current viewer shown is the empty viewer. */ private boolean fIsShowingEmptyViewer= true; => What this field really does is this: * Indicates whether empty viewers should keep showing. If false, replace * them with fEmptyTypesViewer. => Rename the field to 'fKeepShowingEmptyViewers' and adapt Javadoc and getter/setter. Remove the empty line in the setter. - setCanceledViewer(boolean) has a bad name and is a confusing API that half of the time does nothing. Please rename to showEmptyViewer(), remove the parameter, and put the 'if' to the call sites. - fIsCancelImplicit would better be turned into its inverse (replace true <-> false) and called fRefreshHierarchyJobCanceledExplicitly - setViewerVisibility() and updateHierarchyViewer() should stay private * TypeHierarchyLifeCycle - fIsCancelImplicit: same as above, but name fRefreshHierarchyJobCanceledExplicitly - isRefreshJob() does not do what its name nor what it Javadoc says. The TypeHierarchyLifeCycle is not a refresh job, so you maybe meant isRefreshJobRunning()? But that's not what the implementation does (fRefreshHierarchyJob still references a job after computation has completed). Please clarify this and check whether the implementation is really OK like this. If you're confident about these last tweaks, please release them to HEAD and mark this bug fixed.
(In reply to comment #17) > - isRefreshJob() does not do what its name nor what it Javadoc says. The > TypeHierarchyLifeCycle is not a refresh job, so you maybe meant > isRefreshJobRunning()? But that's not what the implementation does > (fRefreshHierarchyJob still references a job after computation has completed). > Please clarify this and check whether the implementation is really OK like > this. > It should do what the Javadoc says, renamed it. Also added a null check and assignment to null so that the reference is not held after computation. Thanks! Released to HEAD.
Verified for 3.6 M4 with I20091207-1800
