Community
Participate
Working Groups
GridLayoutFactory and GridDataFactory have very few examples within the Eclipse code base. Eclipse should adopt them within the JFace framework. This will: - Help make JFace layout code more readable - Provide some examples of their use
Created attachment 49789 [details] Patch for this enhancement This patch adopts Grid*Factory throughout most of JFace.
This patch breaks at least TitleAreaDialog - I am not sure about the others as I didn't check. Please test these changes and reopen with updates.
Created attachment 50075 [details] Bugfixed and tested patch This patch has been merged with head, bugfixed, and tested. I have excercised and tested all changes in the patch with the exception of the following (which I haven't yet found a code path to exercise from the UI). Note that in many cases I've tested the rest of the method, but I just haven't exercised the branch on the indicated line number. Tod: if you could help me out with the last little bit of testing, I'd appreciate it. ContentProposalAdapter$ContentProposalPopup [line: 671] - adjustBounds() ContentProposalAdapter$ContentProposalPopup$InfoPopupDialog [line: 432] - createDialogArea(Composite) DialogPage [line: 349] - setButtonLayoutData(Button) ErrorDialog [line: 201] - createDialogArea(Composite) ErrorDialog [line: 250] - createDropDownList(Composite) MessageDialogWithToggle [line: 467] - createToggleButton(Composite) PopupDialog [line: 458] - createContents(Composite) PopupDialog [line: 500] - createDialogArea(Composite) ProgressMonitorPart [line: 159] - initialize(Layout, int) StatusDialog [line: 233] - createButtonBar(Composite) TrayDialog [line: 329] - openTray(DialogTray)
Reopening since Tod's bugs have been fixed and (the majority of) the remainder of the patch has been tested.
Released the change (additional asserts) to GridDataFactory.
released so far: PopupDialog, IconAndMessageDialog.
I have a couple of issues with the changes made to GridDataLayout: 1) The spec in the javadoc does not match the code (i.e. there are some additional constants that the code checks for that are not mentioned in the javadoc) 2) This API was added in 3.2 but was modified to assert on invalid values in 3.3. While technically correct, this change has pretty dramatic effects for clients who passed an invalid value. Basically, users will no longer be able to use the plug-ins until they are upgraded (see bug 176300).
(In reply to comment #7) > 1) The spec in the javadoc does not match the code (i.e. there are some > additional constants that the code checks for that are not mentioned in the > javadoc) This is intentional. The constants defined in GridData are not recommended (but not deprecated - yet?), so we don't advertise them in the Javadoc but permit them when checking for illegal arguments. > 2) This API was added in 3.2 but was modified to assert on invalid values in > 3.3. While technically correct, this change has pretty dramatic effects for > clients who passed an invalid value. Basically, users will no longer be able to > use the plug-ins until they are upgraded (see bug 176300). You are right. We should change this to only throw an exception if in debug mode, or log a warning instead of throwing an exception. Any preferences?
In this case, logging is probably OK since the client may not be getting the behavior they are asking for. However, you either need to change the spec or remove the additional constants from the code. Mylar just fixed all their uses of this code but are still not to spec since they are using SWT.TOP as one of the arguments. I think we need to decide now whether SWT.TOP is valid or not. If it is, add it to the spec and, if it isn't, treat it like any other invalid parameter.
IMO, GridDataFactory/GridLayoutFactory should support all the constants that GridData/GridLayout support - in order to permit copying values from externally-initialized GridData/GridLayout objects. However, the JavaDoc should guide the user toward writing consistent code. I suggest that we should JavaDoc these constants as valid but not recommended.
As a "side effect" of the new asserts in GridDataFactory.align(), it is no longer possible to specify that either only the horizontal or only the vertical align is to be changed, while the other should use a default value, such as in align(SWT.END, SWT.DEFAULT). I don't know if using SWT.DEFAULT as a parameter to align() was intentional; however, it was a convenient way to keep a default value for a parameter which shouldn't be modified in a call to align().
> I don't know if using SWT.DEFAULT as a parameter to align() was > intentional; however, it was a convenient way to keep a default > value for a parameter which shouldn't be modified in a call to align(). This didn't do what you thought it did. Calling align(SWT.END, SWT.DEFAULT) would have set the vertical alignment to SWT.DEFAULT, which is not permitted by SWT. It would not have kept the previous value (check the previous version of the code in CVS if you don't believe me). However, this was a very common programming error (many programmers either assumed that SWT.DEFAULT was valid or mixed up an argument intended for hint(...)). Helping people catch this sort of problem was one of the reasons for the new asserts. BTW, Boris... how goes the rest of the code inspection?
(In reply to comment #12) > [..] > (check the previous version of the code in CVS if you don't believe me). > [..] Why shouldn't I? ;) Thx for pointing this out; in fact I think I mixed up the arguments. However, wouldn't SWT.DEFAULT make sense as an argument to align() also? The way it is now does not allow for telling GridDataFactory.align to leave a particular value at its default while changing another one. Just as a suggestion; if this doesn't make sense or if there are other reasons against this, don't hesitate to tell me. :)
> However, wouldn't SWT.DEFAULT make sense as an argument to align() also? Perhaps, but this would prevent SWT.DEFAULT from ever being used as a hint (SWT doesn't allow it now, but they might in the future). Personally, I like the fact that you always need to set both alignments at once, since it makes the code more readable - as long as you're only calling fillDefaults() and not swtDefaults(), you can always tell at a glance what the alignment is... but I'm always happy to go with the majority on stylistic stuff like this. Anyway, we're a bit off topic here. If you'd like a way to set only one alignment at a time, I'd suggest filing a new bug for the enhancement.
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. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. 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. -- The automated Eclipse Genie.
