Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #251280 +++ I was refactoring some code today and came across a PixelConverter class in PDE. I than did a search on the SDK to see who else was privy to having a PixelConverter class... turned out to be a party.
It was mentioned in bug #251280 that we should also take a look a SWTUtil. In that bug, Dani said: > Some of the methods are indeed dialog related (those using the > IDialogConstants) and some live in the SWT layer. As long as we don't separate > this I think putting it into org.eclipse.jface.util is a good solution. > > SWTUtil has the same problem: some of the methods are simply SWT helpers and > some need the IDialogConstants. Ideally we should split PixelConverter and > SWTUtil and create an SWT helper in SWT and a JFace helper in JFace but I guess > that would take much longer than "just do it" ;-) > Well, I thought I'd give it a try while I'm in code mining mode. I'm not going to propose helpers that go in SWT, but it does seem like there are places in JFace where some generic SWT methods could live. Note this proposal involves splitting the methods across several classes, so I don't know if this is controversial or not. I looked at 8 implementations of SWTUtil. There is not nearly as much in common with the various implementations, and they seem to be attacking different problems. Some of the methods have side-effects that don't seem correct for a general-purpose API (like slamming a control with the dialog font if it has the default font, or methods that have no effect if the layout data isn't a GridData, etc.) There were 5 methods that were fairly common to all versions of SWTUtil, so I attempted to provide implementations in the right place in JFace. Plug-ins with small SWTUtil implementations should be able to use these alternates and get rid of SWTUtil. Clients of larger SWTUtil implementations could switch over to use these new methods, though I'm not holding my breath. Or at least the existing larger SWTUtil implementations could call these methods inside. Here are the proposed new classes/methods. New class: org.eclipse.jface.layout.LayoutUtils This is where calculations involving pixel conversion and IDialogConstants would live. We could consider adding more methods in this class, such as the ones for default table height, default combo size, etc. How far should we go? For now I propose getButtonWidthHint(Button) setButtonDimensionHint(Button, GridData) - note the signature and pattern is slightly different. You pass in the grid data and it will get set on the button for you, rather than the typical implementation which assumed you had already set it, retrieved the layout data, and didn't do anything if the button's layout data wasn't a grid data. New methods in org.eclipse.jface.util.Util This is where we could put plain SWT helpers, note we already have some in this class regarding getting the current platform getShell(Widget) getStandardDisplay() New method in org.eclipse.jface.dialogs.Dialog setMinimumDialogSize(Dialog, int int) One could argue that this could be made a generic util involving a Shell rather than a Dialog. However, it didn't seem to have a home if it was a generic setMinimumShellSize(...) method, so I left its dialog-ness and propose putting it in Dialog. I'm a bit torn here. Existing method people should know about GridLayoutFactory.fillDefaults() returns a layout with no margins appropriate for use in a dialog and could replace or at least be used in the implementation of org.eclipse.jdt.internal.ui.util.SWTUtil.newLayoutNoMargins(int) I'll attach the proposed patch for all this and get some feedback before committing anything.
Created attachment 117024 [details] proposed changes patch for changes described in last comment
I must have fumble-fingered the lock button.
Bug 244123 is essentially the same bug, but not so clever a title and no solution.
*** Bug 244123 has been marked as a duplicate of this bug. ***
How widely are these 2 used? getShell(Widget) getStandardDisplay()
(In reply to comment #6) > How widely are these 2 used? > > getShell(Widget) > getStandardDisplay() > I have a workspace with parts of the SDK loaded. Can't speak for other projects... getStandardDisplay() I see 128 references across UI, JDT, PDE, Ant, Debug, Team, Update I see 12 declarations, across copies of SWTUtil or in plug-in classes getShell(Widget) I see 4 implementations, across copies of SWTUtil. I see only 8 references, and it appears only 2 implementations are used, the rest were probably copied while copying SWTUtil.
I'm just a lurker, but it seems awkward to me to add a static Dialog.setMinimumDialogSize(Dialog, int, int) method. Why not make it a non-static and drop the first parameter? In that case, setMinimumSize might be a more natural method name.
(In reply to comment #8) > I'm just a lurker, but it seems awkward to me to add a static > Dialog.setMinimumDialogSize(Dialog, int, int) method. Why not make it a > non-static and drop the first parameter? In that case, setMinimumSize might be > a more natural method name. > Interesting. One worry I have with an instance method is that the method might imply some minimum size is honored throughout the life of the dialog, rather than just a one-time setting of the size to the greater of the default or proposed size. Maybe that could be overcome with a better name...? Either way, we have to be clear about when the method should be called.
If we could truely get rid of these methods (ie. they are not API and will never be called again), then we could consider inlining them or adding the functionality to SWT and calling that. getStandardDisplay() is "give me the display for the thread, if there isn't one, give me the default". We could add Display.findDisplay(Thread, boolean) that did this or add a new concept/name to SWT. getShell(Widget) is "I don't know what I have, because I have lost the type information, but I know it exists in a shell somewhere. Find me that shell." We could add getShell() to widget or getShell() to some of the types in question. Before doing either, I'd like to understand if inlining makes sense.
Please rename src/org/eclipse/jface/util/Util.java to "JFaceUtil.java". If you have to use more than one "Util" class, this really helps :)
Some more ideas for SWTUtil: - Save/restore preferences - Null-safe dispose: public static Color dispose (Color c) { if (c != null) c.dispose (); return null; } Used like so: errorColor = dispose (errorColor);
(In reply to comment #11) > Please rename src/org/eclipse/jface/util/Util.java to "JFaceUtil.java". If you > have to use more than one "Util" class, this really helps :) > Aaron, if you feel strongly on this, I'd open a separate bug. This would be a breaking API change and I don't think we would pursue this suggestion unless there was broad community support. (In reply to comment #12) > Some more ideas for SWTUtil: > > - Save/restore preferences > - Null-safe dispose: > > public static Color dispose (Color c) > { > if (c != null) > c.dispose (); > return null; > } > > Used like so: > > errorColor = dispose (errorColor); > hmmm...I don't see replacing a simple two-liner with a new method, esp if there's not already a pattern in the SDK where people have done this. The crux of this bug is trying to look at the helpers that are copied all over the SDK and see if putting them in JFace can get rid of them, or convincing clients that the helpers are so simple that they should in-line them.
All - is there any feedback regarding: - the proposal in comment #1 and the patch - the suggestion in comment #10 that some of the SWT-level methods could be inlined back to the caller or that SWT could support them otherwise - other suggested changes to the proposal (comment #8) This is something I was willing to do if there is clear consensus and we know at least a few of the SWTUtil classes will go away if we do this, but so far I'm not hearing much enthusiasm.
Susan, how about an email to cross-projects-dev asking for feedback? I'm sure there are clients outside the SDK that may benefit from this. (I'm enthusiastic)
(In reply to comment #13) > (In reply to comment #11) > > Please rename src/org/eclipse/jface/util/Util.java to "JFaceUtil.java". If you > > have to use more than one "Util" class, this really helps :) > > > Aaron, if you feel strongly on this, I'd open a separate bug. This would be a > breaking API change and I don't think we would pursue this suggestion unless > there was broad community support. How can this be a breaking change when you just created the file if the attached patch? Has there been an Eclipse release since the 4th of November and I didn't notice? > > - Save/restore preferences > > - Null-safe dispose: > > > > public static Color dispose (Color c) > > { > > if (c != null) > > c.dispose (); > > return null; > > } > > > > Used like so: > > > > errorColor = dispose (errorColor); > > hmmm...I don't see replacing a simple two-liner with a new method, esp if > there's not already a pattern in the SDK where people have done this. It's not a two liner, it'd be a five liner: if (errorColor != null) { errorColor.dispose(); errorColor = null; } It helps to avoid leaks and find many kinds of resource bugs (by causing NPEs when a disposed resource is used). That's why I try to enforce this pattern. SWT often calls isDisposed(). With my pattern, you wouldn't need this call and if you keep it, you would have a second safety net at very little extra cost.
(In reply to comment #16) > How can this be a breaking change when you just created the file if the > attached patch? Has there been an Eclipse release since the 4th of November and > I didn't notice? > > hmmm...I don't see replacing a simple two-liner with a new method, esp if > > there's not already a pattern in the SDK where people have done this. > > It's not a two liner, it'd be a five liner: > > if (errorColor != null) > { > errorColor.dispose(); > errorColor = null; > } > > It helps to avoid leaks and find many kinds of resource bugs (by causing NPEs > when a disposed resource is used). That's why I try to enforce this pattern. > SWT often calls isDisposed(). With my pattern, you wouldn't need this call and > if you keep it, you would have a second safety net at very little extra cost. > Understood, I wasn't considering the brackets or the reset of the variable since often this happens in the dispose method of a dialog. I agree that when you are reusing/reopening a dialog instance the nulling of the variable becomes important. Let's see what folks have to say about this one.
(In reply to comment #16) > How can this be a breaking change when you just created the file if the > attached patch? Has there been an Eclipse release since the 4th of November and > I didn't notice? (weird, I typed an answer to this in the previous comment and must have deleted my remarks somehow)... Util has been around in JFace as API since 3.1. The patch merely adds two more methods to it...
> > How can this be a breaking change when you just created the file if the > > attached patch? Has there been an Eclipse release since the 4th of November and > > I didn't notice? > > (weird, I typed an answer to this in the previous comment and must have deleted > my remarks somehow)... > > Util has been around in JFace as API since 3.1. The patch merely adds two more > methods to it... Thanks for the clear up. I just looked at the patch file and it seemed to be "+" only :)
> if (errorColor != null) > { > errorColor.dispose(); > errorColor = null; > } This is a perfect example of a wierd util method that might make sense internally, but should not be API. It is a wrapper for calling a method and setting a variable to null. if (errorColor != null) errorColor.dispose(); errorColor = null; vs. errorColor = dispose (errorColor); What did Color do that was so special to be singled out among other things in the world that need to be disposed and set to null? In a class or component that dealt with colors, creating and disposing many of them, manipulating them in interesting ways etc. (ie. where Color is the major concept) then the component owner could make a good argument for elavating the "dispose then set to null" concept to a until method in his class or component (ie. non-API). The argument for a non-API method might go like this "I do it in a million places, colors are the major thing I deal with so I am saving code and avoiding bugs in my component (ie disposing but not setting to null)". The arguement for an API util method might go like this: "World! Listen up! Color disposing is a big problem and I have just the solution for you. Everyone pleawse call this method and your color disposing and non-nil'ing problems will be over." Personally, it's only 2 lines, so I would delete the method and inline it.
(In reply to comment #20) > > if (errorColor != null) > > { > > errorColor.dispose(); > > errorColor = null; > > } > > This is a perfect example of a wierd util method that might make sense > internally, but should not be API. It is a wrapper for calling a method and > setting a variable to null. > > if (errorColor != null) errorColor.dispose(); > errorColor = null; > > vs. > > errorColor = dispose (errorColor); > > What did Color do that was so special to be singled out among other things in > the world that need to be disposed and set to null? Color is just an example which I chose because everyone here knows it. This API should be available for all resources (fonts, colors, images, widgets). In my code, I began to use this pattern for JDBC resources and I found the change really worth it (especially because of the Exceptions I need to handle there). I also find your choice of formatting interesting. For me, your argument sounds as if you didn't care whether code is readable. From the view of the brain, the less it has to decode and the more similar the lines are (think "tables" and why and when they are more readable than prose text), the easier it is to understand what is going on. Your code quickly becomes hard to read when you have to dispose many resources. With my pattern, you just read the first line and then your brains says "the next 20 lines are the same". That's just two bits of information instead of having to read every line to check what it says. Plus, when you cut'n'paste this code, you have to replace the field name just twice instead of three times and the chance to make a mistake is only 50% instead of 66%. That said, if we introduced an interface "ToDispose", we would need just one util method for all resources that need to be disposed and it would be possible to write a tool that can search for all fields which aren't disposed anywhere. Personally, I'd love to see this but I doubt that anyone here would agree, so I'm opting for the two util methods I outlined above (one for the class Resource from which Color, Image, etc. are derived and one for Widget).
This is one reason why there will always be util classes with a random assortment of methods in them: There can never be agreement on subjective concepts such as readable code. For example, given the amount of code I read, heading off to SWTUtil.dispose() only to find out that all it does it dispose a color and return null is a bit of a let down. I can understand that it could make sense if it were extended to track colors, or reference count them, or avoid disposing system colors etc. At this point, reading the code for a set of classes that was focused on color manipulation, even if I headed off to SWTUtil.dispose() and found nothing interesting, I'd say "well, they're making color disposing all go through one place" (I'd also expect to see allocation done there too). A better alternative to the interface "ToDispose", would be to simply change dispose() to return null but then, you get into the casting stupidity of Java in order to return the right kind of null. errorColor = (Color) SWTUtil.dispose (errorColor); errorImage = (Image) SWTUtil.dispose (errorImage);
(In reply to comment #15) > Susan, how about an email to cross-projects-dev asking for feedback? I'm sure > there are clients outside the SDK that may benefit from this. > > (I'm enthusiastic) > I did post to the cross-project-issues list on 11/26 but didn't get any new commenters in the bug as a result. I'm going to remove the M4 milestone since this won't be resolved this week. I'm not going to reassign a milestone unless I get feedback in this bug that someone would actually remove their SWTUtils class if this idea (or an alternate) is implemented.
As per http://wiki.eclipse.org/Platform_UI/Bug_Triage_Change_2009
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
