Community
Participate
Working Groups
In 3.2, the lightweight system paints when it receives a native paint event. However, it's possible that this could happen when a figure is invalid -- after it's been invalidated but before the update manager performs validation. This can cause several problems since figures usually don't expect to be painted when invalid. Not marking this bug as critical because whatever problems you run into would affect only that paint invocation. And since an update has already been scheduled, a follow-up layout and paint will occur soon after, which means the user might not notice any problem visually. This could be fixed by the update manager ignoring the native paint request if an update has already been scheduled, or by forcing validation at that point.
Needed in maintenance release.
I am wondering is you have a use case to duplicate this issue. (In reply to comment #0) > [...] Not marking this bug as critical [...] This is marked as major so we (I) need to understand the issue.
You might try something like: 1) Drop a sticky note in the logic example 2) Increase the amount of text 3) Open the context menu on top of the note 4) Invoke UNDO from the context menu. On GTK, when the context menu hides, it should cause a paint event (I think windows actually caches the pixels behind the menu). The sticky note will try to paint using its STALE textfragments that correspond to when the TextFlow had a larger string value. When those fragments paint, you should get an IndexOutOfBounds exception when trying to take the substring beyong the end of the text. I would say this is borderline critical. It could be a maintenance issue since it causes random exceptions to appear in the log.
Created attachment 54360 [details] proposed patch Attached patch validates invalid figures in the DeferredUpdateManager#paint(GC) method. Randy or Pratik, could you please review this patch when you get a free minute? Thanks in advance.
The requirements as I see them are: 1) We can not paint at all, unless we call performValidate() first. 2) performValidate() can not be reentered, so we need a way of knowing if paint() is happening during performValidate(). Checking the "updating" flag is not quite the same.
Why don't we call performUpdate(Rectangle) in the paint method? Looks like in the case of paint we redraw the whole visible region of the diagram... performUpdate(Rectangle) looks like a good candidate for this purpose if the proper rectangle provided. This would also take care of validation...
Created attachment 54567 [details] proposed patch First of all, never mind the previous comment. With this patch I'm suggesting to call a synchronized doPaint(Graphics) method. 1) doPaint method validates figures right before before painting, hence 1st requirement should be satisfied 2) doPaint is synchronized, so performUpdate that calls performValidation would either validate figures before or after doPaint called from paint. P.S. Alternatively I could make performValidation synchronized.
The concern is not with multiple threads, which isn't supported. The possible problem is with reentry. So, paint actually get's called while performValidation() is still on the stack. It seems like a very remote possibility, but someone could be resizing the canvas or moving children controls on the canvas and calling update() for some reason during the actual layout. I'm not really sure why anyone would do this, but Paint is a native event and who knows what will trigger it.
Created attachment 54963 [details] proposed patch Here is the patch that should be dealing with perofrmValidation() method reentering. Idea: count the number of reentries in performValidation() method. Clear the list of invalid figures only if this number becomes 0. Don't remove or clear entries from the list of invalid figures if number of reentries is not 0. There is no big deal if we call validate() on a valid figure, since validate() will return right away for a valid figure. To me this is a harmless fix, which should take care of reentries. I understand that this fix simply avoids having an NPE in performValidation(), but for a more interesting fix I really need a tip and a way to cause a native paint event to occur during the layout of a figure, i.e. reproduce that special use case. Could you please take a look at the patch and provide some ideas for a more elegant fix if the patch is not acceptable? Thanks in advance.
Any feedback on the last patch?
Assume Figure B is a child of Figure A and not a validation root (and thus never added to the list of invalid figures). While A is being validated, paint() is invoked because something funky happens while A is laying out. With the latest patch, the rest of the invalid figures in the list will be validated when performValidation() is re-entered, but B will still be invalid when paint happens (because A hasn't had a chance to validate its children). How about scheduling an async paint if performValidation is on the stack when paint is invoked? Can we verify that the scenarios that Randy mentions in comment 8 are possible? Maybe the SWT team can help with that.
First of all, thank you for the feedback. (In reply to comment #11) > Assume Figure B is a child of Figure A and not a validation root (and thus > never added to the list of invalid figures). While A is being validated, > paint() is invoked because something funky happens while A is laying out. With > the latest patch, the rest of the invalid figures in the list will be validated > when performValidation() is re-entered, but B will still be invalid when paint > happens (because A hasn't had a chance to validate its children). Yes... I agree that patch wouldn't work... B won't get validated. > How about scheduling an async paint if performValidation is on the stack when > paint is invoked? I think I've tried something like that. The problem was that GC is disposed before the async message is executed... (this is if I did everything correctly) However, I have an alternative idea. How about simply adding a dirty region in this case. If figures are being validated or update is still queued we haven't started processing the dirty regions yet. So since in the paint(GC) method we have a rectangle that needs to be painted, perhaps we could add this rectangle to dirty regions instead of painting in case of validation in progress or even if update is still queued? Does it sound reasonable in theory at least? (I didn't get a chance to verify it with the use case from comment #3 and comment #8 - will try that tomorrow) > Can we verify that the scenarios that Randy mentions in comment 8 are possible? > Maybe the SWT team can help with that. To be honest, I didn't verify the scenario from comment #8 yet. I was mislead by the scenario in bug 150310. I thought performValidation is reentrant not as a result of native paint event but rather a call to performUpdate() during validation. I'll get on reproducing the scenarion from comment #8 first thing tomorrow morning.
I am not convinced we can solve the problem of calling performUpdate recursively. But, paint(GC) needs to call performUpdate() to fix the current issue. So, if paint(GC) is called during an invocation of performUpdate(), we could just record the dirty region and not paint until performUpdate finished calculating all of the dirty regions and requests another redraw. The only problem I see with this approach is that Canvas#scroll(...) assumes that paint does something, and it will proceed to copy stale pixels. I think this is fine, and we would have to translate the cached dirty rectangle if it every became an issue.
Created attachment 55517 [details] patch Proposed patch for adding a dirty region in case of native paint event occuring during validation. Verified with the use case from comment #3. Couldn't verify with use case from comment #8. Tried to send a WM_PAINT message to the FigureCanvas control through the following hack: org.eclipse.swt.internal.win32.OS.SendMessage (control.handle, OS.WM_PAINT, 0, 0); but that didn't work well, paint(GC) is not invoked because Composite#WM_PAINT() method need non-0 width and height... currently can't track down the place where dimensions of PAINTSTRUCT object become non-0. Do we still need to verify the scenario from comment #8?
Created attachment 55530 [details] patch I managed to verify the scenario from comment #8 with this patch. It works for this scenario (previous wasn't). Simulated a native paint event: by adding the following hack in performValidation (after validation of the first figure in the list): control.redraw(0,0,1000,1000,true); control.update(); This triggered a native paint event. This included the case of sending a paint event to the control when it validates figures for the previous paint event. Could you please take a look at the patch and provide some feedback? Thanks in advance.
Created attachment 55531 [details] patch I managed to verify the scenario from comment #8 with this patch. It works for this scenario (previous wasn't). Simulated a native paint event: by adding the following hack in performValidation (after validation of the first figure in the list): control.redraw(0,0,1000,1000,true); control.update(); This triggered a native paint event. This included the case of sending a paint event to the control when it validates figures for the previous paint event. Could you please take a look at the patch and provide some feedback? Thanks in advance.
Sorry about the double-posting. Two last patches as comments 15 and 16 are identical.
Created attachment 55559 [details] cleaned up patch This is the cleaned up patch for a code review.
Created attachment 55871 [details] patch forcode review Corrected per Randy's commnets
Created attachment 56843 [details] patch suitable for 3.2.2 Patch suitable for 3.2.2 with copyrights and manifest file changed apropriately
The patch has been committed to HEAD and R32_Maintenance. This will do into 3.2.2 ( I will respin the 3.2.2 final build ).
Comment on attachment 55871 [details] patch forcode review According to Anthony's comment, patch 56843 has been committed, thus this was not relevant.
Comment on attachment 55871 [details] patch forcode review It seems only patch 56843 was of relevance, thus marking this as obsolete.
