You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by emilianbold <gi...@git.apache.org> on 2017/07/22 11:18:22 UTC

[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 <em...@apache.org>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by undera <gi...@git.apache.org>.
Github user undera 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.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
Github user FSchumacher 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. My steps where:
    * start a new test plan `Ctrl+L`
    * add a thread group `Ctrl+0`
    * add a http sampler `Ctrl+1`
    * add a view results tree `Ctrl+9`
    * move view results tree above the http sampler
    * undo (I wish I could have pressed `Ctrl+Z`)
    I end up with two view results trees.



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

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


---

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

Posted by emilianbold <gi...@git.apache.org>.
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 issue #300: Bug 57039 - Inconsistency with the undo/redo log

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    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:
    
    1. Open JMeter
    2. Select Menu File > New => An error log appears with the stacktrace (as per line 310 in 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    @emilianbold , do you think computing dirty should be part of the undoable 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 #300: Bug 57039 - Inconsistency with the undo/redo log

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    Hello @emilianbold ,
    I have merged 310. 
    Can you close this one ?
    
    Thank you


---

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

Posted by undera <gi...@git.apache.org>.
Github user undera commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    Amazing initiative!


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    Hi @undera,
    I agree, but for now it is not yet fully fixed although a lot of progress have been made thanks to Emilian.
    Regards


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    Hi @emilianbold ,
    The bug that you previously fixed with TreeState has reappeared.
    
    Regards


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    Hi Emilian,
    Thanks for feedback.
    For now with my last tests:
    - I still have issues with check dirty when I do an intermediate save and undo all changes, dirty is not detected
    - I have the issue you explained, it is explainable but I am afraid it looks strange to users.
    
    Can others confirm the issues I face ?
    
    The feature is clearly better than what it was but I would prefer to have it completed before release.
    I can commit it but should we do that before 3.3 ? 
    Another option is to commit it but keep it disabled.
    



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    Hi Emilian,
    Thanks for the fix for last bug.
    I cannot provide the 900K test plan as it's a real life one from a customer. I'll try to  create a fake one.
    
    Anyway, I think the most annoying issues now would be by descending priorities:
    - https://bz.apache.org/bugzilla/show_bug.cgi?id=57040
    - The non restored marked for search nodes + performance issue


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    Create a Tree like this:
    Test Plan
    |-------------User Defined Variable
    |-------------Thread Group 1
                                      |----------TC1
    |-------------Thread Group 2
                                      |----------TC2
    
    Expand Thread Group 1 and Unexpand Thread Group 2.
    
    Clone User Defined Variable
    Undo
    You end up with both Thread Groups expanded 



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    HI @emilianbold ,
    It seems to work correctly as of my tests for now, but it requires further tests.
    
    Testing it, I have noticed we have an existing bug with dirty check, it seems it doesn't correctly detect CUT and does not propose save.
    
    I have fixed it, shall I commit it or wait for your PR to be complete ?
    Do you consider it mature enough for a commit now ?
    
    Thanks
     



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by undera <gi...@git.apache.org>.
Github user undera commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    If undo/redo is going to be fixed, can we consider re-enabling it by default through default properties?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    Hello Emilian,
    1) Thanks for fix, see my comment
    
    2) I've noticed another issue.
    
    Create a Tree like this:
    Test Plan
    |-------------User Defined Variable
    |-------------Thread Group 1
                                       |----------TC1
    |-------------Thread Group 2
                                       |----------TC2
    
    Expand Thread Group 1 and Unexpand Thread Group 2.
    
    Clone User Defined Variable
    Undo
    You end up with Thread Group 2 expanded and Thread Group 1  unexpanded.
    
    TreeState#restore() seems to be lost in indexes.



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    I'm using code from:
    - https://patch-diff.githubusercontent.com/raw/apache/jmeter/pull/300.patch
    
    I have just found the problem.
    I start jmeter using :
    - t LAST
    
    which reloads last opened plan.
    
    This option seems to break some fields:
    - Loaded test plan is detected as dirty immediately
    
    
    Still I confirm that I face the issue with unfolding, let me clarify the scenario:
    Create a Tree like this:
    Test Plan
    |-------------User Defined Variable
    |-------------Thread Group 1
                                    |----------Debug Sampler1
    |-------------Thread Group 2
                                    |----------Debug Sampler2
    
    Expand Thread Group 1 and Unexpand Thread Group 2.
    Clone User Defined Variable using menu Edit > Duplicate
    Undo
    You end up with both Thread Groups expanded



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    Hi @emilianbold , 
    AS you dropped your repository, I am not able to get the patch file to merge it.
    Any chance you have it ?
    
    Thanks


---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad 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.
    
    Am I wrong to think dirty may be simpler to compute now ?
    - Any change (Element addition/removal) on tree makes dirty = TRUE
    - Any change in properties of Element makes dirty = TRUE 
    - When Doing/Undo => dirty = (dirty) ? (undohistory == empty ? TRUE : FALSE) : TRUE 
    
    There is the more complex case of loading a Test plan in version N from a JMeter in version N+X
    



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    It seems the creation order is important to reproduce:
    First create Thread Group 1, add to it a Debug Sampler1
    Second create Thread Group 2, add to it a Debug Sampler2
    Add User Defined Variables, move it before Thread Group 1
    Unfold TG2 and fold TG1
    Duplicate User Defined Variables
    
    Play with undo, redo it is broken,  it seems the element moving is not correctly handled.



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
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<Integer> 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<Integer> savedExpanded;
    +
    +        // GUI tree selected row
    +        private final int savedSelected;
    +
    +        public TreeStateImpl(int savedSelected, List<Integer> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    Thanks @emilianbold , unfortunately I am still not able to generate diff nor patch file to merge it.



---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by Antonio Gomes Rodrigues <ra...@gmail.com>.
Ok for me

2017-09-03 17:31 GMT+02:00 pmouawad <gi...@git.apache.org>:

> Github user pmouawad commented on the issue:
>
>     https://github.com/apache/jmeter/pull/300
>
>     I will merge this one after 3.3 (note I could do it before provided
> the feature is disabled by default).
>     I think that new code makes feature better anyway than the current
> existing one.
>
>     Ok for you team ?
>     Thanks
>
>
> ---
> 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 infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>

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

Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:

    https://github.com/apache/jmeter/pull/300
  
    I will merge this one after 3.3 (note I could do it before provided the feature is disabled by default).
    I think that new code makes feature better anyway than the current existing one.
    
    Ok for you team ?
    Thanks


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by pmouawad <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by FSchumacher <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by emilianbold <gi...@git.apache.org>.
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 <no...@github.com> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---