Skip to content

Commit 9bfa03f

Browse files
vlaknvlakn
committed
Fixed Topic copy with children
When creating a Topic from another ITopic also create copies of the topics children and bind these to the newly created topic instead of moving the children from the original topic to new newly created one. This fixes the issue of not returning the right parent when calling getChildren() for a child topic. Contributes to #525
1 parent a329aa5 commit 9bfa03f

File tree

4 files changed

+54
-38
lines changed

4 files changed

+54
-38
lines changed

‎ua/org.eclipse.help/src/org/eclipse/help/internal/Topic.java‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,13 @@
1313
*******************************************************************************/
1414
package org.eclipse.help.internal;
1515

16+
import java.util.ArrayList;
17+
import java.util.List;
18+
1619
import org.eclipse.help.ICriteria;
1720
import org.eclipse.help.ITopic;
1821
import org.eclipse.help.ITopic2;
22+
import org.eclipse.help.IUAElement;
1923
import org.w3c.dom.Element;
2024

2125
public class Topic extends UAElement implements ITopic2 {
@@ -24,7 +28,7 @@ public class Topic extends UAElement implements ITopic2 {
2428
public static final String ATTRIBUTE_HREF = "href"; //$NON-NLS-1$
2529
public static final String ATTRIBUTE_LABEL = "label"; //$NON-NLS-1$
2630
public static final String ATTRIBUTE_ICON = "icon"; //$NON-NLS-1$
27-
public static final String ATTRIBUTE_SORT= "sort"; //$NON-NLS-1$
31+
public static final String ATTRIBUTE_SORT = "sort"; //$NON-NLS-1$
2832

2933
public Topic() {
3034
super(NAME);
@@ -34,16 +38,21 @@ public Topic(ITopic src) {
3438
super(NAME, src);
3539
setHref(src.getHref());
3640
setLabel(src.getLabel());
37-
appendChildren(src.getChildren());
41+
List<IUAElement> copiedchildren = new ArrayList<>();
42+
for (IUAElement child : src.getChildren()) {
43+
UAElement newElement = UAElementFactory.newElement(child);
44+
copiedchildren.add(newElement);
45+
}
46+
appendChildren(copiedchildren.toArray(IUAElement[]::new));
3847
}
3948

4049
@Override
41-
public String getIcon(){
50+
public String getIcon() {
4251
return getAttribute(ATTRIBUTE_ICON);
4352
}
4453

4554
@Override
46-
public boolean isSorted(){
55+
public boolean isSorted() {
4756
return "true".equalsIgnoreCase(getAttribute(ATTRIBUTE_SORT)); //$NON-NLS-1$
4857
}
4958

‎ua/org.eclipse.help/src/org/eclipse/help/internal/UAElement.java‎

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import org.w3c.dom.Document;
3535
import org.w3c.dom.Element;
3636
import org.w3c.dom.Node;
37+
3738
/*
3839
* Base class for UA model elements.
3940
*/
@@ -59,6 +60,7 @@ public Filter(String name, String value, boolean isNegated) {
5960
this.value = value;
6061
this.isNegated = isNegated;
6162
}
63+
6264
String name;
6365
String value;
6466
boolean isNegated;
@@ -81,8 +83,13 @@ public UAElement(String name, IUAElement src) {
8183
}
8284
}
8385

86+
public UAElement(UAElement src) {
87+
this(src.getElementName());
88+
copyFilters(src);
89+
}
90+
8491
private void copyFilters(IUAElement src) {
85-
UAElement sourceElement = (UAElement)src;
92+
UAElement sourceElement = (UAElement) src;
8693
String filter = sourceElement.getAttribute(ATTRIBUTE_FILTER);
8794
if (filter != null && filter.length() > 0) {
8895
this.setAttribute(ATTRIBUTE_FILTER, filter);
@@ -101,15 +108,14 @@ private Filter[] getFilterElements() {
101108
if (node.getNodeType() == Node.ELEMENT_NODE) {
102109
String elementKind = node.getNodeName();
103110
if (ExpressionTagNames.ENABLEMENT.equals(elementKind)) {
104-
Element enablement = (Element)node;
111+
Element enablement = (Element) node;
105112
try {
106113
enablementExpression = ExpressionConverter.getDefault().perform(enablement);
107-
}
108-
catch (CoreException e) {
114+
} catch (CoreException e) {
109115

110116
}
111117
} else if (ELEMENT_FILTER.equals(elementKind)) {
112-
Element filter = (Element)node;
118+
Element filter = (Element) node;
113119
String filterName = filter.getAttribute(ATTRIBUTE_NAME);
114120
String value = filter.getAttribute(ATTRIBUTE_VALUE);
115121
if (filterName.length() > 0 && value.length() > 0) {
@@ -146,14 +152,15 @@ public void appendChildren(IUAElement[] children) {
146152
if (this.children == null && children.length > 0) {
147153
this.children = new ArrayList<>(4);
148154
}
149-
for (int i=0;i<children.length;i++) {
150-
appendChild(children[i] instanceof UAElement ? (UAElement)children[i] : UAElementFactory.newElement(children[i]));
155+
for (IUAElement child : children) {
156+
appendChild(child instanceof UAElement ? (UAElement) child : UAElementFactory.newElement(child));
151157
}
152158
}
153159

154160
/*
155-
* This method is synchronized to fix Bug 232169. When modifying this source be careful not
156-
* to introduce any logic which could possibly cause this thread to block.
161+
* This method is synchronized to fix Bug 232169. When modifying this source
162+
* be careful not to introduce any logic which could possibly cause this
163+
* thread to block.
157164
*/
158165
synchronized public String getAttribute(String name) {
159166
String value = element.getAttribute(name);
@@ -164,9 +171,10 @@ synchronized public String getAttribute(String name) {
164171
}
165172

166173
/*
167-
* This method is synchronized to fix Bug 230037. A review of the code indicated that there was no
168-
* path which could get blocked and cause deadlock. When modifying this source be careful not
169-
* to introduce any logic which could possibly cause this thread to block.
174+
* This method is synchronized to fix Bug 230037. A review of the code
175+
* indicated that there was no path which could get blocked and cause
176+
* deadlock. When modifying this source be careful not to introduce any
177+
* logic which could possibly cause this thread to block.
170178
*/
171179
@Override
172180
public synchronized IUAElement[] getChildren() {
@@ -176,7 +184,7 @@ public synchronized IUAElement[] getChildren() {
176184
Node node = element.getFirstChild();
177185
while (node != null) {
178186
if (node.getNodeType() == Node.ELEMENT_NODE) {
179-
UAElement uaElement = UAElementFactory.newElement((Element)node);
187+
UAElement uaElement = UAElementFactory.newElement((Element) node);
180188
if (uaElement != null) {
181189
uaElement.parent = this;
182190
children.add(uaElement);
@@ -196,8 +204,7 @@ public <T> T[] getChildren(Class<T> clazz) {
196204
IUAElement[] children = getChildren();
197205
if (children.length > 0) {
198206
List<Object> list = new ArrayList<>();
199-
for (int i=0;i<children.length;++i) {
200-
IUAElement child = children[i];
207+
for (IUAElement child : children) {
201208
if (clazz.isAssignableFrom(child.getClass())) {
202209
list.add(child);
203210
}
@@ -256,8 +263,8 @@ public boolean isEnabled(IEvaluationContext context) {
256263
return isEnabledByFilterAttribute(filter);
257264
}
258265
Filter[] filterElements = getFilterElements();
259-
for (int i = 0; i < filterElements.length; i++) {
260-
if (!isFilterEnabled(filterElements[i])) {
266+
for (Filter filterElement : filterElements) {
267+
if (!isFilterEnabled(filterElement)) {
261268
return false;
262269
}
263270
}
@@ -291,12 +298,12 @@ public void setAttribute(String name, String value) {
291298
private void importElement(UAElement uaElementToImport) {
292299
Element elementToImport = uaElementToImport.element;
293300
Document ownerDocument = element.getOwnerDocument();
294-
if (!ownerDocument.equals(elementToImport.getOwnerDocument()) ) {
295-
elementToImport = (Element)ownerDocument.importNode(elementToImport, true);
301+
if (!ownerDocument.equals(elementToImport.getOwnerDocument())) {
302+
elementToImport = (Element) ownerDocument.importNode(elementToImport, true);
296303
uaElementToImport.children = null;
297-
} else {
304+
} else {
298305
if (elementToImport.getParentNode() != null) {
299-
elementToImport = (Element)ownerDocument.importNode(elementToImport, true);
306+
elementToImport = (Element) ownerDocument.importNode(elementToImport, true);
300307
uaElementToImport.children = null;
301308
} else {
302309
}

‎ua/org.eclipse.help/src/org/eclipse/help/internal/UAElementFactory.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ public class UAElementFactory {
7474
{ ICriteria.class, Criteria.class },
7575
{ ICriteriaDefinition.class, CriteriaDefinition.class },
7676
{ ICriterionDefinition.class, CriterionDefinition.class },
77-
{ ICriterionValueDefinition.class, CriterionValueDefinition.class },
77+
{ ICriterionValueDefinition.class, CriterionValueDefinition.class }
7878
};
7979

8080
private static final Map<String, Class<?>> classByElementName;
@@ -119,9 +119,9 @@ public static UAElement newElement(Element element) {
119119
}
120120

121121
public static UAElement newElement(IUAElement src) {
122-
for (int i=0;i<interfaceTable.length;++i) {
123-
Class<?> interfaze = interfaceTable[i][0];
124-
Class<?> clazz = interfaceTable[i][1];
122+
for (Class<?>[] element : interfaceTable) {
123+
Class<?> interfaze = element[0];
124+
Class<?> clazz = element[1];
125125
if (interfaze.isAssignableFrom(src.getClass())) {
126126
try {
127127
Constructor<?> constructor = clazz.getConstructor(interfaze);
@@ -132,6 +132,9 @@ public static UAElement newElement(IUAElement src) {
132132
ILog.of(UAContentFilter.class).error(msg, e);
133133
}
134134
}
135+
if(src instanceof UAElement) {
136+
return new UAElement((UAElement) src);
137+
}
135138
}
136139
return null;
137140
}

‎ua/org.eclipse.ua.tests/help/org/eclipse/ua/tests/help/other/TopicTest.java‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,7 @@ public void testCopyTopicWithChild() {
129129
assertThat(topic2.getSubtopics()).hasSize(1);
130130
}
131131

132-
/*
133-
* Disabled, see Bug 210024 [Help] Topic element problems constructing from an ITopic
132+
@Test
134133
public void testCopyTopicWithChildRemoveChild() {
135134
Topic topic1;
136135
topic1 = createTopic(TOPIC_WITH_CHILD);
@@ -142,14 +141,12 @@ public void testCopyTopicWithChildRemoveChild() {
142141
assertEquals(0, topic1.getSubtopics().length);
143142
assertEquals(1, topic2.getSubtopics().length);
144143
}
145-
*/
146144

147145
/*
148146
* Test the assumption that when a topic is created from another topic not only
149147
* the topic but all the children are recursively copied
150148
*/
151-
/*
152-
* Disabled, see Bug 210024 [Help] Topic element problems constructing from an ITopic
149+
@Test
153150
public void testCopyTopicWithChildCheckParents() {
154151
Topic topic1;
155152
topic1 = createTopic(TOPIC_WITH_CHILD);
@@ -158,14 +155,14 @@ public void testCopyTopicWithChildCheckParents() {
158155
assertEquals(ECLIPSE_HREF, topic1.getHref());
159156
assertEquals(1, topic1.getSubtopics().length);
160157
Topic child1 = (Topic)topic1.getSubtopics()[0];
161-
assertTrue(child1.getParentElement() == topic1);
158+
assertEquals(child1.getParentElement(), topic1);
162159
assertEquals(ECLIPSE, topic2.getLabel());
163160
assertEquals(ECLIPSE_HREF, topic2.getHref());
164161
assertEquals(1, topic2.getSubtopics().length);
165-
Topic child2 = (Topic)topic1.getSubtopics()[0];
166-
assertTrue(child2.getParentElement() == topic2);
162+
Topic child2 = (Topic) topic2.getSubtopics()[0];
163+
assertEquals(child2.getParentElement(), topic2);
167164
}
168-
*/
165+
169166
@Test
170167
public void testEnabledTopic() {
171168
Topic topic = createTopic(TOPIC_WITH_ENABLEMENT);

0 commit comments

Comments
 (0)