Skip to content

Commit 4c400c2

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 4c400c2

4 files changed

Lines changed: 46 additions & 38 deletions

File tree

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

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

16+
import java.util.Arrays;
17+
1618
import org.eclipse.help.ICriteria;
1719
import org.eclipse.help.ITopic;
1820
import org.eclipse.help.ITopic2;
21+
import org.eclipse.help.IUAElement;
1922
import org.w3c.dom.Element;
2023

2124
public class Topic extends UAElement implements ITopic2 {
@@ -24,7 +27,7 @@ public class Topic extends UAElement implements ITopic2 {
2427
public static final String ATTRIBUTE_HREF = "href"; //$NON-NLS-1$
2528
public static final String ATTRIBUTE_LABEL = "label"; //$NON-NLS-1$
2629
public static final String ATTRIBUTE_ICON = "icon"; //$NON-NLS-1$
27-
public static final String ATTRIBUTE_SORT= "sort"; //$NON-NLS-1$
30+
public static final String ATTRIBUTE_SORT = "sort"; //$NON-NLS-1$
2831

2932
public Topic() {
3033
super(NAME);
@@ -34,16 +37,19 @@ public Topic(ITopic src) {
3437
super(NAME, src);
3538
setHref(src.getHref());
3639
setLabel(src.getLabel());
37-
appendChildren(src.getChildren());
40+
IUAElement[] copiedChildren = Arrays.stream(src.getChildren()).map(UAElementFactory::newElement)
41+
.toArray(IUAElement[]::new);
42+
43+
appendChildren(copiedChildren);
3844
}
3945

4046
@Override
41-
public String getIcon(){
47+
public String getIcon() {
4248
return getAttribute(ATTRIBUTE_ICON);
4349
}
4450

4551
@Override
46-
public boolean isSorted(){
52+
public boolean isSorted() {
4753
return "true".equalsIgnoreCase(getAttribute(ATTRIBUTE_SORT)); //$NON-NLS-1$
4854
}
4955

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

Lines changed: 23 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;
@@ -82,7 +84,7 @@ public UAElement(String name, IUAElement src) {
8284
}
8385

8486
private void copyFilters(IUAElement src) {
85-
UAElement sourceElement = (UAElement)src;
87+
UAElement sourceElement = (UAElement) src;
8688
String filter = sourceElement.getAttribute(ATTRIBUTE_FILTER);
8789
if (filter != null && filter.length() > 0) {
8890
this.setAttribute(ATTRIBUTE_FILTER, filter);
@@ -101,15 +103,14 @@ private Filter[] getFilterElements() {
101103
if (node.getNodeType() == Node.ELEMENT_NODE) {
102104
String elementKind = node.getNodeName();
103105
if (ExpressionTagNames.ENABLEMENT.equals(elementKind)) {
104-
Element enablement = (Element)node;
106+
Element enablement = (Element) node;
105107
try {
106108
enablementExpression = ExpressionConverter.getDefault().perform(enablement);
107-
}
108-
catch (CoreException e) {
109+
} catch (CoreException e) {
109110

110111
}
111112
} else if (ELEMENT_FILTER.equals(elementKind)) {
112-
Element filter = (Element)node;
113+
Element filter = (Element) node;
113114
String filterName = filter.getAttribute(ATTRIBUTE_NAME);
114115
String value = filter.getAttribute(ATTRIBUTE_VALUE);
115116
if (filterName.length() > 0 && value.length() > 0) {
@@ -146,14 +147,15 @@ public void appendChildren(IUAElement[] children) {
146147
if (this.children == null && children.length > 0) {
147148
this.children = new ArrayList<>(4);
148149
}
149-
for (int i=0;i<children.length;i++) {
150-
appendChild(children[i] instanceof UAElement ? (UAElement)children[i] : UAElementFactory.newElement(children[i]));
150+
for (IUAElement child : children) {
151+
appendChild(child instanceof UAElement ? (UAElement) child : UAElementFactory.newElement(child));
151152
}
152153
}
153154

154155
/*
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.
156+
* This method is synchronized to fix Bug 232169. When modifying this source
157+
* be careful not to introduce any logic which could possibly cause this
158+
* thread to block.
157159
*/
158160
synchronized public String getAttribute(String name) {
159161
String value = element.getAttribute(name);
@@ -164,9 +166,10 @@ synchronized public String getAttribute(String name) {
164166
}
165167

166168
/*
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.
169+
* This method is synchronized to fix Bug 230037. A review of the code
170+
* indicated that there was no path which could get blocked and cause
171+
* deadlock. When modifying this source be careful not to introduce any
172+
* logic which could possibly cause this thread to block.
170173
*/
171174
@Override
172175
public synchronized IUAElement[] getChildren() {
@@ -176,7 +179,7 @@ public synchronized IUAElement[] getChildren() {
176179
Node node = element.getFirstChild();
177180
while (node != null) {
178181
if (node.getNodeType() == Node.ELEMENT_NODE) {
179-
UAElement uaElement = UAElementFactory.newElement((Element)node);
182+
UAElement uaElement = UAElementFactory.newElement((Element) node);
180183
if (uaElement != null) {
181184
uaElement.parent = this;
182185
children.add(uaElement);
@@ -196,8 +199,7 @@ public <T> T[] getChildren(Class<T> clazz) {
196199
IUAElement[] children = getChildren();
197200
if (children.length > 0) {
198201
List<Object> list = new ArrayList<>();
199-
for (int i=0;i<children.length;++i) {
200-
IUAElement child = children[i];
202+
for (IUAElement child : children) {
201203
if (clazz.isAssignableFrom(child.getClass())) {
202204
list.add(child);
203205
}
@@ -256,8 +258,8 @@ public boolean isEnabled(IEvaluationContext context) {
256258
return isEnabledByFilterAttribute(filter);
257259
}
258260
Filter[] filterElements = getFilterElements();
259-
for (int i = 0; i < filterElements.length; i++) {
260-
if (!isFilterEnabled(filterElements[i])) {
261+
for (Filter filterElement : filterElements) {
262+
if (!isFilterEnabled(filterElement)) {
261263
return false;
262264
}
263265
}
@@ -291,12 +293,12 @@ public void setAttribute(String name, String value) {
291293
private void importElement(UAElement uaElementToImport) {
292294
Element elementToImport = uaElementToImport.element;
293295
Document ownerDocument = element.getOwnerDocument();
294-
if (!ownerDocument.equals(elementToImport.getOwnerDocument()) ) {
295-
elementToImport = (Element)ownerDocument.importNode(elementToImport, true);
296+
if (!ownerDocument.equals(elementToImport.getOwnerDocument())) {
297+
elementToImport = (Element) ownerDocument.importNode(elementToImport, true);
296298
uaElementToImport.children = null;
297-
} else {
299+
} else {
298300
if (elementToImport.getParentNode() != null) {
299-
elementToImport = (Element)ownerDocument.importNode(elementToImport, true);
301+
elementToImport = (Element) ownerDocument.importNode(elementToImport, true);
300302
uaElementToImport.children = null;
301303
} else {
302304
}

‎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 uaElement) {
136+
return new UAElement(uaElement.getElementName(), uaElement);
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)