[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 I've created PR #310 so it's easier to pick the patch https://github.com/apache/jmeter/pull/310 ---
[GitHub] jmeter pull request #310: Bug 57039 - Inconsistency with the undo/redo log
GitHub user emilianbold opened a pull request: https://github.com/apache/jmeter/pull/310 Bug 57039 - Inconsistency with the undo/redo log Doing another PR because the previous PR can't provide the patch anymore. See https://github.com/apache/jmeter/pull/300#issuecomment-329994413 You can merge this pull request into a Git repository by running: $ git pull https://github.com/emilianbold/jmeter-contributions emilianbold-undoredo Alternatively you can review and apply these changes as the patch at: https://github.com/apache/jmeter/pull/310.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #310 commit ea9859995d1fb53905b720bc57a1ddf283b29ce8 Author: Emilian Bold Date: 2017-07-22T11:17:41Z Bug 57039 - Inconsistency with the undo/redo log Bugzilla Id: 57039 commit 401221e9f6e480e43fcf51ee99458a0d7fdd661d Author: Emilian Bold Date: 2017-07-22T12:22:08Z Use javax.swing.undo classes in UndoHistory, introduce undo transactions to group edits commit 3a06e2e0080f8c72c1ee67f238e6be7eedcca945 Author: Emilian Bold Date: 2017-07-22T21:29:03Z Avoid undo transaction on File | New and open actions commit 45bd1a718f73a5bc58fcc614d3c51377f941c815 Author: Emilian Bold Date: 2017-07-23T14:14:50Z TreeState is part of the undoable edit commit 0115fdd0d55e8ea4d9e27e9f791c162192daa771 Author: Emilian Bold Date: 2017-07-23T19:11:58Z Add dirty state to the undoable edit. Reload nodes in Checkdirty after an undo/redo ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 I've pushed the branch again on https://github.com/emilianbold/jmeter/commits/emilianbold-undoredo Let me know if it's working for you. ---
[GitHub] jmeter pull request #299: Mavenization
Github user emilianbold closed the pull request at: https://github.com/apache/jmeter/pull/299 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 > IMO it is better to merge it even before 3.3. It will not do much harm because most of users have the feature disabled by default. Merging would simplify further reviews and improvements, I'd favor developer's needs here. Making undo flawless from the first pull request is impossible. But if I define the base layer and some common patterns then each new undo issue/feature will be easier to fix/add by me or somebody else. Feel free to skip this issue until after 3.3, I am trying to reduce my open-source time in August. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter pull request #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/300#discussion_r131213400
--- Diff: src/core/org/apache/jmeter/gui/UndoHistory.java ---
@@ -160,63 +133,46 @@ public void add(JMeterTreeModel treeModel, String
comment) {
// first clone to not convert original tree
tree = (HashTree) tree.getTree(tree.getArray()[0]).clone();
-position++;
-while (history.size() > position) {
-if (log.isDebugEnabled()) {
-log.debug("Removing further record, position: {}, size:
{}", position, history.size());
-}
-history.remove(history.size() - 1);
-}
-
// cloning is required because we need to immute stored data
HashTree copy = UndoCommand.convertAndCloneSubTree(tree);
-history.add(new UndoHistoryItem(copy, comment));
+GuiPackage guiPackage = GuiPackage.getInstance();
+//or maybe a Boolean?
+boolean dirty = guiPackage != null ? guiPackage.isDirty() : false;
--- End diff --
The fact that we have the `CheckDirty` class and `GuiPackage.isDirty` makes
me think there's room for bugs. I haven't really tried to fix the underlying
design, just build upon it to see if I can also undo/redo the dirty flag.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 > I think I can reproduce Philippes problem. And it will probably look really strange to a user. Currently undo stores the model changes, not every UI change. There are some other hooks to add for that. And it's not necessarily an easy thing to add, depending what kind of state hides behind that UI component. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter pull request #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/300#discussion_r131212163
--- Diff: src/core/org/apache/jmeter/gui/action/ActionRouter.java ---
@@ -67,6 +72,9 @@ public void actionPerformed(final ActionEvent e) {
private void performAction(final ActionEvent e) {
String actionCommand = e.getActionCommand();
+if(!NO_TRANSACTION_ACTIONS.contains(actionCommand)) {
--- End diff --
Yes, it would.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jmeter pull request #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/300#discussion_r131212090
--- Diff: src/core/org/apache/jmeter/gui/UndoHistoryItem.java ---
@@ -35,31 +36,48 @@
private final HashTree tree;
// TODO: find a way to show this comment in menu item and toolbar
tooltip
private final String comment;
+private final TreeState treeState;
+private final boolean dirty;
/**
* This constructor is for Unit test purposes only
* @deprecated DO NOT USE
*/
@Deprecated
public UndoHistoryItem() {
-tree = null;
-comment = null;
+this(null, null, null, false);
}
/**
* @param copy HashTree
* @param acomment String
*/
-public UndoHistoryItem(HashTree copy, String acomment) {
+public UndoHistoryItem(HashTree copy, String acomment, TreeState
treeState, boolean dirty) {
tree = copy;
comment = acomment;
--- End diff --
I generally don't beautify the code, but try to keep the patch minimal.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jmeter pull request #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/300#discussion_r131211581
--- Diff: src/core/org/apache/jmeter/gui/TreeState.java ---
@@ -0,0 +1,84 @@
+package org.apache.jmeter.gui;
+
+import java.util.ArrayList;
+import java.util.List;
+import javax.swing.JTree;
+
+public interface TreeState {
+
+/**
+ * Restore tree expanded and selected state
+ *
+ * @param guiInstance GuiPackage to be used
+ */
+void restore(GuiPackage guiInstance);
+
+final static TreeState NOTHING = new TreeState() {
--- End diff --
I know, it just didn't feel like a lambda to me since it holds state.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jmeter pull request #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/300#discussion_r131211065
--- Diff: src/core/org/apache/jmeter/gui/SimpleCompoundEdit.java ---
@@ -0,0 +1,14 @@
+package org.apache.jmeter.gui;
+
+import javax.swing.undo.CompoundEdit;
+
+public class SimpleCompoundEdit extends CompoundEdit {
--- End diff --
I don't see the reason I would encourage serialization here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jmeter pull request #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/300#discussion_r131211056
--- Diff: src/core/org/apache/jmeter/gui/SimpleCompoundEdit.java ---
@@ -0,0 +1,14 @@
+package org.apache.jmeter.gui;
+
+import javax.swing.undo.CompoundEdit;
+
+public class SimpleCompoundEdit extends CompoundEdit {
+
+public boolean isEmpty() {
+return size() == 0;
--- End diff --
To `edits`? Seems a matter of taste. I would prefer to limit access to the
protected variable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jmeter issue #299: Mavenization
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/299 > I think we should wait for 3.3 release , don't you think so ? I have checked to JARs to be identical so moving some files around while preserving the output seems safe. Still, 3.3 is close, so it makes sense to have a code freeze. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #299: Mavenization
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/299 Any update on this? Most of the steps, like "Move resources to src/main/resources" are quite harmless and could be done at any time. It's complicated to work on different branches when I have Maven only on one of them. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 It's not the same bug, but a limitation of the current undo design I've built upon. We basically take a snapshot at the *end* of an action. The snapshot includes the tree data and state. >step 3. Add User Defined Variables, move it before Thread Group 1 >step 4. Unfold TG2 and fold TG1 >step 5. Duplicate User Defined Variables So when you undo, you don't go back in time to right before step 5 in order to see that tree state. You actually go at the end of step 3, and see the tree state *before* step 4. I have some ideas how step 4 might become an undoable edit itself but I'd rather not complicate the patch too much. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue:
https://github.com/apache/jmeter/pull/300
Hm, double-checked now and I still don't see it anymore. Are we looking at
the same code? Note that I rebased at some point and did a force push so
previous patches might not be identical.
Still, the latest commit ("Add dirty state to the undoable edit...")
couldn't have introduced a regression.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 The patches are clean, as far as I understood the JMeter codebase. At minimum, with undo disabled, they should be harmless. >The bug that you previously fixed with TreeState has reappeared. I don't see it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 > Maybe we could change how dirty check works which I think was here because there was no Redo/Undo Management. See the latest commit which patches CheckDirty to be undo aware. We could change how dirty works but at this moment I'd rather use the existing mechanisms. We can replace them with something simpler in a separate fix. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 Actually, dirty check will be more complicated due to CheckDirty which seems to save a list of elements... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 >@emilianbold , do you think computing dirty should be part of the undoable edit ? yes, that's the plan to fix #57040 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 > I think you need to filter also OPEN and OPEN_RECENT I'm updating the commit to exclude more actions. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 > a 900 KB plan Could you email me such a plan to see how slow it is? > You end up with Thread Group 2 expanded and Thread Group 1 unexpanded. >TreeState#restore() seems to be lost in indexes. The commit 'TreeState is part of the undoable edit' fixes this. Doesn't really make sense to restore the tree state on another tree. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 > A third one is the following: [...] > Search something > Redo => Nothing happens => KO setMarkedBySearch is called on JMeterTreeNode not on some model class like TestElement. So, it's never actually saved by the undo/redo mechanism. Undo clears the node because it does a refresh actually, it doesn't save/restore the flag at all. The current design doesn't take this situation into account. Performance could be fixed by coalescing. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 I didn't look into #57040, not sure yet what flag represents 'needs saving'. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 The latest commit fixes the File | New log. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 Ah, yes. It happens because I call begin/endUndoTransaction in ActionRouter and the New action clears the whole undo history (see log line with 'Clearing undo history with 1 unfinished transactions'). So it's a matter of detecting the special New action. The exception is harmless, it's just for logging. --emi On Sat, Jul 22, 2017 at 10:40 PM, Philippe M wrote: > Hello Emilian, > Thanks for this great piece of work ! > First test look very promising in regards of performances. > > I have a first issue to report: > > Open JMeter > Select Menu File > New => An error log appears with the stacktrace (as per > line 310 in UndoHistory) > > â > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub, or mute the thread. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #300: Bug 57039 - Inconsistency with the undo/redo log
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/300 I wanted the PR only for the 'Bug 57039 - Inconsistency with the undo/redo log' commit. But I see GitHub picks up subsequent commits too... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter pull request #300: Bug 57039 - Inconsistency with the undo/redo log
GitHub user emilianbold opened a pull request: https://github.com/apache/jmeter/pull/300 Bug 57039 - Inconsistency with the undo/redo log Bugzilla Id: 57039 You can merge this pull request into a Git repository by running: $ git pull https://github.com/emilianbold/jmeter emilianbold-undoredo Alternatively you can review and apply these changes as the patch at: https://github.com/apache/jmeter/pull/300.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #300 commit ea9859995d1fb53905b720bc57a1ddf283b29ce8 Author: Emilian Bold Date: 2017-07-22T11:17:41Z Bug 57039 - Inconsistency with the undo/redo log Bugzilla Id: 57039 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter pull request #299: Mavenization
GitHub user emilianbold opened a pull request: https://github.com/apache/jmeter/pull/299 Mavenization I'm starting this pull request to get some feedback on the current changes. Adding a Maven-based build system for JMeter implies moving and renaming some files and it would be best if we discuss this before I go too deep with customisations. If you look at my commits, excluding the last one 'Add pom.xml files', they just rearrange files and update the changes in build.xml. So, the plan is to still use ant, except on a Maven friendly file layout. Note how I haven't moved the sources under src/main/java like Maven expects. I'm loading the sources from ./ but I would also like to move them. Each module (src/components, src/core, etc.) would get its own folder (src/components/src/main/java, src/core/src/main/java, etc). As I've mentioned on the mailing list, I would also prefer to have all the modules at the same level. This means moving all the protocol/ftp, protocol/http modules a folder up, next to core and components/. I'm able to load JMeter under Apache NetBeans and IntelliJ IDEA with the new Maven config so I am pleased with this as this is what was bothering me. Replacing ant altogether is a big step but if we do it incrementally like I'm showing here by just preparing the file layout, etc. we can do it with less friction. build.xml is 3600 lines long and can't easily be replaced all at once. I can imagine a transition period where ant will still be used for many targets, perhaps via the maven-antrun-plugin Maven plugin. You can merge this pull request into a Git repository by running: $ git pull https://github.com/emilianbold/jmeter emilianbold-mavenization Alternatively you can review and apply these changes as the patch at: https://github.com/apache/jmeter/pull/299.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #299 commit 085dbe25ce9957095577079dd6f5846b6a044c0e Author: Emilian Bold Date: 2017-07-08T06:35:22Z Move resources to src/main/resources commit 22f43bbfd4798ca2435b749217ee2b417fed7111 Author: Emilian Bold Date: 2017-07-09T16:30:51Z Load resources from src/main/resources in build.xml commit 9e55baac85a478b7ccd57ffb8152036788c95616 Author: Emilian Bold Date: 2017-07-10T21:11:58Z Move launcher to dedicated folder commit 2468c2b5a9ca0bf740a3f1458436adff6240848e Author: Emilian Bold Date: 2017-07-10T21:17:59Z Avoid cyclic dependency in ShutdownClient commit 849db7464435122717d452d16f6acf033a3fa67a Author: Emilian Bold Date: 2017-07-10T22:05:04Z Build the launcher from the new folder commit e498c8233c8cb9f9d90af98378ff0e01178a7295 Author: Emilian Bold Date: 2017-07-11T08:48:41Z Add pom.xml files commit c3c4c7fe3cb3674620e8247a5690ccf832b0aa49 Author: Emilian Bold Date: 2017-07-11T13:53:23Z Move bshclient to dedicated folder commit ee90e0de95fc934bd69c01aea024b04e75e60437 Author: Emilian Bold Date: 2017-07-16T20:27:19Z Rename default.notice to plain NOTICE and default.license to LICENSE to make it easy on Maven --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #293: Prevent use of the same array in CollectionProperty (clos...
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/293 Done. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #293: Prevent use of the same array in CollectionProperty (clos...
Github user emilianbold commented on the issue:
https://github.com/apache/jmeter/pull/293
Like I've said, I wanted my patch to be low-impact. Ideally normalizeList
should be something like this:
```java
protected Collection normalizeList(Collection coll) {
return coll.stream()
.map(this::convertObject)
.collect(Collectors.toCollection(ArrayList::new));
}
```
Then normalizeMap should be like this:
```java
protected Map normalizeMap(Map coll) {
return coll.entrySet()
.stream()
.collect(Collectors.toMap(entry -> {
Object key = entry.getKey();
if (key == null) {
return null;
} else {
if (!(key instanceof String)) {
log.error("Expected key type String, found:
{}", key.getClass());
}
return key.toString();
}
}, entry -> convertObject(entry.getValue(;
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jmeter issue #293: Prevent use of the same array in CollectionProperty (clos...
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/293 > could be the same issue available with the next method "normalizeMap"? Actually, yes! Should I update this patch or leave normalizeMap for another fix? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #293: Prevent use of the same array in CollectionProperty (clos...
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/293 It's unclear to me why it's so important to use the same kind of collection class. So I tried to keep the patch low-impact. Instead of null or an empty list it could return a new ArrayList with the objects. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #293: Prevent use of the same array in CollectionProperty (clos...
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/293 Done. The new patch is much cleaner. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter issue #293: Prevent use of the same array in CollectionProperty (clos...
Github user emilianbold commented on the issue: https://github.com/apache/jmeter/pull/293 Ammeding the commit as we speak. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] jmeter pull request #293: Prevent use of the same array in CollectionPropert...
GitHub user emilianbold opened a pull request: https://github.com/apache/jmeter/pull/293 Prevent use of the same array in CollectionProperty (close #58743) CollectionProperty constructor receives an empty ObjectTableModel.objects collection but normalizeList will return it as-is instead of creating a new empty collection. The two classes share the same array but add different kind of classes. NOTE: Basically the bug is in AbstractProperty.normalizeList because it should never reuse the same instance. I've done the patch as you see it because it has the lowest impact and does not change the normalizeList API (perhaps reusing is expected in some cases?) Let me know if I should redo the patch by just changing normalizeList! Constructor stacktrace: at org.apache.jmeter.testelement.property.CollectionProperty.(CollectionProperty.java:40) at org.apache.jmeter.testelement.property.AbstractProperty.makeProperty(AbstractProperty.java:395) at org.apache.jmeter.testelement.property.AbstractProperty.createProperty(AbstractProperty.java:368) at org.apache.jmeter.testbeans.gui.TestBeanGUI.setPropertyInElement(TestBeanGUI.java:277) at org.apache.jmeter.testbeans.gui.TestBeanGUI.modifyTestElement(TestBeanGUI.java:266) at org.apache.jmeter.gui.tree.JMeterTreeModel.addComponent(JMeterTreeModel.java:148) at org.apache.jmeter.gui.action.AddToTree.doAction(AddToTree.java:70) at org.apache.jmeter.gui.action.ActionRouter.performAction(ActionRouter.java:74) at org.apache.jmeter.gui.action.ActionRouter.lambda$actionPerformed$28(ActionRouter.java:59) at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:311) You can merge this pull request into a Git repository by running: $ git pull https://github.com/emilianbold/jmeter trunk Alternatively you can review and apply these changes as the patch at: https://github.com/apache/jmeter/pull/293.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #293 commit 2b829035f144c49e565d99ed0ca8f83ba6f85cf4 Author: Emilian Bold Date: 2017-04-26T14:57:58Z Prevent use of the same array in CollectionProperty (close #58743) CollectionProperty constructor receives an empty ObjectTableModel.objects collection but normalizeList will return it as-is instead of creating a new empty collection. The two classes share the same array but add different kind of classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
