Community
Participate
Working Groups
Build id: I20090603-2000 1. Create a class like test.Test. 2. Create a second class test2.Test. 3. In a third class, write something like 'Test t'. 4. Invoke Ctrl+Shift+O to organize imports. 5. Since there are two classes named 'Test', a dialog should show up offering the two suggestions. Notice that the dialog's buttons are lined up as '< Back / Finish / Cancel / Next >'. I presume this is a problem on the Mac as well.
Test Case: run org.eclipse.jdt.ui.examples.MultiElementListSelectorExample. Looks OK on WindowsXP. Can reproduce on Linux-GTK. I wonder how many dialogs are now broken due to the "fix" for bug 57762. One other example (that got fixed) of such a breakage is e.g. bug 270174.
Every subclass of org.eclipse.jface.dialogs.Dialog that defines a default button which is not a dismissal button is broken. It can also happen that Yes/No dialogs get non-standard positions if the 'No' button is set to be the default button. This is something we need to fix in the Platform for 3.5.
Created attachment 138387 [details] Too big fix (has whitespace changes) Instead of the shell's default button the fix uses one of the known dismissal buttons if available. Otherwise the current behavior is kept. MessageDialog behavior is not touched as it breaks the API of Dialog when it comes to button IDs and getButton(int), see bug 279235. To keep the fix minimal it does revert the now obsolete changes made for bug 270174. I'll attach a separate optional patch for that.
Created attachment 138388 [details] Fix
Created attachment 138390 [details] Optional patch to remove obsolete code in WizardDialog
Tested on WindowsXP and Linux-GTK. Markus can you check the fix on Mac?
Looks good on the Mac. +1 for the patch for RC4. In 3.6, the dismissal button could be made more explicit, e.g. by making something like findDefaultDismissalButton() API.
Created attachment 138435 [details] patch with try-catch After discussing with Markus, Dani, and McQ we agreed that we should add a try/catch to safeguard against subclasses that don't honor the API contract for getButton().
+1 for "patch with try-catch". Tested again on Mac and Win. Filed bug 279297 for 3.6.
-1 I went over the patch with Boris in his office and was not convinced that the out come would be better in all cases. We search for known default buttons in a good order and move them over. In the past, someone could have been setting a button to be the default and expecting that to be moved over. It *probably* works in most/some of the cases and might not be harmful. At this point in the release though, since it is not a crash or loss of data or a UI blooper that makes Eclipse unusable, I can't vote for it. Here is another argument: It was broken on the Mac in Eclipse 3.4. At that point in time, the Mac was the only platform that had the dismissal alignment on the right. We fixed the dismissal alignment for GTK in 3.5, bringing it up to par with the implementation on the Mac (bugs and all). In a future version of Eclipse, possibly 3.6, we will improve support for dismissal alignment by adding API and/or applying the patch in this bug report. This will give the community and the developers a long enough period of time to test out the solution on multiple different dialogs and subclasses.
For me the current solution is not acceptable: it can result in very weird layouts. If the fix is -1 (which I can accept) then we must pull this button shifting hack entirely for 3.5 and work on a correct solution during 3.6. For me this is a stop ship until we either fix as outlined in this bug or revert the "fix" for bug 57762.
The patch on this bug is not going to go in. A PMC member has -1'ed it. Reverting the other fix will cause GTK to behave like it did in 3.4, but Cocoa to *not* behave like Carbon did in 3.4, so from my pov that is not a reasonable solution either. At this point, I believe the best answer is to just _not_touch_this_stuff_ any more until R3.5.1.
I'll give my two cents on this bug since I was the one that "fixed" bug 57762. To be honest, when I filed this bug yesterday, I never would've thought we would be pushing for a fix in 3.5.0. (In reply to comment #10) > It was broken on the Mac in Eclipse 3.4. Based on CVS annotations, it seems that the "moving" code was introduced into the Dialog class years ago when Nick fixed bug 22621. This alignment problem has been in Eclipse for over half a decade and it seems no one has ever complained to JDT about the weird button ordering of this particular dialog. I don't think there's any question about a B/F/C/N button ordering being completely wrong but if you look at the class's CVS annotations, you will realize that this particular block of button creation code has actually not changed at all since it was first committed into CVS in 2001. Whether it is a good or bad reputation, we all know how stingy Mac users are about their user interfaces. However, it seems that they don't really care about this dialog, or any other Eclipse dialog that happens to set the default button to _not_ be a dismissal Yes/Ok button. Wizards appears to be ignored by Mac users also. No one seems to have ever complained about the problem described by bug 270174 until I noticed it myself. This button ordering apathy seems to extend to Linux users too though. Since the beginning of Eclipse, we've always lived with Ok/Cancel. Bug 57762 and its two duplicate has sat there for years with little public pressure for a fix. Heck, it doesn't even have a single vote. Looking back, I was just trying to be helpful and reduce Susan's backlog of bugs but with 20/20 hindsight now, maybe I should've just left that bug alone to rot. (In reply to comment #11) > For me the current solution is not acceptable: it can result in very weird > layouts. The users that will notice this change are Linux users that are jumping onto 3.5. Mac users are not really in this category because they've "been broken for a while (presumably since Eclipse 2.1)". This layout problem is not something that is terribly difficult to workaround. The simplest method would be to just wrap the lone default button in another Composite so that moveBelow(Control) will do nothing. As for the button ordering itself, to look on the bright side in the face of this ugly problem, this particular developer will now become aware of the concept of dismissal alignment and that something as subtle as button ordering on a dialog window can differ on different platforms. He or she can apply and share this knowledge with others to help construct native looking dialogs with SWT's and JFace's APIs (where their dialog's button's orderings may have previously been completely hardcoded). > For me this is a stop ship until we either fix as outlined in this bug or > revert the "fix" for bug 57762. Originally, I would've been for "do nothing" or "revert" for 3.5.0. However, McQ's comment 12 remark about Cocoa has me leaning towards the former. I sincerely apologize for having caused so much problems for our downstream consumers of this particular API. I will take this lesson to heart and forever remind myself that even a small drop of water can cause ripples in a distant ocean.
> just _not_touch_this_stuff_ any more until R3.5.1. I agree. We should not throw out the baby with the bath water and revert bug 57762 just because we now found 1 glitch that has not been detected for years before with the same code on the Mac. (In reply to comment #13) > Bug 57762 [..] Heck, it doesn't even have a single vote. I think it had some votes before it was fixed, but Bugzilla doesn't show vote count history. > maybe I should've just left that bug alone to rot. Nah, I still think it's good that you got this ball rolling. It's unfortunate that none of us involved in the previous fixes dug deep enough for the full solution until now (I have to admit I had a gut feeling that moving the default button could not be the complete solution, but I also didn't try to find or construct a counterexample). We will be a much better platform citizen on GTK for 3.5, and we can address this bug in 3.5.1 and apply the full fix in 3.6.
>At this point, I believe the best answer is to just _not_touch_this_stuff_ any >more until R3.5.1. OK, point taken. I let's go with this for 3.5. Could you agree to a local simple fix in our internal JDT dialog for RC5?
I'd be happiest if we weren't making any more code changes, and I find it hard to convince myself that changing the button order would be something that is a stop ship (which is the only reason we would be doing it during RC5). If you feel strongly about it, you can enter a new bug and add Steve and I to the CC so we can discuss. Experience has shown Steve is even less willing to make late changes than I am though.
>I'd be happiest if we weren't making any more code changes, OK, it's really late in the cycle. I filed bug 279425 to fix the known issue in JDT's MultiElementListSelectionDialog for 3.5.1 as correctly fixing this bug here will require new API and is probably not in scope for 3.5.1.
(mass update) Removing target milestone, this bug was targeted at a milestone in the past.
Marking 3.6 since it will require new API.
Marking as WONTFIX as we decided to do nothing for 3.5 and the correct fix will be covered by bug 279297.
