Community
Participate
Working Groups
Currently, SWT layouts work like this: - Some event occurs in the app - Method X updates a set of widgets - Method X notifies the wigdet's layout of the change (by calling changed() and layout()) This prevents writing any black box object that knows how to update a set of wigdets without depending on where those wigdets are used. This is creating a severe maintnance problem within the Eclipse UI. If code modifies a widget, it needs to: 1. Know about every other piece of code that calls that widget's computeSize method. 2. Know which object to trigger a layout on. 3. Know when to trigger a layout. (Or when to defer and undefer layouts) Issue 1 creates very large (and complicated) update methods. Issue 2 means that the spaghetti code is always specific to a particular layout and cannot be reused. Even minor layout changes often require severe refactoring. Issue 3 is impossible to get right. In our world, everything is triggered by listeners. Even though the layout only needs to happen after the last listener has done its thing and control returns to the event loop, there is no way to know that "this will be the last change to this widget before control returns to the event loop". We work around this by heuristically batching updates together, and making individual update methods responsible for as many widgets as possible. Our current heuristics work well, but the code is extremely complicated and brittle. This PR requests the following: a. Provide a way to flag a widget as "changed", such that any dependant layout will be notified and triggered when control returns to the event loop. (The client code MUST NOT be responsible for calling layout(...) or setLayoutDeferred) b. Provide a "preferred size changed" listener, so that a widget can have its position managed by something other than its immediate parent (aka proxy controls). c. Provide a way for layouts to indicate whether a change to a particular child will affect their own preferred size. This will be needed for SWT to correctly compute the effects of a "changed" event. Not sure whether to flag as "blocker" or "enhancement". It is really an enhancement, but it is blocking further work on the Eclipse layouts. Flagging as major as a compromise. :-)
Created attachment 24773 [details] Suggested algorithm (implemented through JFace) This patch suggests a new layout protocol. It is currently implemented as a bunch of static methods and coding conventions in JFace, but the same pattern would work if the static methods were moved to the Control class and the coding conventions happened automatically. The patch also addresses an independant issue involving computeSize (in other words, feel free to ignore the new compute*Size methods).
Created attachment 24775 [details] Standalone SWT applications that demonstrate the problem This project contains the same SWT application coded in three ways: 1. Using the pattern currently used by Eclipse: a big update method that does everything. Good (but non-optimal) performance, complicated code. 2. Using a pattern suggested by Veronika: call Shell.layout(changedControl) every time any control changes. Very poor performance, well encapsulated code. 3. Using the proposed pattern. Controls are dirtied whenever they change, and any necessary layouts happen when control returns to the event loop. Optimal performance, well encapsulated code. The app implements a simple model/view/controller pattern, where there are two buttons to add and remove elements from a set and a bunch of "views" that display the size of the set as asterisks. A second shell monitors the number of times the last label is moved or resized.
CC'ing all the people I promised to keep informed about this issue. :-)
I still haven't tested this algorithm with the ProxyControl pattern used commonly in the workbench. In order to fully clean up the mess, the code that changes the code being proxied should not need to know that the control is being proxied, clipping should be computed correctly (and should update if one of the ancestors contributing to the clipping region changes), and the target should not move more than once per pass through the event loop.
Proxy'd widgets aside, the mechanism I would propose for minimizing the number of layouts is shown in the following example. The controlChanged method could be a utility function in JFace. In the example, any button you click on will update two labels, one which is a peer of the button and one which is a peer of the button's parent. To count the number of layouts performed, I hacked FillLayout and GridLayout to print out each time layout was called. I also hacked Button and Label to print out each time computeSize was called on the widgets. public class DeferredLayoutExample { static int INDEX; public static void main (String [] args) { Display display = new Display (); Shell shell = new Shell (display); shell.setLayout(new GridLayout()); doit(shell); shell.pack(); shell.open(); while (!shell.isDisposed ()) { if (!display.readAndDispatch ()) display.sleep (); } display.dispose (); } static void doit(Composite parent) { if (INDEX == 10) return; Composite c = new Composite(parent, SWT.BORDER); c.setLayout(new GridLayout(3, false)); Button b = new Button(c, SWT.PUSH); b.setLayoutData(new GridData(SWT.LEFT, SWT.TOP, false, false)); b.setText("Button "+INDEX++); controlChanged(b); // wrapping label final Label l1 = new Label(c, SWT.WRAP); GridData data = new GridData(SWT.LEFT, SWT.TOP, false, false); data.widthHint = 100; l1.setLayoutData(data); final Label l2 = new Label(parent, SWT.WRAP); data = new GridData(SWT.LEFT, SWT.TOP, true, false, 2, 1); data.widthHint = 100; l2.setLayoutData(data); b.addListener(SWT.Selection, new Listener() { public void handleEvent(Event e) { l1.setText(l1.getText()+"* "); controlChanged(l1); l2.setText(l2.getText()+"x "); controlChanged(l2); } }); doit(c); } static void controlChanged(Control c) { final Shell shell = c.getShell(); shell.setLayoutDeferred(true); shell.layout(new Control[] {c}); shell.getDisplay().asyncExec(new Runnable() { public void run() { shell.setLayoutDeferred(false); } }); } }
There are several problems with comment 5: 1. The layouts do not occur in the same pass through the event loop. Although the total number of layouts is the same, there could be considerable flicker because SWT may repaint the widgets between each *syncExec. In order to be flicker-free, the layouts must occur in the same pass through the event loop as they were dirtied. This is why the change should go in SWT and not JFace. The best JFace can do is rely on *syncExec (creating flicker) or use a specialized event loop (like my example) which won't work when a non-conforming modal dialog is open. SWT can add code to readAndDispatch, which ensures that the layouts always work and never flicker. 2. It will not work with the proxy pattern, which is a firm requirement for the workbench (a solution that does not address this cannot be used). 3. Layouts do not have the opportunity opt out of dirtying their parent when the child changes. In both of our examples this makes no difference, but in the Eclipse workbench this makes a huge difference. 4. The requirement to call controlChanged after any modification to any control is cumbersome. Putting this method in SWT means that commonly-used setters could be updated to call this themselves rather than push the responsibility onto their callers. (Any method that auto-flags the object as changed could be JavaDoc'd as such so that callers know when they need to flag the object themselves) The patch I attached was mostly flicker-free (it flickered at most once when a non-conforming event loop was running) and did not suffer from issues 2 and 3. I am not looking for assistance with a JFace-based protocol (I have one): I am looking for a more elegant SWT-based solution.
See bug 46112, bug 104234, and bug 103863. These are the three biggest issues that are complicating the workbench layouts.
GEF has solved these problems, if you replace "Control" with "Figure". We always layout one time and only layout the minimum number of figures required, regardless of how many changes you make. Are you suggesting that SWT change such that calling.. Button#setText(String) ...walks up the parent composite chain, marking the appropriate composites as needing to be layed out, and they get layed out later via async? We call this process revalidate(). It works great for us but its a major change in SWT. I think the request for proxies should be considered step 2. You may not need proxies if the API gives you the context of the layout request, allowing you to multiplex a bunch of SWT layouts in some placehold layout on the workbench page's composite. What became of all the new layout API added in 3.1?
> Are you suggesting that SWT change such that calling.. > Button#setText(String) > ...walks up the parent composite chain, marking the appropriate composites > as needing to be layed out, and they get layed out later via async? I'd like to see a standard protocol for marking the composites for layout, that would potentially permit setText to do as you describe in the future. For now, manual marking is enough. One difference: if possible, the layout should happen later in the same pass through the event loop rather than in a later async. Doing it in an async means that the user may first see the button with new text + old position before seeing it with new text + new position. > its a major change in SWT. ...but not a breaking one. > I think the request for proxies should be considered step 2. Step 1 must not rule out step 2. The layout protocol needs to be spec'd in a manner that allows us to continue to implement the current layouts. Right now, the API on ViewForm and CTabFolder requires us to use proxies (they only report the bounds of certain regions by setting the bounds of a child control - if we need to know those bounds, we need to supply a proxy). Unless this changes, we can't consider a solution that rules out the use of proxies. The reason we need this change is precisely to simplify the code for managing those proxies. > You may not need proxies if the API gives you the context of the > layout request, allowing you to multiplex a bunch of SWT layouts in > some placehold layout on the workbench page's composite. I'm not sure I follow that. :-( > What became of all the new layout API added in 3.1? The new API makes it possible for a layout to flush cached information for a single child without recomputing the entire layout. This was a necessary step forward, but not a complete solution.
> manual marking is enough. One difference: if possible, the layout should happen > later in the same pass through the event loop rather than in a later async. > Doing it in an async means that the user may first see the button with new text > + old position before seeing it with new text + new position. You could hook painting and do a pre-check to see if any layouts are pending first. This is what we have to do in GEF. But, we'd like to do it the way you suggest. Something like a Display#afterCurrentEventExec(Runnable). One problem we have with asyncExec is that its priority is not high enough and a stream of UI events can preempt it indefinitely. Do events in the OS queue have priorities?
> Display#afterCurrentEventExec(Runnable) YES! That would be superb.
Veronika suggested that if setParent() worked on all platforms, we would not need the proxy pattern. I suspect that (with considerable refactoring) this is true. However, as long as Eclipse supports any platform (Motif) which doesn't support reparenting, we will need the proxies.
Moving back to inbox (since veronika no longer owns this code AFAIK).
Most of the criticism I've heard about this pattern regards the following: > b. Provide a "preferred size changed" listener, so that a widget can have > its position managed by something other than its immediate parent (aka > proxy controls). ...since its possible for the preferred size of a widget to change due to events beyond SWT's control and therefore no way to get an event every time. If, however, we called it a "layout requested" listener, this would work just as well without suggesting it will be called on every preferred size change.
Veronika still owns the layout bugs (for now), even if she is not working on them. That way they all stay in the same place. Given the amount of work we are doing for 3.3, I can't see us looking into this. I'm really sorry about this.
cc'ing boris. He was working on a databinding utility that would automatically update toolbars. However, without a solution to this bug, there would be no way to correctly update the layout containing the toolbar after the update.
In common cases, isn't it sufficent to schedule a call to layout(true) on the composite common to the set of widgets binded to a given model? Do things have to be updated at all times in quasi real time? I guess here we're precisely talking about supporting the difficult cases. I'll hide :-)
(In reply to comment #17) > In common cases, isn't it sufficent to schedule a call to layout(true) on the I think the answer is yes, but the part missing is the schedule support. You can do an asyncExec, but things could paint in their intermediate states before the layout occurs (refer to setText() scenario). In a simple closed system you might know what the last change is and you could do the layout then.
> In common cases, isn't it sufficent to schedule a call to layout(true) > on the composite common to the set of widgets binded to a given model? Well, if you knew what composite was common, and you knew when all the changes to those widgets were finished, then a call to layout(...) would do the trick. The specific problem we're looking at here are the cases where you don't know which widget is common or when to call layout(...) on it. > Do things have to be updated at all times in quasi real time? They just need to be updated before the next repaint, and only once. The cases we're talking about here are things like the workbench layout, or the Sections in the Forms editors, where a change to a widget modifies its enclosing layout.
Created attachment 70900 [details] SWT-only snippet New snippet without any JFace. This is for looking at the "layout only once" issue.
This is still a problem. Steve, would it improve the chances of getting this fixed if I were to offer a patch?
Created attachment 104919 [details] Improved SWT-only example that demonstrates the extra repaints I've attached an improved version of Boris' code, that does a better job demonstrating the flicker in case two. Case 1 needs no modification. I can see the flicker on GTK, and you can see the redundant moves and resizes being written to the console. In case 2, the flicker arises from the multiple repaints, so I've attached a paint listener that shows the widget repainting twice.
Created attachment 104929 [details] Tests a layout with an excessive number of widgets I've attached a test that uses veronica's approach in a layout with an excessive number of widgets, most of which are not affected by the changes being made to the layout. I suspected that this would prove that the depth-first-search being done in Composite.updateLayout would end up causing a performance problem... however what it actually proved is that you run out of widget handles long before you encounter a serious performance problem here. I'm attaching my test anyway. This basically makes comment 6, issue 3 redundant. If we can fix the flicker and make it work with the proxy pattern, Veronica's approach would be acceptable.
We have added new layout API that should address this problem. See: Composite.layout(Control[] changed, int flags) SWT.ALL SWT.CHANGED SWT.DEFER Please try the new API out. If the DEFER flag is specified the layout will be deferred until just before the next event is handled.
Hi Silenio, Just curious. How is the new API different from the following? Control[] changed = new Control[]{child1, child2}; someComposite.setLayoutDeferred(true); someComposite.layout(changed, SWT.ALL); someComposite.getDisplay().asyncExec(new Runnable() { public void run() { someComposite.setLayoutDeferred(false); } }); I see that the SWT.DEFER flag causes the deferred layouts to take place before the event loop is next run (as opposed to when the event loop is next idle which is what asyncExec() does). Is this timing difference required to stop the flickering that Stefan mentions? Cheers, Veronika
Hi Vick, good to know you are still watching :-) I think you answered your own question. When SWT.DEFER is used, the layouts are done before the event loop has a chance to process any event. With the asyncExec() approach, paint events could happen before the widgets were laied out. This does not happen that often on Windows, but it happens almost all the time on GTK and Cocoa. I am not sure why it does not happen on Windows more often. It seems that the app goes idle before it paints. The new API has two other advantages. One is simplicity, there is no need to have a cake in jface that clients have to call. This would be necessary (with asyncExec()) to make sure all layouts happen in one single step in the event loop. And the second is that SWT is in control when layout happens. If running before the next event has issues, we have a chance of find a better place. It is hard to see the simple app flickering. E4 workbench has some issues related to layout and it is probably a good candidate to use this API.
Silenio, this is great news. It sounds like you've addressed the core issues here. I'll start tinkering with the new APIs shortly. Is there any way to use the proxy pattern with this new API, or is there still some additional support needed?
I do not think the new API addresses the ProxyControl problem. I am not sure how this could be addressed by SWT. This might not be a issue to the E4 workbench, since I beleive it is using Control.setParent() instead of proxy controls.
(In reply to comment #28) > I do not think the new API addresses the ProxyControl problem. I am not sure > how this could be addressed by SWT. > > This might not be a issue to the E4 workbench, since I beleive it is using > Control.setParent() instead of proxy controls. Yes, e4 won't run on platforms that don't support setParent. With e4, the Workbench is no longer in the business of "the big lie" :-)
If I understand correctly... after I change a widget, I should do the following to make it update its position: widget.getParent().layout(new Control[] {widget}, SWT.DEFER); ...or am I supposed to do this? widget.getShell().layout(new Control[] {widget}, SWT.DEFER); Either way, it might be easier for programmers if there was a convenience API that looked like this: widget.requestLayout(); ...it's simpler and has less room for programmer error.
> I do not think the new API addresses the ProxyControl problem. I am not > sure how this could be addressed by SWT. FYI, if you wanted to support ProxyControl (though the new setParent changes may have eliminated the driving use case for the pattern), it could be done with an additional listener type that allowed other objects to observe when a target widget is dirtied in its layout.
(In reply to comment #30) > If I understand correctly... after I change a widget, I should do the following > to make it update its position: > widget.getParent().layout(new Control[] {widget}, SWT.DEFER); > ...or am I supposed to do this? > widget.getShell().layout(new Control[] {widget}, SWT.DEFER); > Either way, it might be easier for programmers if there was a convenience API > that looked like this: > widget.requestLayout(); > ...it's simpler and has less room for programmer error. I would prefer not to add new concepts. People are used to call layout() on a composite. I believe widget.requestLayout() would also have one fallback. It would be hard for SWT to determine which parent of the widget it should call setLayoutDeffered(true). The simplest solution would be to do it for the whole shell. This would case a relayout on the whole shell (performance problem). The APP code knows this better. For example, the workbench can probably request layout on the root of a given view (ViewForm?) because views represent internal shells in a sense. SWT would have to add concepts like what is the root composite for layout purposes making the API more complicate I believe.
I echo the sentiment that this is good news... The scenario under which I expect this to be of the most use is in the 'trim' layout. The top trim in E4 will contain the toolbars and under a context switch (i.e. editor activation) many of them could change size (or become invisible...) but we'd only like one layout ultimately. It's worth noting that this is also about the -only- case in the UI Structure where a 'computeSize' makes sense. The Window/Stack/Sash containers need not ever ask their children how big they'd like to be (since we ignore the answer anyways...;-). Can you tell me if the deferred layout is smart enough to handle cases where we have a deferred layout on a Composite "A" and also on a child of "A" (in which case I'd expect the child's 'deferred' layout to be removed since it's inferred in the layout for "A"...? This means that if there's a layout pending for the Shell that -no other- deferred layouts need to be remembered...
(In reply to comment #33) > Can you tell me if the deferred layout is smart enough to handle cases where we > have a deferred layout on a Composite "A" and also on a child of "A" (in which > case I'd expect the child's 'deferred' layout to be removed since it's inferred > in the layout for "A"...? This means that if there's a layout pending for the > Shell that -no other- deferred layouts need to be remembered... No, the new API only affects the composite where the method is called (not the children). Calling: comp.layout(new Control[] {child}, SW.DEFER); is essentially the same as: comp.setLayoutDeferred(true); comp.layout(new Control[] {child}); comp.getDisplay().asyncExec(new Runnable() { public void run() { comp.setLayoutDeferred(false); } }); but the asyncExec() is done in a special way.
Reopening since - according to comment 32 - issue 2 in the original description is still not fixed. A requirement of this bug was that the app should not need to know the root widget affected by any given change, and the only way offered to do so is this: widget.getShell().layout(new Control[] {widget}, SWT.DEFER); However, if the app shouldn't do this due to performance issues, then it's still not fixed. Note: the original request also asks for support for the proxy pattern, but since SWT now supports reparenting everywhere, this is no longer necessary.
Actually, I'll leave this closed and can file a new bug tracking the remaining work.
Filed bug 468854 to request the missing functionality.
