Community
Participate
Working Groups
We have a component which allows pixel-perfect scrolling of native SWT Tables on Win/Mac/Linux. We'd like to open source these components, but we need to be able to call the methods of SWT Layout directly. SWT Layout's methods are protected. This means that if you want to create a Layout that modifies the behavior of an existing layout by calling its methods, it is impossible unless you write that code inside the `org.eclipse.swt.widgets` package. To workaround this, we have had to strip the Eclipse signing metadata out of our SWT binaries and patch them with our 'DelegatingLayout' class. It's simply an SWT Layout which delegates all its method calls to another layout, with the important caveat that all of its methods are public. This effectively acts as an escape hatch to allow public access to the methods of SWT Layout. Because anyone can extend SWT Layout, it isn't any riskier in terms of security or exceptions than the existing code. It just allows custom 3rd party components to have control over when the methods of a Layout are called. I will submit an implementation shortly.
New Gerrit change created: https://git.eclipse.org/r/94877
Ned, I think it would help a lot if you can list the methods that you need and how they are used. Honestly, I'd rather avoid the DelegatingLayout and see if there are other ways to support you use case (eg., introducing new public API on Layout).
I need all 3 of Layout's methods to be public, rather than protected. This makes it possible for Layouts to be composed (one layout can call into other layouts). If you made Layout's methods public, that would break API compatibility. DelegatingLayout is a clean way to make the Layout API public without breaking API compat. For example: FixedWidthLayout extends DelegatingLayout { final int fixedWidth; public FixedWithLayout(Layout delegate, int width) { super(delegate) this.fixedWidth = width; } @Override public Point computeSize(Composite composite, int wHint, int hHint, boolean flushCache) { int height = super.computeSize(composite, fixedWidth, hHint, flushCache); return new Point(fixedWidth, height); } } This lets me take *any* existing Layout, and modify it so that it will always have a fixed width. Because Layout's methods are protected, it is impossible to achieve this without introducing new API to the SWT jar. The three ways to achieve this are: - add new public methods to Layout that delegate to the existing ones e.g. public final boolean flushCachePublic(Control ctl) { return flushCache(ctl); } - add a new class such as DelegatingLayout - add C-style static methods, which someone outside of SWT can use to make DelegatingLayout e.g. public static boolean flushCachePublic(Layout layout, Control control) Of these three, DelegatingLayout seems to me like the cleanest change. For the specific case of pixel-perfect scrolling for native tables, we use positioning tricks to achieve pixel scrolling. The user can use whatever layout they were using before (TableLayout or a custom Layout) and we are able to modify its behavior from the outside to achieve pixel-perfect smooth scroll. But we can only do this if we are able to call the Layout methods through a public API.
New Gerrit change created: https://git.eclipse.org/r/95027
I added a Gerrit change with the alternative: Add public API to Layout: https://git.eclipse.org/r/#/c/95027/1/bundles/org.eclipse.swt/Eclipse+SWT/common/org/eclipse/swt/widgets/Layout.java Add a new DelegatingLayout class: https://git.eclipse.org/r/#/c/94877/1/bundles/org.eclipse.swt/Eclipse+SWT/common/org/eclipse/swt/widgets/DelegatingLayout.java
(In reply to Ned Twigg from comment #3) > This lets me take *any* existing Layout, and modify it so that it will > always have a fixed width. Because Layout's methods are protected, it is > impossible to achieve this without introducing new API to the SWT jar. I'm trying to understand the problem. I think my concern with DelegatingLayout is that it sounds like a hack. It's technically ok and clean but introducing a "delegating" class for accessing protected methods feels wrong to me API wise. It sounds to me like the best solution is to make the pixel-perfect scrolling functionality part of the same package. This would still be new API (potentially) but the API offers new functionality. However, without seeing how you intend to use the methods, it's hard to come up with alternate solutions. Right now, the pixel-perfect scrolling would be the only client of these methods. It's not a good idea to add new API for just one single client. I can't judge if those methods are save to be called by anyone, i.e. become public in some form or another. Is it possible to implement the "scrolling component" in the same package so that it can access the required methods in Layout?
> I think my concern with DelegatingLayout is that it sounds like a hack. I think Layout + DelegatingLayout is an elegant solution to the tension between hiding implementation details and allowing composition. Usually, people don't need to call the methods of a layout - it's handled by the Composite. So it makes sense to hide them most of the time. And for the less-common case where you're using a Layout as a component to implement a new kind of Layout, it makes sense to wrap the Layout as a DelegatingLayout to signal "we're using this Layout as a component in a broader system". Java generally prefers inheritance over composition, but Java 8 has helped to move functional programming and preferring composition to inheritance into the mainstream. With SWT Layouts, it is impossible to use composition - it is only possible to use inheritance. That limitation is the reason why I have to bother you with this PR at all - to make it possible to use composition instead of inheritance. > Is it possible to implement the "scrolling component" in the same package so that it can access the required methods in Layout? Our class is a drop-in replacement for table. If you're currently using the standard TableLayout, or any custom in-house implementation of Layout, we can give you pixel-perfect scrolling - if this PR is merged in some form. If this PR isn't merged, we'll have to do something like this: public class PublicLayout extends Layout (make all methods public) public class PublicTableLayout extends PublicLayout (copy-paste TableLayout's implementation) Users have to change all of their layouts to extend PublicLayout instead of Layout. We also have some ideas about responsive design. Currently, you can't reuse GridLayout / FillLayout / RowLayout etc. in any way. With this PR, you could allow users to wrap the layouts they know with new functionality.
(In reply to Ned Twigg from comment #7) > Our class is a drop-in replacement for table. This is actually my point. Instead of providing DelegatingLayout and opening up methods which should only be called from within Composite, why wouldn't you want to enhance/fix/patch table directly to provide the pixel perfect scrolling? It's just not good API design to introduce something for just one consumer. So, let me precise my question, is it possible to submit a patch for Table to get pixel-perfect scrolling implemented without having to open up those Layout methods? This is just a question, though. Please don't go down that route if it's a lot of work. There might be additional questions as well. Some of which could be show-stoppers, which explains why it's not possible. For example, what are the performance implications of patching Table, would it break existing consumers, etc? I think such discussion is necessary to really get the context that drove the solution of DelegatingLayout. :)
> It's just not good API design to introduce something for just one consumer. Absolutely!! I think I won't be the only consumer if it's released, but we'll see. FixedWidthLayout shows the broad power of composition. > Is it possible to submit a patch for Table to get pixel-perfect scrolling implemented without having to open up those Layout methods? Nope. Standard way to make a table is this: Composite cmp = new Composite Table table = new Table TableLayout layout = new TableLayout cmp.setLayout(layout) To achieve pixel-perfect scrolling, you replace Composite with SmoothTableComposite. I should have said it was a drop-in replacement for the parent composite, rather than a replacement for Table, apologies. So I guess the next question is "could we add the methods to Composite?". If we go with "C-style OO", then we can add the methods to any class in `swt.widgets`, but that seems like more of a hack. I threw the relevant part of the code up in a gist: https://gist.github.com/nedtwigg/ede2fb9537141965fd1cebceb8a4c9fb#file-abstractsmoothtable-java-L86-L112 Technically, I only need access to `void layout(Composite composite, boolean flushCache)` for this particular case, but the technique has been so effective we're hoping to use it for other cases. It's also possible that there are bugs because we're ignoring some method calls. We like DelegatingLayout because it gives us all the access we need to fix bugs in the future.
Delegating between layouts in the manner suggested here is dangerous because of the extremely tricky (and poorly documented) API contracts on layouts. For example, the FixedWidthLayout example here contains a bug that is quite common in custom layouts: computeSize is expected to return wHint and hHint verbatim when they are not SWT.DEFAULT. The correct code would look more like this: public Point computeSize(Composite composite, int wHint, int hHint, boolean flushCache) { if (wHint == SWT.DEFAULT) { wHint = fixedWidth; } if (hHint == SWT.DEFAULT) { hHint = super.computeSize(composite, wHint, SWT.DEFAULT, flushCache); } else if (flushCache) { // If flushCache is true but we don't actually need the result of // computeSize, perform the least-expensive call to computeSize // that will initiate a cache flush. super.computeSize(composite, SWT.DEFAULT, SWT.DEFAULT, flushCache); } return new Point(wHint, hHint); } ...and sadly this sort of bug is both extremely common and extremely easy to write by accident. Encouraging more custom 3rd-party layouts will make this type of bug more common. Also, delegation between layouts is inherently problematic. The reason you'd normally want to do so (as with the example here) is to adjust the result of computeSize. However, computeSize isn't side-effect-free. Layouts are allowed to attach and cache metadata to the child controls using child.setLayoutData(...), or to modify the layout data that is already attached to the child controls. If there are two layouts delegating to one another and attached to the same composite, ownership of the child control's layout data is no longer well specified. If they both try to modify the layout data, one of the two layouts will have their caches overwritten. True, we could fix this with careful JavaDoc (most likely, we'd want to give ownership of the layout data to the innermost layout in a delegation chain), but then the contracts would be different for delegating and non-delegating layouts. I'd like to suggest some alternatives: Option 1: Rather than allowing layout delegation, we write a new ISizeComputer interface that can be attached to any control in order to modify its preferred size. ISizeComputer would have simpler contracts than layout and would be intended specifically for this sort of delegation. ISizeComputer might look something like this: interface ISizeComputer { /** * wHint the outer width of the control or SWT.DEFAULT if unbounded. Must * return this size verbatim if it isn't SWT.DEFAULT. * hHint the outer height of the control or SWT.DEFAULT if unbounded. Must * return this size verbatim if it isn't SWT.DEFAULT. */ public Point computeSize(int wHint, int hHint); /** * Clears any cached information about this control's size. */ public void clearCache(); } All controls have a getter and setter for its ISizeComputer, and the default ISizeComputer for a control behaves just like the innards of Control.computeSize, however, when computeSize invokes the sizeComputer it would ensure it handles the special cases for flashCache and for converting the inner bounds hints passed to computeSize into the outer-bounds hints that are used by layouts. Since ISizeComputers aren't allowed to cache anything in the child control's layout data, they can safely delegate to one another. Option 2: We handle the use-case of FixedWidthLayout by adding a new preferredWidthOverride attribute to Control. If set anything other than SWT.DEFAULT, it does what FixedWidthLayout was intended for. This should accomplish what you want with FixedWidthLayout but means we only need to add a single new int as API rather than a new overridable method. Option 3: Instead of a single new int, we add two: minWidthOverride and maxWidthOverride. minWidthOverride would address the common problem of Buttons, where we want a minimum size for the click target but we still want the button to grow if necessary to fit the text label. maxWidthOverride would address the common problem of wrapping controls, where we want the control to wrap after a certain number of horizontal pixels but we still want to permit the control to shrink if the wrapping control isn't large enough to fill a single line. If both attributes are non-SWT.DEFAULT and are equal to one another then they behave exactly like perferredWidthOverride would in option 2. Option 4: We do the same as Option 2 or Option 3, but put the new attributes on GridLayout rather than Control. Option 5: No new API at all, since you can already delegate between Layouts by wrapping them in an extra Composite. See org.eclipse.ui.internal.layout.CacheWrapper for an example. IMO, any of these is preferable to direct delegation between layouts. I personally like option 3. It's not nearly as general as option 1, but it has a smaller API footprint and would be very easy for end-users to consume.
Note that an alternative to the ISizeComputer interface suggested in comment 10 would be the ISizeProvider interface suggested in the patch attached to bug 103863. It serves the same purpose (provides a simpler API that supports delegation and allows consumers to modify the preferred size behavior of existing controls). The patch attached to bug 103863 contains a demo implementation demonstrating how the interface would be used in context.
These are all good workarounds for FixedWidthLayout, and also good documentation for parts of Layout that I didn't know about, thanks! The real reason for this PR isn't about FixedWidthLayout, but about pixel-perfect native table scroll, which is unrelated to the the workarounds. https://gist.github.com/nedtwigg/ede2fb9537141965fd1cebceb8a4c9fb#file-abstractsmoothtable-java-L86-L112 I agree that Layout is tricky to implement and custom solutions should be discouraged. DelegatingLayout is a simple way for people who *have* to implement a new layout to leverage the existing ones. Your Option 5 is a very good suggestion: > Option 5: No new API at all, since you can already delegate between Layouts by wrapping them in an extra Composite. See org.eclipse.ui.internal.layout.CacheWrapper for an example. This can probably be made to work for our usecase. It's less efficient and less elegant (one extra heavyweight Composite), but we can probably make it work.
> It's less efficient and less elegant (one extra heavyweight Composite), > but we can probably make it work. Efficiency is important. Even if you go with my Composite hack, we should consider improving the layout API to do some of this stuff better. However, it does make sense to separate the concerns -- if the Composite hack works, you could start with that and then investigate improving the performance by adding some API. If we can also address some of the missing use-cases (such as wrapping text and buttons, mentioned in my earlier comment), that's even better. I also agree with the earlier comment suggesting that we need to see this in context to ensure we do a good job of designing the API. (To be honest, I don't really know what you mean by "pixel-perfect scrolling").
By pixel-perfect I mean that instead of `setTopIndex(int)` we can do `setTopRow(double)`, which allows smooth scrolling. You can see the code in the gist. We can take any Layout that currently supports Table, and add the ability to do smooth scroll with the layout you're already using. Your points about some Layout not being pure is an important one, but so long as the container layout isn't relying on caching (we don't) then composition will still work. Layouts already have gotchas, this doesn't seem to make them much worse. DelegatingLayout really seems like a missing piece of API to me. There's no way to reuse an existing layout without either hurting performance (unnecessary Composites) or forking SWT (our current solution). We've got two diverse use cases (FixedWidthLayout and our scroll thing), and they're both tricky, but both doable with DelegatingLayout. The other workarounds seem like point-solution gymnastics compared to the general purpose problem of making an already user-accessible API callable (protected -> public).
> The other workarounds seem like point-solution gymnastics compared to > the general purpose problem of making an already user-accessible API > callable (protected -> public). You're not just making it public, you're also breaking an important invariant by using delegation - Layouts by design have a 1-to-1 relationship with their Composite and most layouts rely on this quite deeply. Wouldn't the ISizeProvider/ISizeComputer interface discussed above give you all the generality you need? It would do everything you can currently do by overriding computeSize, but it would support exactly the sort of composition you envision.
Regarding this: https://gist.github.com/nedtwigg/ede2fb9537141965fd1cebceb8a4c9fb#file-abstractsmoothtable-java-L86-L112 I took a quick look at AbstractSmoothTable, and I had some comments. It looks to me as though it handles sub-row scrolling by shifting the entire table up or down to position the rows correctly. This is probably less efficient than implementing this feature directly in SWT (since most of the underlying widget toolkits already support fine scrolling). The technique it uses to compute the width/height attributes is incorrect (relying on event timing to save the widget width in a resize listener and reading it back in the layout is error-prone). The delegating layout itself seems unnecessary. Why would someone want to call setLayout on an AbstractSmoothTable to begin with? Wouldn't you always use the default implementation set in the constructor? Even if someone did want to set a custom layout for this Composite, the DelegatingLayout itself isn't doing anything. Assuming your implementation of DelegatingLayout is the same as the one attached here, this line: this.layout = new DelegatingLayout(layout); is the same as this: this.layout = layout; ...and the DelegatingLayout is a no-op.
> Layouts by design have a 1-to-1 relationship with their Composite and most layouts rely on this quite deeply. Depending on your point of view, this design is either a bug or a feature. From a functional perspective, this is a bug. If you want to even try a functional take on SWT layouts, there is currently no workaround without forking or taking a performance hit with phantom Composites. > the DelegatingLayout is a no-op. That's the point. It's a no-op that allows us to call a layout's methods. This way we can make custom layout that moves the whole table, then calls methods on an existing layout to align the columns. > It looks to me as though it handles sub-row scrolling by shifting the entire table up or down to position the rows correctly. On GTK and Cocoa, we do use native pixel scrolling to achieve sub-row scrolling. On win32, we have no choice but to use the inefficient but effective technique of shifting the entire table up and down. (The gist is AbstractSmoothTable, there's a different SmoothTable impl per-platform). We also allow for the top row to be negative, or to scroll past the bottom of the table. To achieve this, we have to position the entire table even on GTK and Cocoa. Negative indices are useful for showing modifications smoothly (e.g. delete the top rows, then smoothly scroll the new top of the table to the top of the window). > Why would someone want to call setLayout on an AbstractSmoothTable to begin with? Because they're using a Layout to align their columns. > (relying on event timing to save the widget width in a resize listener and reading it back in the layout is error-prone) It was indeed error-prone, but getting this hacky smooth scroll at 60 fps took some tricks. We've been shipping it for a year without any bug reports on this feature.
> This way we can make custom layout that moves the whole table, then > calls methods on an existing layout to align the columns. My bad. I missed the call to layout.layout on line 96. Okay, I see that it's not a NO-OP. > From a functional perspective, this is a bug. It's not a bug - it's working as intended and the intent was wrong. It's a design flaw. One of many design flaws in the SWT layout system. However, we can't ignore the API contracts on SWT layouts just because they're poorly designed. Violating the API contracts would be a bug. That's what the forms library did, and it's the reason why form controls are incompatible with every other SWT contro (fixing it has probably taken way more man-hours than ever went into that library to begin with). As long as layouts permit data to be cached on the controls and write their output by modifying the controls directly, we can't support delegation between layouts. It will only work in the simplest cases and everywhere else you'll end up in situations where you have contradictory API contracts. > We've been shipping it for a year without any bug reports on this feature. I'm worried about breakage due to future trivial refactoring within SWT. You might want to consider using the width and height of the composite passed to the layout method instead. But that's orthogonal to the layout discussion. Sorry for pulling us off topic. > On win32, we have no choice but to use the inefficient but > effective technique of shifting the entire table up and down. Fair enough. I'd suggest asking some SWT Win32 gurus if they can think of anything better, but if you've already tried all the elegant solutions, what you describes sounds like an acceptable last-resort workaround. > Because they're using a Layout to align their columns. Could you provide an example of such a Layout? One alternative might be to use a new custom interface for computing Table column widths. Something like this: interface IColumnSizer { int[] computeColumnWidthsFor(int tableWidth); } ...and in that case, the problem of delegating between Layouts disappears. You don't need to expose the ability to modify the table layout and you'll have full control over the layout in the custom implementation you provide in the AbstractSmoothTable constructor. Your comment explains this bit of unusual code: table.setBounds(0, offset, width, tableHeight); if (layout != null) { layout.layout(composite, flushCache); } Normally it's the responsibility of the layout to set the bounds of all child controls so any bounds set by table.setBounds(...) would be ignored and overwritten by the call to layout.layout. However, it sounds like that particular layout isn't really behaving like a normal layout -- you just need a callback that gets invoked during the layout pass so you have a place to put the code that sets the table's column widths. That seems like an even stronger argument for writing a new interface.
I really don't think supporting delegation between SWT layouts is a productive thing to discuss. I'd suggest we focus on the bigger problem of trying to get your smooth scrolling table working efficiently without an SWT fork.
> Could you provide an example of such a Layout? One alternative might be to use a new custom interface for computing Table column widths. http://www.vogella.com/tutorials/EclipseJFaceTableAdvanced/article.html#tablecolumnlayout This problem is already very well solved! We don't need a new interface, we just need a way to reuse the existing solutions. > supporting delegation between SWT layouts is not a productive thing to discuss Fair enough, feel free to close. We'll look at the performance impact of an extra Composite, and depending on that we'll take the hit or publish our fork since it's so easy to maintain this single isolated class. We're not interested in discussing brand new API's - the existing Layout API works great for us. We just want access to what's already there, so that it's possible to layer new functionality. I'm surprised that it's easier to add new Layout API to SWT than to expose the Layout API it already has. > We can't ignore the API contracts on SWT layouts just because they're poorly designed. Violating the API contracts would be a bug. Sure we can! We've been doing it productively for over a year! I don't understand the reasoning of your core argument - it is possible to write an SWT Layout with side-effects, so they should always be written from scratch. I take the same premise and come to the opposite conclusion - it's hard to write an SWT Layout, so they should be reused if possible. Your argument about side effects is important but incomplete - there is such a thing as useful but imperfect composability - a grey area between the current SWT Layout and pure functions. DelegatingLayout makes it very easy to harvest the latent value in that grey area. Almost all java concurrency code is in this category of useful but risky composability.
> We're not interested in discussing brand new API's - the existing Layout > API works great for us. I disagree. You're asking for a change, which suggests it doesn't address your use-cases perfectly. > We don't need a new interface, we just need a way to reuse the > existing solutions. It looks to me as though you're authoring both the implementation of the interface and all of the callers. Why insist upon changing an interface that isn't exactly what you need when it's so easy to make a new one that can do anything you want? > there is such a thing as useful but imperfect composability - a grey area > between the current SWT Layout and pure functions. Fair enough. But if we permitted such uses, we'd have to document exactly what is and isn't allowed. It's not enough to just make the methods public. Layouts that exist in the "gray area" would need to make this as a guarantee to their callers. Such layouts that initiate delegation would need to - for example - promise not to create, read, or modify the layout data attached to child controls. They would need to make promises about whether or not they will invoke setBounds on each child control, since any given delegation chain should only do so once per control regardless of how many layouts are in the chain. You'd also have to deal with breakage in libraries like forms, where layouts recognize instances of themselves in child controls by calling (child.getLayout() instanceof Something). Is this still allowed? Will there be restrictions on it? You could deal with it by banning such layouts from being delegated to in delegation chains, but you'd have to be explicit about which layouts use that pattern and any layout that uses delegation would need to be explicit that they don't support such layouts. All this stuff should be thought through and well documented... and once it's documented, the new API contracts need to be adhered to for all time. The sort of convoluted JavaDoc we'd have to write for this doesn't sound easy for developers to understand or program against, and I'm not a fan of the corner it would paint us into when making future changes to the layout system. > Fair enough, feel free to close. I'm not going to do that. Having you maintain a fork of SWT is awful, and we shouldn't storm off an a huff and use the worst-possible solution. There's ways to make your proposal work and - provided all the corner cases are properly documented and non-contradictory - I'm not completely opposed to them. I just think that there are much better alternatives we should consider first. Do we have evidence that one extra Composites per table is actually expensive, or would we be prematurely optimizing? What is the problem with creating a new interface for setting column widths in your custom Table? Why does it need to be specifically an instance of Layout?
Sorry, didn't mean to imply a huff :) You have a broader audience to worry about than I do, I 100% respect that. But this thread is consuming your time and my time, and we already have the workflow for maintaining this fork, so our threshold is low. I don't want you to merge code that makes SWT worse, but we also don't have an unlimited attention budget for contributing upstream. > It looks to me as though you're authoring both the implementation of the interface and all of the callers Nope. The common implementation is this: http://www.vogella.com/tutorials/EclipseJFaceTableAdvanced/article.html#tablecolumnlayout But users might have others. No one can call these methods without DelegatingLayout. > Do we have evidence that one extra Composites per table is actually expensive? This could very well be a solution. We've put a lot of testing into our current solution on many platforms, so it's not an easy swap for us, just like merging this code isn't an easy thing for you. No huff, we just don't have alignment :)
Sorry, I didn't mean to misrepresent myself: I'm not an SWT committer.
