Bug 92518

Comment 1 ]-[appy 2005-06-24 04:59:22 EDT

Created attachment 23930 [details]
patch to remove section bug

little patch to remove section

Comment 2 Mykola Nikishov 2008-11-28 05:38:29 EST

ping

Comment 3 Susan McCourt 2009-07-09 15:56:28 EDT

As per http://wiki.eclipse.org/Platform_UI/Bug_Triage_Change_2009

Comment 4 Rüdiger Herrmann 2013-01-28 07:53:12 EST

What would be necessary to make this change happen? Or are there objections against the enhancement request itself?

Comment 5 Paul Webster 2013-01-28 11:20:19 EST

I had a quick look, IDialogSettings is API and doesn't specify @noimplement, so we can't add methods to it.

But we could consider adding the method to the concrete class, DialogSettings.  I'd need a re-worked patch with the new method and a test or two in http://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/tests/org.eclipse.ui.tests/Eclipse%20JFace%20Tests/org/eclipse/jface/tests/dialogs/DialogSettingsTest.java

PW

Comment 6 Rüdiger Herrmann 2013-01-29 10:40:35 EST

Created attachment 226265 [details]
Proposed fix

Introduces method removeSection(IDialogSettings) in DialogSettings. Attempting to remove a unrelated section is ignored, passing null (implicitly) causes a NPE.

Please let me know if there is anything missing.

Comment 7 Paul Webster 2013-02-05 16:23:39 EST

Released as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=771a38806e8efa85e0b3f46e38eab1f41a3e81ad

Thanx  Rüdiger

PW

Comment 8 Markus Keller 2013-02-06 09:13:44 EST

Why is the API "void removeSection(IDialogSettings)" and not like this?

	public IDialogSettings removeSection(String sectionName) {
		return (IDialogSettings) sections.remove(sectionName);
	}

This API would also work in cases where you're not sure if the section already exists, and it even tells you whether the section existed or not.

Comment 9 Paul Webster 2013-02-06 09:37:15 EST

(In reply to comment #8)
> Why is the API "void removeSection(IDialogSettings)" and not like this?
> 
> 	public IDialogSettings removeSection(String sectionName) {
> 		return (IDialogSettings) sections.remove(sectionName);
> 	}
> 

The addNewSection took a String, but addSection takes an IDialogsSettings and the remove was the mirror of that method.  But I like the above suggestion to use a String and return the removed section.

Rüdiger if you have no objections I'll make the modification.

PW

Comment 10 Rüdiger Herrmann 2013-02-06 10:02:24 EST

(In reply to comment #9)
> (In reply to comment #8)
> > Why is the API "void removeSection(IDialogSettings)" and not like this?
> > 
> > 	public IDialogSettings removeSection(String sectionName) {
> > 		return (IDialogSettings) sections.remove(sectionName);
> > 	}
> > 
> 
> The addNewSection took a String, but addSection takes an IDialogsSettings
> and the remove was the mirror of that method.  But I like the above
> suggestion to use a String and return the removed section.
> 
> Rüdiger if you have no objections I'll make the modification.
> 
> PW
I like the current form for its symmetry and the client code I had in mind already holds a reference to the IDialogSettings to be removed.
But, all in all, I have no real strong opinion on that matter.

Comment 11 Paul Webster 2013-02-06 10:46:26 EST

(In reply to comment #10)
> I like the current form for its symmetry and the client code I had in mind
> already holds a reference to the IDialogSettings to be removed.
> But, all in all, I have no real strong opinion on that matter.

How about I add the second method, and I'll add a slight rider to the current method that the DialogSettings must contain the one to be removed?

PW

Comment 12 Paul Webster 2013-02-06 13:14:16 EST

Added the second method.
http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=05f26c2cbba44a20fc68af98fe25be755732c0ff

PW

Comment 13 Paul Webster 2013-03-12 10:53:46 EDT

In 4.3.0.I20130311-2000
PW

Comment 14 Paul Webster 2013-03-12 11:40:49 EDT

.