Bugzilla – Full Text Bug Listing
| Summary: | Cleanup of DefaultGalleryItemRenderer | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Peter Centgraf <peter> | ||||||
| Component: | Nebula | Assignee: | Nicolas Richeton <nicolas.richeton> | ||||||
| Status: | CLOSED INVALID | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P1 | CC: | olaf.klischat, remy.suen, rkandepu | ||||||
| Version: | unspecified | Keywords: | plan | ||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Peter Centgraf
Created attachment 89087 [details]
Gallery Renderer cleanup
I get intermittent "SWTException: Invalid thread access" exceptions in the callstack below the Display#getSystemColor() call in the DefaultGalleryItemRenderer() constructor. Apparently, Display.getDefault() sometimes returns a display that doesn't belong to the calling thread. I don't fully understand it, but I'd say that all the Display.getDefault() calls should be replaced with org.eclipse.ui.PlatformUI.getWorkbench().getDisplay() calls. See also the Javadoc for IWorkbench#getDisplay(), which clearly states that you should use that method instead of Display.getDefault(). (In reply to comment #2) > I don't > fully understand it, but I'd say that all the Display.getDefault() calls should > be replaced with org.eclipse.ui.PlatformUI.getWorkbench().getDisplay() calls. > See also the Javadoc for IWorkbench#getDisplay(), which clearly states that you > should use that method instead of Display.getDefault(). I'm not sure how this is possible since the Nebula Gallery plug-in doesn't have any dependencies on the RCP components of Eclipse. I'd personally propose that the default constructor of the DefaultGalleryItemRenderer class be deprecated and replaced with one that takes a Display as an argument. This should prevent the race condition that Olaf is having. Olaf : it seems that you are calling DefaultGalleryItemRenderer from a separate thread. Usualy, you should call it from the main thread or use Display.syncExec(). We should add some code to fail directly if called from a thread (like checkWidget() ) (In reply to comment #5) > Olaf : it seems that you are calling DefaultGalleryItemRenderer from a separate > thread. I'm not. Sorry if I didn't make this clear enough -- I basically do: Gallery gallery = new Gallery(parent, SWT.V_SCROLL | SWT.VIRTUAL); gallery.setItemRenderer(new DefaultGalleryItemRenderer()); in the createPartControl() method of a View in an RCP application -- this method is run in the GUI thread. The exception happens in the DefaultGalleryItemRenderer() constructor. Olaf : Ok, but there should not be any problem with Display.getDefault() if this method is called by the UI thread. Which platform are you using (Win32, GTK )? Are you opening this view from your code or by using an extension point ? I suspect that the whole createPart code is running in a separate thread. (In reply to comment #7) > Olaf : Ok, but there should not be any problem with Display.getDefault() if > this method is called by the UI thread. OK, I checked some more -- there may be a race condition in my application. There is a Display.getDefault().syncExec(...) here in a method that's called regularly by a spring quartz trigger -- i.e. an internal spring thread. Sometimes it may happen that this is called before the RCP app's main class Application#run() method got a chance to PlatformUI.createDisplay() -- and thus, that internal spring thread would become the GUI thread. I'll investigate this further. Sorry if I caused confusion here :P That still raises interesting questions for me though -- is it technically feasible to have more than one Display in an RCP app, e.g. in multi-monitor configurations? And if so, shouldn't using Display.getDefault() be discouraged for widget writers (and replaced by parent.getDisplay(), or, if that's impossible, taking a Display as a constructor parameter)? Yes, it is possible to have multiple Displays in the same JVM. Widget.getDisplay() should be used instead of Display.getCurrent() or Display.getDefault(). Any updates on this to remove calling the static methods of Display? I think this bug is the cause of bug 184413. Remy, Well, I may be wrong, but it should be safe to call this method from the UI thread. Since this is a breaking API change, I want to be sure that this bug is really caused by the Gallery widget. Note : my own application also uses perspective save/restore with this widget and I never got this issue on restore. the change is really easy to do, so I would like that someone having this problem to try the proposed fix and ensure it really solves the problem. OR : attach a snippet/simple product that shows the issue so I can check by myself. Unfortunately, attachment 89087 [details] no longer applices cleanly to HEAD. Nicolas, regarding API breakage, would you adding and deprecating constructors per my suggested comment 4? Made the Display fix without changing API. Default colors are now defined in setGallery(), not in constructor, by using gallery.getDisplay(). Committed in CVS > 20090416 Back to Peter's patch, it seems to set a background color for every item which can lead to unnecessary drawing operations and redraw slowdown. I need to review it in details. Just to be sure : is this patch only code clean up or does this includes a fix for something required in another bug ? Created attachment 132117 [details]
mylyn/context/zip
The main problem that the patch was intended to fix is stale cached Color values. SWT defines setBackground() and setForeground() methods on both the main Control and its Items, and new values should take effect immediately. The value set on the Control acts as a default, in case no value is set on a particular Item. The value set in the Display acts as a global default, in case no value is set in the Control, either. The code as it was originally written cached the Color values from the Gallery in the constructor and never checked for changes, so calling Gallery.setBackground() had no effect. In my opinion, we should handle this like SWT does, and repeat the lookup on Item, Gallery, and Display for each Item to ensure that the colors are up to date when painting. Ok, changing priority to P1 since this is a difference with SWT API. I'll take a look at it ASAP. I've made the change to use Parent's color as in Tree. Item background is only drawn if different from gallery background color. Note : There is a bug on SWT carbon 3.4.1 (at least) causing item background to be always drawn : Control.getColor returns a new object every times, so gallery.getBackground() != gallery.getBackground() Ok, this is not a bug. There is a probably a performance issue here, because every time we paint an item, we create a least 2 Color objects. 100 items visible + animations = 2500 Colors per second. Maybe Peter's patch is better after all :) *** Bug 184413 has been marked as a duplicate of this bug. *** This bug does not have a target milestone assigned and is automatically closed as part of the 2.3.0 release cleanup. It could be that this bug is accidentally closed for which we apologize. If this bug is still relevant, please re-open and set a target milestone. |