Home » Eclipse Projects » Remote Application Platform (RAP) » Bug with fix contributed hasn't made it into code base in two years
| Bug with fix contributed hasn't made it into code base in two years [message #1197504] |
Tue, 19 November 2013 18:32  |
Eclipse User |
|
|
|
I wonder if a RAP committer could look into this bug which seems to have fallen through the cracks.
A fix was uploaded to bugzilla two years ago, but it hasn't yet made it into the code base. I discovered the bug in RAP 1.5, but a comment in the bug thread says it's still not fixed in RAP 2.1. Was it perhaps fixed in one of the RAP 2.2 milestones? If not, can it be included in an upcoming release?
It's a simple fix, but I ran into problems trying to build RCP products with a patched version of SWT in the past. There was a mysterious problem with PDE build that we were unable to resolve. We're using Maven now, so maybe we would fare better, but I'm not keen to try it. Without the fix, DeferredTreeContentManagers are not usable in RAP.
[Updated on: Tue, 19 November 2013 18:35] by Moderator |
|
|
| Re: Bug with fix contributed hasn't made it into code base in two years [message #1198140 is a reply to message #1197504] |
Wed, 20 November 2013 01:58  |
| Eclipse User |
|
|
|
Hi Mark,
did you tested the attached to the bug patch? Does it fix the problem?
Best,
Ivan
On 11/20/2013 1:32 AM, Mark Leone wrote:
> I wonder if a RAP committer could look into
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=364661 which seems to
> have fallen through the cracks.
>
> A fix was uploaded to bugzilla two years ago, but it hasn't yet made
> it into the code base. I discovered the bug in RAP 1.5, but a comment
> in the bug thread says it's still not fixed in RAP 2.1. Was it perhaps
> fixed in one of the RAP 2.2 milestones? If not, can it be included in
> an upcoming release?
>
> It's a simple fix, but I ran into problems trying to build RCP
> products with a patched version of SWT in the past. There was a
> mysterious problem with PDE build that we were unable to resolve.
> We're using Maven now, so maybe we would fare better, but I'm not keen
> to try it. Without the fix, DeferredTreeContentAdapters are not usable
> in RAP.
--
Ivan Furnadjiev
Twitter: @EclipseRAP
Blog: http://eclipsesource.com/blogs/
Professional services for RAP and RCP?
http://eclipsesource.com/services/rap/
|
|
|
| Re: Bug with fix contributed hasn't made it into code base in two years [message #1199483 is a reply to message #1198140] |
Wed, 20 November 2013 16:42 |
| Eclipse User |
|
|
|
Hi Ivan,
I tested the patch you submitted in bugzilla, not the one submitted by the OP. It required two small changes to make it work, and it works properly in my app as modified. I created a patch file and will attach that to the bugzilla entry. I've pasted it below as well. My patch is relative to the code as modified by your patch, so you can see the two extra things I found necessary. It may be that my solution needs to be modified, but I think you can see from the patch the problem that I ran into.
On line 394 (after my patch applied) I added the display instance you created to the WorkbenchJob constructor. Otherwise the WorkbenchJob used a display that had no session context, and it threw an exception. A WorkbenchJob is also created in addChildren(), but this method was always called in the UI thread during my testing, while runClearPlaceholderJob() was not. Do you think anything needs to be done in addChildren()?
On line 230, I added code to make the call to IDeferredWorkbecnhAdapter#fetchDeferredChildren run on the UI thread. My implementation of IDeferredWorkbecnhAdapter is declared in plugin.xml so it has a no-argument constructor. I was therefore unable to give it a reference to a Display instance, and the only way I can get it to work in RAP then is to have the DeferredTreeContentManager call the adapter's method in the UI thread. Perhaps there's a better way to deal with that.
diff --git a/org/eclipse/ui/progress/DeferredTreeContentManager.java b/org/eclipse/ui/progress/DeferredTreeContentManager.java
index 2b89595..ed9b07e 100644
--- a/org/eclipse/ui/progress/DeferredTreeContentManager.java
+++ b/org/eclipse/ui/progress/DeferredTreeContentManager.java
@@ -226,8 +226,14 @@ public class DeferredTreeContentManager {
*
* @see org.eclipse.core.jobs.Job#run(org.eclipse.core.runtime.IProgressMonitor)
*/
- public IStatus run(IProgressMonitor monitor) {
- adapter.fetchDeferredChildren(parent, collector, monitor);
+ public IStatus run(final IProgressMonitor monitor) {
+ display.asyncExec(new Runnable() {
+
+ public void run() {
+ adapter.fetchDeferredChildren(parent, collector, monitor);
+ }
+
+ });
if (monitor.isCanceled()) {
return Status.CANCEL_STATUS;
}
@@ -385,7 +391,7 @@ public class DeferredTreeContentManager {
}
// Clear the placeholder if it is still there
- WorkbenchJob clearJob = new WorkbenchJob(
+ WorkbenchJob clearJob = new WorkbenchJob(display,
ProgressMessages.get( display ).DeferredTreeContentManager_ClearJob) {
/*
* (non-Javadoc)
|
|
|
| Re: Bug with fix contributed hasn't made it into code base in two years [message #1200500 is a reply to message #1199483] |
Thu, 21 November 2013 04:12 |
| Eclipse User |
|
|
|
Hi Mark,
see my comments below:
On 11/20/2013 11:42 PM, Mark Leone wrote:
> On line 394 (after my patch applied) I added the display instance you
> created to the WorkbenchJob constructor. Otherwise the WorkbenchJob
> used a display that had no session context, and it threw an exception.
> A WorkbenchJob is also created in addChildren(), but this method was
> always called in the UI thread during my testing, while
> runClearPlaceholderJob() was not. Do you think anything needs to be
> done in addChildren()?
Agree with this. I think that adding the display to the second use of
WorkbenchJob (in addChildren()) will not hurt. The session display and
the display provided from outside are the same if you run this coded
from the UI thread.
>
> On line 230, I added code to make the call to
> IDeferredWorkbecnhAdapter#fetchDeferredChildren run on the UI thread.
> My implementation of IDeferredWorkbecnhAdapter is declared in
> plugin.xml so it has a no-argument constructor. I was therefore unable
> to give it a reference to a Display instance, and the only way I can
> get it to work in RAP then is to have the DeferredTreeContentManager
> call the adapter's method in the UI thread. Perhaps there's a better
> way to deal with that.
Why do you need a reference to the display in your implementation. Is it
enough to execute the code with fake context like:
RWT.getUISession( display ).exec( new Runnable() {
public void run() {
adapter.fetchDeferredChildren(parent, collector, monitor);
}
} );
instead asynExec?
Best,
Ivan
--
Ivan Furnadjiev
Twitter: @EclipseRAP
Blog: http://eclipsesource.com/blogs/
Professional services for RAP and RCP?
http://eclipsesource.com/services/rap/
|
|
|
| Re: Bug with fix contributed hasn't made it into code base in two years [message #1200532 is a reply to message #1199483] |
Thu, 21 November 2013 04:33 |
| Eclipse User |
|
|
|
.... small addition... According to
IDeferredWorkbenchAdapter#fetchDeferredChildren JavaDoc:
"Called by a job run in a separate thread to fetch the children of this
adapter."
Your change to run it in UI thread violates it.
Best,
Ivan
On 11/20/2013 11:42 PM, Mark Leone wrote:
> Hi Ivan,
>
> I tested the patch you submitted in bugzilla, not the one submitted by
> the OP. It required two small changes to make it work, and it works
> properly in my app as modified. I created a patch file and will attach
> that to the bugzilla entry. I've pasted it below as well. My patch is
> relative to the code as modified by your patch, so you can see the two
> extra things I found necessary. It may be that my solution needs to be
> modified, but I think you can see from the patch the problem that I
> ran into.
>
> On line 394 (after my patch applied) I added the display instance you
> created to the WorkbenchJob constructor. Otherwise the WorkbenchJob
> used a display that had no session context, and it threw an exception.
> A WorkbenchJob is also created in addChildren(), but this method was
> always called in the UI thread during my testing, while
> runClearPlaceholderJob() was not. Do you think anything needs to be
> done in addChildren()?
>
> On line 230, I added code to make the call to
> IDeferredWorkbecnhAdapter#fetchDeferredChildren run on the UI thread.
> My implementation of IDeferredWorkbecnhAdapter is declared in
> plugin.xml so it has a no-argument constructor. I was therefore unable
> to give it a reference to a Display instance, and the only way I can
> get it to work in RAP then is to have the DeferredTreeContentManager
> call the adapter's method in the UI thread. Perhaps there's a better
> way to deal with that.
>
> diff --git a/org/eclipse/ui/progress/DeferredTreeContentManager.java
> b/org/eclipse/ui/progress/DeferredTreeContentManager.java
> index 2b89595..ed9b07e 100644
> --- a/org/eclipse/ui/progress/DeferredTreeContentManager.java
> +++ b/org/eclipse/ui/progress/DeferredTreeContentManager.java
> @@ -226,8 +226,14 @@ public class DeferredTreeContentManager {
> *
> * @see
> org.eclipse.core.jobs.Job#run(org.eclipse.core.runtime.IProgressMonitor)
> */
> - public IStatus run(IProgressMonitor monitor) {
> - adapter.fetchDeferredChildren(parent, collector, monitor);
> + public IStatus run(final IProgressMonitor
> monitor) {
> + display.asyncExec(new Runnable() {
> +
> + public void run() {
> + adapter.fetchDeferredChildren(parent, collector, monitor);
> + }
> + + });
> if (monitor.isCanceled()) {
> return Status.CANCEL_STATUS;
> }
> @@ -385,7 +391,7 @@ public class DeferredTreeContentManager {
> }
>
> // Clear the placeholder if it is still there
> - WorkbenchJob clearJob = new WorkbenchJob(
> + WorkbenchJob clearJob = new WorkbenchJob(display,
> ProgressMessages.get( display
> ).DeferredTreeContentManager_ClearJob) {
> /*
> * (non-Javadoc)
>
--
Ivan Furnadjiev
Twitter: @EclipseRAP
Blog: http://eclipsesource.com/blogs/
Professional services for RAP and RCP?
http://eclipsesource.com/services/rap/
|
|
|
| Re: Bug with fix contributed hasn't made it into code base in two years [message #1201090 is a reply to message #1200532] |
Thu, 21 November 2013 10:19 |
| Eclipse User |
|
|
|
Thanks for your help, Ivan. I'll have to find a way to get a Display reference in my IDeferredWorkbenchAdapter implementation. I'm currently on RAP 1.5, so I can't use
RWT.getUISession(display) RWT.getSessionStore().getAttribute(someKey)
If not, I'll have to find a way to pass the Display into the cosntructor, and I'll probably also have to make the adapter a session singleton.
As far as the fix needed to the RAP code base, it looks like just adding the Display reference to the WorkbenchJob constructor in addChildren() and runClearPlaceholderJob() is all that's needed. Please note that line 382 of DeferredTreeContentManager in your proposed patch seems to be unnecessary. A method-scoped reference to treeViewer's display is created in that line, but there is an instance-scoped reference assigned in the constructor.
Are you planning to submit the patch as amended here, and if so do you have any idea what version or milestone it will be appear in? I pasted a patch below that adds the two changes we agreed on here to your original proposed patch, this time relative to the original code base. However, please note that this is based on RAP 1.5, so a different patch may be needed to apply to a RAP 2.x baseline.
diff --git a/org/eclipse/ui/internal/progress/ProgressMessages.java b/org/eclipse/ui/internal/progress/ProgressMessages.java
index feb1607..d390163 100644
--- a/org/eclipse/ui/internal/progress/ProgressMessages.java
+++ b/org/eclipse/ui/internal/progress/ProgressMessages.java
@@ -11,6 +11,8 @@
package org.eclipse.ui.internal.progress;
import org.eclipse.rwt.RWT;
+import org.eclipse.rwt.lifecycle.UICallBack;
+import org.eclipse.swt.widgets.Display;
// RAP [fappel]: NLS needs to be session/request aware
//public class ProgressMessages extends NLS{
@@ -138,4 +140,14 @@ public class ProgressMessages {
Object result = RWT.NLS.getISO8859_1Encoded( BUNDLE_NAME, clazz );
return ( ProgressMessages )result;
}
+
+ public static ProgressMessages get( Display display ) {
+ final ProgressMessages[] result = { null };
+ UICallBack.runNonUIThreadWithFakeContext( display, new Runnable() {
+ public void run() {
+ result[ 0 ] = get();
+ }
+ } );
+ return result[ 0 ];
+ }
}
diff --git a/org/eclipse/ui/progress/DeferredTreeContentManager.java b/org/eclipse/ui/progress/DeferredTreeContentManager.java
index 9cb9f57..8b93168 100644
--- a/org/eclipse/ui/progress/DeferredTreeContentManager.java
+++ b/org/eclipse/ui/progress/DeferredTreeContentManager.java
@@ -41,6 +41,9 @@ import org.eclipse.ui.model.IWorkbenchAdapter;
*/
public class DeferredTreeContentManager {
+// RAP [if] display used to access NLS messages
+ private final Display display;
+
AbstractTreeViewer treeViewer;
IWorkbenchSiteProgressService progressService;
@@ -129,6 +132,7 @@ public class DeferredTreeContentManager {
*/
public DeferredTreeContentManager(AbstractTreeViewer viewer) {
treeViewer = viewer;
+ display = treeViewer.getControl().getDisplay();
}
/**
@@ -145,7 +149,7 @@ public class DeferredTreeContentManager {
*/
public boolean mayHaveChildren(Object element) {
Assert.isNotNull(element,
- ProgressMessages.get().DeferredTreeContentManager_NotDeferred);
+ ProgressMessages.get( display ).DeferredTreeContentManager_NotDeferred);
IDeferredWorkbenchAdapter adapter = getAdapter(element);
return adapter != null && adapter.isContainer();
}
@@ -316,7 +320,7 @@ public class DeferredTreeContentManager {
protected String getFetchJobName(Object parent,
IDeferredWorkbenchAdapter adapter) {
return NLS.bind(
- ProgressMessages.get().DeferredTreeContentManager_FetchingName,
+ ProgressMessages.get( display ).DeferredTreeContentManager_FetchingName,
adapter.getLabel(parent));
}
@@ -329,8 +333,8 @@ public class DeferredTreeContentManager {
*/
protected void addChildren(final Object parent, final Object[] children,
IProgressMonitor monitor) {
- WorkbenchJob updateJob = new WorkbenchJob(
- ProgressMessages.get().DeferredTreeContentManager_AddingChildren) {
+ WorkbenchJob updateJob = new WorkbenchJob(display,
+ ProgressMessages.get( display ).DeferredTreeContentManager_AddingChildren) {
/*
* (non-Javadoc)
*
@@ -375,14 +379,13 @@ public class DeferredTreeContentManager {
// if (placeholder.isRemoved() || !PlatformUI.isWorkbenchRunning()) {
// return;
// }
- Display display = treeViewer.getControl().getDisplay();
if (placeholder.isRemoved() || !ProgressUtil.isWorkbenchRunning( display )) {
return;
}
// Clear the placeholder if it is still there
- WorkbenchJob clearJob = new WorkbenchJob(
- ProgressMessages.get().DeferredTreeContentManager_ClearJob) {
+ WorkbenchJob clearJob = new WorkbenchJob(display,
+ ProgressMessages.get( display ).DeferredTreeContentManager_ClearJob) {
/*
* (non-Javadoc)
*
|
|
|
| Re: Bug with fix contributed hasn't made it into code base in two years [message #1201225 is a reply to message #1201090] |
Thu, 21 November 2013 11:35 |
| Eclipse User |
|
|
|
Hi Mark,
I fixed it in master with commit [1]. Fetching children stays the same -
executed in background thread. You still have to find a way how to
obtain a display reference in your IDeferredWorkbenchAdapter implementation.
[1]
http://git.eclipse.org/c/rap/org.eclipse.rap.git/commit/?id=f8a286b305cd65959c562767120fabd017255783
Best,
Ivan
On 11/21/2013 5:19 PM, Mark Leone wrote:
> Thanks for your help, Ivan. I'll have to find a way to get a Display
> reference in my IDeferredWorkbenchAdapter implementation. I'm
> currently on RAP 1.5, so I can't use
> RWT.getUISession(display) to get a reference to the Display. Is the
> Display stored in a session attribute so that I could retrieve it as
> follows in RAP 1.5? RWT.getSessionStore().getAttribute(someKey)
> If not, I'll have to find a way to pass the Display into the
> cosntructor, and I'll probably also have to make the adapter a session
> singleton.
>
> As far as the fix needed to the RAP code base, it looks like just
> adding the Display reference to the WorkbenchJob constructor in
> addChildren() and runClearPlaceholderJob() is all that's needed.
> Please note that line 382 of DeferredTreeContentManager in your
> proposed patch seems to be unnecessary. A method-scoped reference to
> treeViewer's display is created in that line, but there is an
> instance-scoped reference assigned in the constructor.
>
> Are you planning to submit the patch as amended here, and if so do you
> have any idea what version or milestone it will be appear in? I pasted
> a patch below that adds the two changes we agreed on here to your
> original proposed patch, this time relative to the original code base.
> However, please note that this is based on RAP 1.5, so a different
> patch may be needed to apply to a RAP 2.x baseline.
>
> diff --git a/org/eclipse/ui/internal/progress/ProgressMessages.java
> b/org/eclipse/ui/internal/progress/ProgressMessages.java
> index feb1607..d390163 100644
> --- a/org/eclipse/ui/internal/progress/ProgressMessages.java
> +++ b/org/eclipse/ui/internal/progress/ProgressMessages.java
> @@ -11,6 +11,8 @@
> package org.eclipse.ui.internal.progress;
>
> import org.eclipse.rwt.RWT;
> +import org.eclipse.rwt.lifecycle.UICallBack;
> +import org.eclipse.swt.widgets.Display;
>
> // RAP [fappel]: NLS needs to be session/request aware
> //public class ProgressMessages extends NLS{
> @@ -138,4 +140,14 @@ public class ProgressMessages {
> Object result = RWT.NLS.getISO8859_1Encoded( BUNDLE_NAME, clazz );
> return ( ProgressMessages )result;
> }
> +
> + public static ProgressMessages get( Display display ) {
> + final ProgressMessages[] result = { null };
> + UICallBack.runNonUIThreadWithFakeContext( display, new
> Runnable() {
> + public void run() {
> + result[ 0 ] = get();
> + }
> + } );
> + return result[ 0 ];
> + }
> }
> diff --git a/org/eclipse/ui/progress/DeferredTreeContentManager.java
> b/org/eclipse/ui/progress/DeferredTreeContentManager.java
> index 9cb9f57..8b93168 100644
> --- a/org/eclipse/ui/progress/DeferredTreeContentManager.java
> +++ b/org/eclipse/ui/progress/DeferredTreeContentManager.java
> @@ -41,6 +41,9 @@ import org.eclipse.ui.model.IWorkbenchAdapter;
> */
> public class DeferredTreeContentManager {
>
> +// RAP [if] display used to access NLS messages + private final
> Display display;
> +
> AbstractTreeViewer treeViewer;
>
> IWorkbenchSiteProgressService progressService;
> @@ -129,6 +132,7 @@ public class DeferredTreeContentManager {
> */
> public DeferredTreeContentManager(AbstractTreeViewer viewer) {
> treeViewer = viewer;
> + display = treeViewer.getControl().getDisplay();
> }
>
> /**
> @@ -145,7 +149,7 @@ public class DeferredTreeContentManager {
> */
> public boolean mayHaveChildren(Object element) {
> Assert.isNotNull(element,
> - ProgressMessages.get().DeferredTreeContentManager_NotDeferred);
> + ProgressMessages.get( display
> ).DeferredTreeContentManager_NotDeferred);
> IDeferredWorkbenchAdapter adapter = getAdapter(element);
> return adapter != null && adapter.isContainer();
> }
> @@ -316,7 +320,7 @@ public class DeferredTreeContentManager {
> protected String getFetchJobName(Object parent,
> IDeferredWorkbenchAdapter adapter) {
> return NLS.bind(
> - ProgressMessages.get().DeferredTreeContentManager_FetchingName,
> + ProgressMessages.get( display
> ).DeferredTreeContentManager_FetchingName,
> adapter.getLabel(parent));
> }
>
> @@ -329,8 +333,8 @@ public class DeferredTreeContentManager {
> */
> protected void addChildren(final Object parent, final Object[]
> children,
> IProgressMonitor monitor) {
> - WorkbenchJob updateJob = new WorkbenchJob(
> - ProgressMessages.get().DeferredTreeContentManager_AddingChildren) {
> + WorkbenchJob updateJob = new WorkbenchJob(display,
> + ProgressMessages.get( display
> ).DeferredTreeContentManager_AddingChildren) {
> /*
> * (non-Javadoc)
> * @@ -375,14 +379,13 @@ public class
> DeferredTreeContentManager {
> // if (placeholder.isRemoved() ||
> !PlatformUI.isWorkbenchRunning()) {
> // return;
> // }
> - Display display = treeViewer.getControl().getDisplay();
> if (placeholder.isRemoved() ||
> !ProgressUtil.isWorkbenchRunning( display )) {
> return;
> }
>
> // Clear the placeholder if it is still there
> - WorkbenchJob clearJob = new WorkbenchJob(
> - ProgressMessages.get().DeferredTreeContentManager_ClearJob) {
> + WorkbenchJob clearJob = new WorkbenchJob(display,
> + ProgressMessages.get( display
> ).DeferredTreeContentManager_ClearJob) {
> /*
> * (non-Javadoc)
> *
--
Ivan Furnadjiev
Twitter: @EclipseRAP
Blog: http://eclipsesource.com/blogs/
Professional services for RAP and RCP?
http://eclipsesource.com/services/rap/
|
|
|
Goto Forum:
Current Time: Tue Jul 29 12:59:55 EDT 2025
Powered by FUDForum. Page generated in 0.07578 seconds |
Home