Community
Participate
Working Groups
Michael Valenta has seen this problem on my machine. It appears that if you create a project patch in the past, the UI remembers the project setting and tried to use even for a workspace patch on multiple projects. The project option is selected by greyed out & the workspace option is available but not selected.
This is easily reproducable but not a stop ship for 3.3 so we'll look at it in 3.3.1.
Created attachment 71920 [details] A patch The patch should fix current and some future problems related to the selection and enablement of radio buttons group. AFAIK there is no way to make buttons mutually exclusive using standard SWT API. My question is: Do we have any other solution to that problem: enforcing single selection among radio buttons (eg. some custom class for grouping radio buttons)?. I'm sure it's not a new problem. I can write such class if you wish :)
I've looked at the patch and the part that fixes the problem reported in this bug is fine but the part that tries to adjust the state when Selection is not an option seems to leave two items enabled. I guess this is what you are talking about in your previous comment. I think that the buttons are mutually exclusive when clicked by the user but not when programmatically set. Providing a class to ensure that mutual exclusion is maintained when setting programmatically makes sense to me. I await your next patch.
Created attachment 72163 [details] Patch v2 This patch contains a private Button2 class which replaces SWT Button used for radio buttons in the dialog.
Created attachment 72184 [details] Patch v3 Patch with a grouping class. I must admit that it wasn't so trivial as I expected it to be. That's why the class can look a bit complicated at a first look. Michael, I'm still opened for your comments :)
The main problem I have with the patch is that it only contains some of the group related behavior. For instance, on line 1111 and below, there are a bunch of selection listeners that set the selectedRoot and selectedFormat. I think it would be cleaner for this to be tracked in each group (i.e. the selectionListener should just inform the appropriate group what the selection should be changed to. Basically, anywhere where the selection of a radio button is changed should be changed to make a call to the group to manage the selection. This will localize the selection handling into the group and make the code easier to read.
Created attachment 72379 [details] Patch v4 I tried to implement your suggestions Michael. But to be honest with you, I started to think that a grouping class wasn't the best idea. It will always look more complicated comparing to some additional if-clauses inside updateEnablements. On the other hand I cannot forget it was my proposition to introduce this extra class :)
I not convinced of that. I think the problem is that we need to find the right balance of what goes into the group and what remains separate. I would like to see the following improvements: 1) I think the selectedFormat and selectedRoot instance variables should be moved into the radio groups. 2) In updateRadioButtons() the calls to the setSelections of each button should be a single call to the group which will select the appropriate button and de-select the others. Your setSeleciton appears to do this already but you still have the extra setSelections in updateRadioButtons. 3) In the setSelection of the radio button group, you do a deselectAll and then select one. I would be concerned about this causing an event loop when called from the selection events. PErhaps you need another method that gets called when the user selects the button (i.e. one that just updates the instance variable for point 1) 4) One of the most complicated aspects of this is the button enablement management. Would it help to add a method that allows you to set the enabled buttons and pick the button to select if the currently selected button becomes disabled. Some thing like setEnabled(new int[], int defaultSelection). This would make the code at the start of updateEnablements look like if (!patchHasCommonRoot){ unifiedRadioGroup.setEnabled(new int[] { ROOT_WORKSPACE }, ROOT_WORKSPACE); diffTypeRadioGroup.setEnabled(new int[] { FORMAT_UNIFIED }, FORMAT_UNIFIED); } What do you think?
Created attachment 72698 [details] Patch v5 I hope this time you will like it better :)
Created attachment 72700 [details] Accompanying test suite
Patch released to HEAD. I'll release it to 3.3.1 once Europa ships.
Patch released to 3.3.1.
REOPENED I prepared following test: 2 projects A i B with modified files. I created path for Project A, then for B and everything was OK. But the I created patch for workspace, the header of the patch was correct, but only project was included into patch, the second one was ignored.
Verified on M20070822-0800 and 3.4M1 . I added new bug 201714 for christoph's issue.
