Hi ! I am very happy with JMeter, which is very useful and powerful ! So, I decided to contribute in my way by pointing those little annoying things which the users had to put up with but didn't notice or didn't take the time to report. My thought are that a Undo/Redo feature is required while editing a test plan as it is well known that user _do_ mistakes and don't want to be punished simply for being human. At first, I would argue that adding such a framework to JMeter would imply some work, refactoring some things and implementing Command Pattern to ensure proper efficiency and power. When I think a little bit more about it, I feel it to be like an "internal XML patching system", as the problem is to apply modifications to the plan test tree (which can be represented as an XML tree) and add more modifications or revert some others. I hope this won't be too hard to implement in JMeter. I simply don't know as I've only _thought_ about it but not designed code to do it (which I am not able to write at this time).
The classic UNDO shortcut 'CTRL-Z' has been assigned to [Launch > Remote start all] which is not very consistent in a GUI point of view. If the undo-redo support is not implemented/finalized/integrated to the next release, the shortcut should be removed/changed even though : I can't count the number of times JMeter lagged for 2 seconds before throwing me an ironical "Bad call to remote host". By the way, that error message should be improved by, for example, showing what's the actual "remote host" and maybe what's the actual "bad call" (i.e command name or something else).
I agree that Control-Z is confusing. Since Control-R is used for a normal run, I've changed Run Remote to Control- Shift-R. This is in SVN r542707. Also fixed the confusing "Bad call" messages in r542633.
It has been more than two years since this enhancement request was opened. Lacking undo/redo is bad and frustrating nuisance. Just one bad edit, forgetting the previous contents of some obscure request parameter... what then?
Yes, it would be a useful enhancement, but as far as I can tell, implementing undo/redo would involve a lot of code changes. If anyone can provide a patch, then of course it will be evaluated and applied if appropriate.
*** Bug 51404 has been marked as a duplicate of this bug. ***
*** Bug 51819 has been marked as a duplicate of this bug. ***
Hi! Just wanted to express my support for addressing this issue. I would love to see undo/redo support for two independent areas: 1) text areas (it would be more-than-sufficient to just have that information while working in the textarea, no need to get sophisticated with having this work in multiple textareas at the same time or shit like that), it is just really annoying to accidentally delete something and then having to do it all again. 2) the element tree on the left side (probably more complicated?) Many thanks for listening :) Imm
Created attachment 28217 [details] proposed patch Here's my current progress patch. Notes: 1) I failed to make it select recorded tree selection path properly, tree shows no nodes selected. Tree selection is commented out and after each undo/redo Workbench becomes selected 2) I didn't use undo action hints like for action labeling like "Undo (Add Thread Group)" but it would be nice to have it. There is "comment" field in history items to have it.
Thanks very much. Looks good but not tried the code yet. Just a couple of minor points: We release code under the Apache License and each file has to have the standard AL header. These are missing from the new files you have provided. Also, we don't use @author tags in code. This is mainly because the ASF is about community rather than individuals; also the tags are impossible to maintain accurately as code is further developed. Instead, we credit contributors in the changes list and elsewhere (e.g. Wiki) So please could you either confirm that it is OK for us to add AL headers and remove @author tags, or provide an updated patch? Thanks!
Hi, I understand that ASF has its rules for code formatting and meta-info (and JMeter has its rules for unit tests, too). So feel free to reformat code. I believe you can make Undo working much better with your experience, starting with my patch. Of course it would be nice to be mentioned in changelog or somewhere else, it is left for your choice. I just glad to be reason for closing 5-year old issue :)
Hello Andrey, I made some tests and I see the following issues: - Limitation : Using CTRL+Z in text fields has some really disturbing behaviour - I have tried to load a consequent Test Plan ( around 6.6 Mo on disk), it ends with OOM and analysing dump, it shows UndoHistory occupates 336 MO, it contains 301 item where each one has a size > 2.3Mo, So I think patch need more work. Thanks for it anyway and we will try to find some solutions. Regards Philippe
Philippe, If it cause problems, Ctrl+Z can be replaced with any other shortcut, or even left without shortcut. To lower memory usage you can limit history size, it is trivial task, I suppose.
Created attachment 28222 [details] Updated Patch Hello, Here is a modified version with the following changes: - Added a limit on the history (25 it should be configurable), to avoid OutOfMemoryError when Test Plan is big - Added Pause/Resume methods that are used during loading , closing and merging of Tree, cause if we don't add it (at least for close and loading): 1) It does not seem useful to be able to undo elements that were added during load phase 2) Performance are really bad , a big test plan (6 Mo) takes more than 30 seconds to load - Added javadocs Regards Philippe http://www.ubik-ingenierie.com
- There is one drawback to previous change, it's that we won't be able to undo merged part (right click>merge) - Added javadocs Also note that patch is on r1231672
Philippe, you made everything much better with your changes! One question: have youn noticed my notes on selecting historical path in tree? Is it possible in JMeter?
I think the problem is due to the following: - You record a TreePath pointing to the node after modification - When you undo, a full JTree with new nodes JMeterTreeNodes is rebuilt - If you try to call tree.setSelectionPath(path), then it uses the content before undo.
Philippe, dou you see any other (working) solution to have historical tree selection paths?
I managed to fix the issue with non-undoable merge. Now will work on historical selection path.
Created attachment 31899 [details] Patch for next attempt of undo Fixed most of the earlier problems.
Created attachment 31900 [details] new files pack
Hello Andrei, I only reviewed patch. no pack file as I only have smartphone currently. I didn't see any change to undo/redo of inout fields. Do you confirm that patch only adresses undo-redo on test tree ? Thanks
Yes, I carefully checked that there is no side changes. I cleared any formatting changes that were produced by IDE.
Thanks Andrey. As I understand you made on patch file for files that differ but already exist and 1 zip that contains new files. Is it possible to only create 1 patch file containing everything ? Thanks
Philippe, I don't know how to do that using 'svn diff' command. If I could just send a pull request into GitHub repository, then it could be very easy.
If you use Eclipse: Right click on jmeter project> Team > create patch and select all files to include. If you want you can also provide a pull request It will allow me to review it and give you feedback very soon, otherwise you will have to wait for 21th august. As you're working on it now, I would br sorry to give you feedback once you don't have time to work on it snymore :) Regards
Otherwise have a look at this: - http://stackoverflow.com/questions/4248768/including-new-files-in-svn-diff
Created attachment 31901 [details] patch with added files Thanks! I completely forgot how to use svn. Added new patch, icon files will follow.
Created attachment 31902 [details] undo toolbar icon
Created attachment 31903 [details] redo toolbar icon
Thanks for update Andrei. First review looks good, some notes: - you should remove import x.y.* from code, only import the classes needed - I suggest we add an option to enable/disable history (undo/redo), we will decide later if we enable/disable it by default - undo/redo looks a bit confusing, as user may think he can also undo changes in text fields or other properties of test element. We need to find a better name although for now I am not inspired - implementing also undo/redo on test element properties would make this feature complete but it is more impacting
Created attachment 31906 [details] proposed patch with more changes Philippe, thanks for reviewing the patch. I made some changes: - removed wildcard import - added some comments and JavaDocs - added undo.size property to manipulate history length, default is 25 The undo for TestElement property change already works. History is recorded once changes applied to TestPlan. I think disabling the undo is unnecessary, since it is vital feature for JMeter users. The option to limit history size helps protecting from too much memory consumption. The choice seems to be to provide people undo without in-fields undo, or not provide ability to revert the actions at all. I believe the value for this feature is too high to block it for more years waiting to implement in-fields undo... Hope this makes sense.
Is there a build already with this feature? I'd like to give that a try...
@Andrei, thanks, I get your point. @Shmullik, see @jmeter_plugins twitter account for info on this. Note that once we integrate this in nightly build, we will require test help from users community, so your tests will be welcome.
Here's the link for build to try the feature: http://jmeter-plugins.org/downloads/file/nightly/jmeter-2.11-undo.tgz
Hello Andrei, Could you generate the patch in Unified format ? I am not able to integrate it with the current format. To do this use Eclipse, select jmeter project and then Team > Create Patch. Thanks
Sorry, I don't use Eclipse. I generated the patch using "svn diff" command, it usually applies OK using "patch" command-line utility.
(In reply to Andrey Pohilko from comment #36) > Sorry, I don't use Eclipse. I generated the patch using "svn diff" command, > it usually applies OK using "patch" command-line utility. Looks like this is a Bugzilla issue. Eclipse expects headers for each patch, but these are dropped when using Diff / Raw Unified You need to click on the attachment name, or use Details.
Date: Sat Sep 6 21:14:49 2014 New Revision: 1622936 URL: http://svn.apache.org/r1622936 Log: Bug 42248 - Undo-redo support on Test Plan tree modification Bugzilla Id: 42248 Added: jmeter/trunk/src/core/org/apache/jmeter/gui/UndoHistory.java (with props) jmeter/trunk/src/core/org/apache/jmeter/gui/UndoHistoryItem.java (with props) jmeter/trunk/src/core/org/apache/jmeter/gui/action/UndoCommand.java (with props) jmeter/trunk/src/core/org/apache/jmeter/images/toolbar/redo.png (with props) jmeter/trunk/src/core/org/apache/jmeter/images/toolbar/undo.png (with props) Modified: jmeter/trunk/bin/jmeter.properties jmeter/trunk/src/core/org/apache/jmeter/gui/GuiPackage.java jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionNames.java jmeter/trunk/src/core/org/apache/jmeter/gui/util/MenuFactory.java jmeter/trunk/src/core/org/apache/jmeter/images/toolbar/icons-toolbar.properties jmeter/trunk/src/core/org/apache/jmeter/resources/messages.properties jmeter/trunk/src/core/org/apache/jmeter/resources/messages_fr.properties jmeter/trunk/xdocs/changes.xml
Date: Sat Sep 6 21:21:41 2014 New Revision: 1622938 URL: http://svn.apache.org/r1622938 Log: Bug 42248 - Undo-redo support on Test Plan tree modification svn:eol Bugzilla Id: 42248 Modified: jmeter/trunk/src/core/org/apache/jmeter/gui/UndoHistory.java (props changed) jmeter/trunk/src/core/org/apache/jmeter/gui/UndoHistoryItem.java (props changed) jmeter/trunk/src/core/org/apache/jmeter/gui/action/UndoCommand.java (props changed) Date: Sat Sep 6 21:23:48 2014 New Revision: 1622939 URL: http://svn.apache.org/r1622939 Log: Bug 42248 - Undo-redo support on Test Plan tree modification svn:mime-type Bugzilla Id: 42248 Modified: jmeter/trunk/src/core/org/apache/jmeter/images/toolbar/redo.png (props changed) jmeter/trunk/src/core/org/apache/jmeter/images/toolbar/undo.png (props changed) Date: Sat Sep 6 21:34:45 2014 New Revision: 1622941 URL: http://svn.apache.org/r1622941 Log: Bug 42248 - Undo-redo support on Test Plan tree modification Changed icons to use open_icon_library-CC and have a different color for undo and redo Bugzilla Id: 42248 Modified: jmeter/trunk/src/core/org/apache/jmeter/images/toolbar/redo.png jmeter/trunk/src/core/org/apache/jmeter/images/toolbar/undo.png
@Andrei, few questions: - Testing feature, I see icon is not disabled when undo or redo cannot be done - Where do the icon come from , we need to know the license of icons. For now I replaced them by open_icon_library used in JMeter Many thanks for this contribution !
Date: Sat Sep 6 21:59:18 2014 New Revision: 1622945 URL: http://svn.apache.org/r1622945 Log: Bug 42248 - Undo-redo support on Test Plan tree modification Update icons state on redo/undo Bugzilla Id: 42248 Modified: jmeter/trunk/src/core/org/apache/jmeter/gui/action/UndoCommand.java jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterToolBar.java
@Andrei, icon update issue fixed now. So only icons license question remains.
Date: Sat Sep 6 22:04:42 2014 New Revision: 1622947 URL: http://svn.apache.org/r1622947 Log: Bug 42248 - Undo-redo support on Test Plan tree modification Update icons state on redo/undo Oups missed 1 class Bugzilla Id: 42248 Modified: jmeter/trunk/src/core/org/apache/jmeter/gui/MainFrame.java
Regarding icons license you did everything right. Using the same icons collection for whole toolbar is better than using different. Initial icons were taken from the Internet, I did not know their license. Regarding toolbar buttons enabling-disabling there is a problem, since JMeter code does not offer interface for enabling/disabling icons easily, at leas I did not find one. So I left it enabled and put a TODO comment for this. I have dowloaded latest code and now toolbar buttons are always disabled. I suppose this was not your intention. From what I've seen in the code, implementing the interface for proper buttons enabling/disabling would require some work, that's why I did not try to solve it together with Undo, just don't want to link two big problems.
Hello Andrei, I made a mistake in the place where I update toolbar, so they are only updated in undo/redo calls instead of being so every time history changes. I will fix this this afternook hopefully, I disabled it temporarily in trunk. Thanks for noticing and reporting it.
Date: Sun Sep 7 08:31:46 2014 New Revision: 1622984 URL: http://svn.apache.org/r1622984 Log: Bug 42248 - Undo-redo support on Test Plan tree modification Temporary disable as I missed the right place to call it Bugzilla Id: 42248 Modified: jmeter/trunk/src/core/org/apache/jmeter/gui/action/UndoCommand.java Date: Sun Sep 7 14:04:34 2014 New Revision: 1623019 URL: http://svn.apache.org/r1623019 Log: Bug 42248 - Undo-redo support on Test Plan tree modification Correct toolbar undo/redo buttons disable/enable Bugzilla Id: 42248 Modified: jmeter/trunk/src/core/org/apache/jmeter/gui/GuiPackage.java jmeter/trunk/src/core/org/apache/jmeter/gui/MainFrame.java jmeter/trunk/src/core/org/apache/jmeter/gui/UndoHistory.java jmeter/trunk/src/core/org/apache/jmeter/gui/action/UndoCommand.java
Hello JMeter Team, We started testing the nightly build recently and we wanted to report some issues we noticed related to this great feature: - When you have a "realistic " Test Plan of 2.4 mb, feature makes JMeter unusable. Whenever you change something, it takes 60 seconds to do it and UI is completely blocked (during clone), this happens with -Xms2g -Xmx under 2.7 Ghz Intel Core I7 Mac Book with 16 Gb of RAM with Java 8u20 Mac OSX Mavericks - Undo/Redo is called "abusively" when for example you: 1) Search for a node by name 2) In Module Controller GUI, when you expand a node Analysing it, is it due to JMeterTreeNode#setMarkedBySearch calling treeModel.nodeChanged(this). Regards Ubik Load Pack Team
Created attachment 32026 [details] Patch that disables Undo/Redo feature if undo.history.size is set to 0 Hello, Find attached a patch that disables effectively feature if "undo.history.size" property is set to 0. Currently (before patch) if it is set to 0, all the work related to it is done but no history is stored, so for a medium to big Test Plan you get big slowdowns without any benefit. Regards Ubik Load Pack
Hi Andrey / Philippe, I've reported on couple of issues that I've found with the most recent nightly build: https://issues.apache.org/bugzilla/show_bug.cgi?id=57039 https://issues.apache.org/bugzilla/show_bug.cgi?id=57040 I think that this undo-redo is a major milestone and it will require some more effort to make it public due to the current quality, which is not enough. It might make sense to close this ticket/bug - and have all follow-ups in separate bugs. This will allow to at least make this feature part of the next release, but disabled with the default configuration, so it will not affect the reputation of JMeter from one hand, but on the other - will allow more users to further test it and report on bugs.
URL: http://svn.apache.org/r1628260 Log: Bug 42248 - Undo-redo support on Test Plan tree modification Disable feature by default as it is still in ALPHA MODE Mention this in changes.xml Bugzilla Id: 42248 Modified: jmeter/trunk/bin/jmeter.properties jmeter/trunk/src/core/org/apache/jmeter/gui/UndoHistory.java jmeter/trunk/xdocs/changes.xml
(In reply to UbikLoadPack support from comment #47) > Hello JMeter Team, > We started testing the nightly build recently and we wanted to report some > issues we noticed related to this great feature: > - When you have a "realistic " Test Plan of 2.4 mb, feature makes JMeter > unusable. Whenever you change something, it takes 60 seconds to do it and UI > is completely blocked (during clone), this happens with -Xms2g -Xmx under > 2.7 Ghz Intel Core I7 Mac Book with 16 Gb of RAM with Java 8u20 Mac OSX > Mavericks > > - Undo/Redo is called "abusively" when for example you: > 1) Search for a node by name > 2) In Module Controller GUI, when you expand a node > > Analysing it, is it due to JMeterTreeNode#setMarkedBySearch calling > treeModel.nodeChanged(this). > > Regards > Ubik Load Pack Team It does not look like JMeterTreeNode#setMarkedBySearch gets called when the project is loaded. So the 60 second delay in loading a 2MB project does not appear to be caused by setMarkedBySearch function. I made this determination by putting a breakpoint on the function: public void setMarkedBySearch(boolean tagged) { this.markedBySearch = tagged; treeModel.nodeChanged(this); } I am working on finding the root cause for the delay during the project loading phase.
(In reply to Philippe Mouawad from comment #50) > URL: http://svn.apache.org/r1628260 > Log: > Bug 42248 - Undo-redo support on Test Plan tree modification > Disable feature by default as it is still in ALPHA MODE > Mention this in changes.xml > Bugzilla Id: 42248 > > Modified: > jmeter/trunk/bin/jmeter.properties > jmeter/trunk/src/core/org/apache/jmeter/gui/UndoHistory.java > jmeter/trunk/xdocs/changes.xml Philippe, It looks like Save#convertSubTree is the culprit for slowing down the clone operation. Any idea why this blow code is slow? void convertSubTree(HashTree tree) { Iterator<Object> iter = new LinkedList<>(tree.list()).iterator(); while (iter.hasNext()) { JMeterTreeNode item = (JMeterTreeNode) iter.next(); convertSubTree(tree.getTree(item)); TestElement testElement = item.getTestElement(); // requires JMeterTreeNode tree.replaceKey(item, testElement); } }
This issue has been migrated to GitHub: https://github.com/apache/jmeter/issues/1930