[GitHub] jmeter pull request #300: Bug 57039 - Inconsistency with the undo/redo log

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/jmeter/pull/300


---

[GitHub] jmeter pull request #300: Bug 57039 - Inconsistency with the undo/redo log

[FSchumacher]

Github user FSchumacher commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r131245265
  
--- 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 --

Fair enough.


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

[FSchumacher]

Github user FSchumacher commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r131244988
  
--- 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 --

My reasoning was that `isEmpty()` can be implemented more efficient than 
`size() == 0`


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

[emilianbold]

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 pull request #300: Bug 57039 - Inconsistency with the undo/redo log

[emilianbold]

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

[emilianbold]

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

[emilianbold]

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

[emilianbold]

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 pull request #300: Bug 57039 - Inconsistency with the undo/redo log

[pmouawad]

Github user pmouawad commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r130241103
  
--- 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 --

I investigated further code and behaviour, I think issue in incorrect dirty 
detection is due to the hypothesis made on GuiPackage#isDirty(), it appears it 
is not up to date when added to UndoHistory. 


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

[FSchumacher]

Github user FSchumacher commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r130236104
  
--- Diff: src/core/org/apache/jmeter/gui/UndoHistory.java ---
@@ -118,8 +87,12 @@ public void clear() {
 return;
 }
 log.debug("Clearing undo history");
-history.clear();
-position = INITIAL_POS;
+manager.discardAllEdits();
+if (isTransaction()) {
+log.warn("Clearing undo history with " + transactions.size() + 
" unfinished transactions");
--- End diff --

I like the logs to use the formatting/placeholder feature `{}` together 
with parameters.


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

[FSchumacher]

Github user FSchumacher commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r130236542
  
--- Diff: src/core/org/apache/jmeter/gui/action/SearchTreeDialog.java ---
@@ -258,6 +259,7 @@ private void doSearch(ActionEvent e) {
 jTree.expandPath(new TreePath(jMeterTreeNode.getPath()));
 }
 }
+guiPackage.endUndoTransaction();
--- End diff --

No `try { ... } finally { guiPackage.endUndoTransaction() }` here? Why 
above?


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

[FSchumacher]

Github user FSchumacher commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r130236022
  
--- 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() {
+@Override
+public void restore(GuiPackage guiInstance) {
+//nothing
+}
+};
+
+/**
+ * Save tree expanded and selected state
+ *
+ * @param guiPackage {@link GuiPackage} to be used
+ */
+public static TreeState from(GuiPackage guiPackage) {
+if (guiPackage == null) {
+return NOTHING;
+}
+
+MainFrame mainframe = guiPackage.getMainFrame();
+if (mainframe != null) {
+final JTree tree = mainframe.getTree();
+int savedSelected = tree.getMinSelectionRow();
+ArrayList savedExpanded = new ArrayList<>();
+
+for (int rowN = 0; rowN < tree.getRowCount(); rowN++) {
+if (tree.isExpanded(rowN)) {
+savedExpanded.add(rowN);
+}
+}
+
+return new TreeStateImpl(savedSelected, savedExpanded);
+}
+
+return NOTHING;
+}
+
+static final class TreeStateImpl implements TreeState {
+
+// GUI tree expansion state
+private final List savedExpanded;
+
+// GUI tree selected row
+private final int savedSelected;
+
+public TreeStateImpl(int savedSelected, List 
savedExpanded) {
+this.savedSelected = savedSelected;
+this.savedExpanded = savedExpanded;
+}
+
+@Override
+public void restore(GuiPackage guiInstance) {
+MainFrame mainframe = guiInstance.getMainFrame();
+if (mainframe == null) {
+//log?
+return;
+}
+
+final JTree tree = mainframe.getTree();
+
+if (savedExpanded.size() > 0) {
--- End diff --

use `isEmpty()` can be more efficient


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

[FSchumacher]

Github user FSchumacher commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r130236378
  
--- 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 --

Perhaps a small helper method `needsTransaction(actionCommand)` would help 
readability, as it would get rid of the double negation (`!NO..`)


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

[FSchumacher]

Github user FSchumacher commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r130235928
  
--- 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 --

`static final` to conform with java conventions and the anonymous class 
implementing `NOTHING` could be rewritten as a lambda function.


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

[FSchumacher]

Github user FSchumacher commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r130236483
  
--- Diff: src/core/org/apache/jmeter/gui/action/CheckDirty.java ---
@@ -64,11 +64,12 @@
 public CheckDirty() {
 previousGuiItems = new HashMap<>();
 ActionRouter.getInstance().addPreActionListener(ExitCommand.class, 
this);
+
ActionRouter.getInstance().addPostActionListener(UndoCommand.class, this);
 }
 
 @Override
 public void actionPerformed(ActionEvent e) {
-if (e.getActionCommand().equals(ActionNames.EXIT)) {
+if (e.getActionCommand().equals(ActionNames.EXIT) || 
e.getActionCommand().equals(ActionNames.UNDO) || 
e.getActionCommand().equals(ActionNames.REDO)) {
--- End diff --

Maybe we should define one set of names that identify the commands that 
should call `doAction` than this would be probably more readable.


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

[FSchumacher]

Github user FSchumacher commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r130235880
  
--- 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 --

Wouldn't it be nicer and more efficient to delegate `isEmpty` to edit?


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

[FSchumacher]

Github user FSchumacher commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r130235898
  
--- 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 --

This is a serializable class but a serial version uid field is missing


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

[FSchumacher]

Github user FSchumacher commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r130236267
  
--- 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 would add `this.` to `copy` and `comment` to be consistent.


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

[pmouawad]

Github user pmouawad commented on a diff in the pull request:

https://github.com/apache/jmeter/pull/300#discussion_r128917971
  
--- Diff: src/core/org/apache/jmeter/gui/action/ActionRouter.java ---
@@ -67,7 +67,10 @@ public void actionPerformed(final ActionEvent e) {
 
 private void performAction(final ActionEvent e) {
 String actionCommand = e.getActionCommand();
-GuiPackage.getInstance().beginUndoTransaction();
+//New action will clear undo, no point in having a transaction
+if(!ActionNames.CLOSE.equals(actionCommand)) {
--- End diff --

I think you need to filter also OPEN and OPEN_RECENT


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

[emilianbold]

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.
---