You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by sebb <se...@gmail.com> on 2013/07/01 16:39:54 UTC
Re: svn commit: r1498398 - in /jmeter/trunk: src/core/org/apache/jmeter/gui/action/
src/core/org/apache/jmeter/gui/plugin/ src/core/org/apache/jmeter/gui/util/ xdocs/
On 1 July 2013 13:06, <pm...@apache.org> wrote:
> Author: pmouawad
> Date: Mon Jul 1 12:06:02 2013
> New Revision: 1498398
>
> URL: http://svn.apache.org/r1498398
> Log:
> Bug 55172 - Provide plugins a way to add Top Menu and menu items
> Bugzilla Id: 55172
-1
Please can we have some discussion of new features before they are added?
For example, there are perhaps better ways of providing the
functionality that don't involve class scanning.
I'm not saying this is a bad feature, but we do need to consider the
effect on the rest of JMeter before adding more code.
> Added:
> jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/
> jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java (with props)
> Modified:
> jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
> jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
> jmeter/trunk/xdocs/changes.xml
>
> Modified: jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java?rev=1498398&r1=1498397&r2=1498398&view=diff
> ==============================================================================
> --- jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java (original)
> +++ jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java Mon Jul 1 12:06:02 2013
> @@ -261,8 +261,8 @@ public final class ActionRouter implemen
> false, // innerClasses - should we include inner classes?
> // contains - classname should contain this string
> // This was added in r325814 as part of changes for the reporting tool
> - "org.apache.jmeter.gui", // $NON-NLS-1$
> - null, // notContains - classname should not contain this string
> + null, // contains - classname should contain this string
> + "org.apache.jmeter.report.gui", // $NON-NLS-1$ // notContains - classname should not contain this string
That change looks wrong.
> false); // annotations - true if classnames are annotations
> commands = new HashMap<String, Set<Command>>(listClasses.size());
> if (listClasses.isEmpty()) {
>
> Added: jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java?rev=1498398&view=auto
> ==============================================================================
> --- jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java (added)
> +++ jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java Mon Jul 1 12:06:02 2013
> @@ -0,0 +1,60 @@
> +/*
> + * Licensed to the Apache Software Foundation (ASF) under one or more
> + * contributor license agreements. See the NOTICE file distributed with
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + *
> + */
> +
> +package org.apache.jmeter.gui.plugin;
> +
> +import javax.swing.JMenu;
> +import javax.swing.JMenuItem;
> +import javax.swing.MenuElement;
> +
> +/**
> + * @since 2.10
> + */
> +public interface MenuCreator {
> + public enum MENU_LOCATION {
> + FILE,
> + EDIT,
> + RUN,
> + OPTIONS,
> + HELP,
> + SEARCH
> + }
> +
> + /**
> + * MenuItems to be added in location menu
> + * @param location in top menu
> + * @return array of {@link JMenuItem}
> + */
> + JMenuItem[] getMenuItemsAtLocation(MENU_LOCATION location);
> +
> + /**
> + * @return array of JMenu to be put as top level menu between Options and Help
> + */
> + JMenu[] getTopLevelMenus();
> +
> + /**
> + * @param menu MenuElement
> + * @return true if menu was concerned by Locale change
> + */
> + boolean localeChanged(MenuElement menu);
> +
> + /**
> + * Update Top Level menu on Locale Change
> + */
> + void localeChanged();
> +}
> \ No newline at end of file
Please ensure files have final EOL
>
> Propchange: jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> ------------------------------------------------------------------------------
> svn:mime-type = text/plain
Also svn:eol-style native.
> Modified: jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java?rev=1498398&r1=1498397&r2=1498398&view=diff
> ==============================================================================
> --- jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java (original)
> +++ jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java Mon Jul 1 12:06:02 2013
> @@ -20,6 +20,8 @@ package org.apache.jmeter.gui.util;
>
> import java.awt.Component;
> import java.awt.event.KeyEvent;
> +import java.io.IOException;
> +import java.lang.reflect.Modifier;
> import java.util.ArrayList;
> import java.util.Collection;
> import java.util.Iterator;
> @@ -43,11 +45,14 @@ import org.apache.jmeter.gui.action.Acti
> import org.apache.jmeter.gui.action.ActionRouter;
> import org.apache.jmeter.gui.action.KeyStrokes;
> import org.apache.jmeter.gui.action.LoadRecentProject;
> +import org.apache.jmeter.gui.plugin.MenuCreator;
> +import org.apache.jmeter.gui.plugin.MenuCreator.MENU_LOCATION;
> import org.apache.jmeter.util.JMeterUtils;
> import org.apache.jmeter.util.LocaleChangeEvent;
> import org.apache.jmeter.util.LocaleChangeListener;
> import org.apache.jmeter.util.SSLManager;
> import org.apache.jorphan.logging.LoggingManager;
> +import org.apache.jorphan.reflect.ClassFinder;
> import org.apache.jorphan.util.JOrphanUtils;
> import org.apache.log.Logger;
>
> @@ -134,6 +139,8 @@ public class JMeterMenuBar extends JMenu
>
> private JMenu searchMenu;
>
> + private ArrayList<MenuCreator> menuCreators;
> +
> public static final String SYSTEM_LAF = "System"; // $NON-NLS-1$
>
> public static final String CROSS_PLATFORM_LAF = "CrossPlatform"; // $NON-NLS-1$
> @@ -229,6 +236,32 @@ public class JMeterMenuBar extends JMenu
> * should be defined in a file somewhere, but that is for later.
> */
> public void createMenuBar() {
> + this.menuCreators = new ArrayList<MenuCreator>();
> + try {
> + List<String> listClasses = ClassFinder.findClassesThatExtend(
> + JMeterUtils.getSearchPaths(),
> + new Class[] {MenuCreator.class });
> + for (String strClassName : listClasses) {
> + try {
> + if(log.isDebugEnabled()) {
> + log.debug("Loading menu creator class: "+ strClassName);
> + }
> + Class<?> commandClass = Class.forName(strClassName);
> + if (!Modifier.isAbstract(commandClass.getModifiers())) {
> + if(log.isDebugEnabled()) {
> + log.debug("Instantiating: "+ commandClass.getName());
> + }
> + MenuCreator creator = (MenuCreator) commandClass.newInstance();
> + menuCreators.add(creator);
> + }
> + } catch (Exception e) {
> + log.error("Exception registering "+MenuCreator.class.getName() + " with implementation:"+strClassName, e);
> + }
> + }
> + } catch (IOException e) {
> + log.error("Exception finding implementations of "+MenuCreator.class, e);
> + }
> +
> makeFileMenu();
> makeEditMenu();
> makeRunMenu();
> @@ -240,6 +273,13 @@ public class JMeterMenuBar extends JMenu
> this.add(searchMenu);
> this.add(runMenu);
> this.add(optionsMenu);
> + for (Iterator iterator = menuCreators.iterator(); iterator.hasNext();) {
> + MenuCreator menuCreator = (MenuCreator) iterator.next();
> + JMenu[] topLevelMenus = menuCreator.getTopLevelMenus();
> + for (JMenu topLevelMenu : topLevelMenus) {
> + this.add(topLevelMenu);
> + }
> + }
> this.add(helpMenu);
> }
>
> @@ -265,6 +305,9 @@ public class JMeterMenuBar extends JMenu
> helpMenu.add(setDebug);
> helpMenu.add(resetDebug);
> helpMenu.add(heapDump);
> +
> + addPluginsMenuItems(helpMenu, menuCreators, MENU_LOCATION.HELP);
> +
> helpMenu.addSeparator();
> helpMenu.add(help_about);
> }
> @@ -307,6 +350,8 @@ public class JMeterMenuBar extends JMenu
>
> JMenuItem expand = makeMenuItemRes("menu_expand_all", ActionNames.EXPAND_ALL, KeyStrokes.EXPAND_ALL); //$NON-NLS-1$
> optionsMenu.add(expand);
> +
> + addPluginsMenuItems(optionsMenu, menuCreators, MENU_LOCATION.OPTIONS);
> }
>
> private static class LangMenuHelper{
> @@ -430,6 +475,8 @@ public class JMeterMenuBar extends JMenu
> runMenu.addSeparator();
> runMenu.add(run_clear);
> runMenu.add(run_clearAll);
> +
> + addPluginsMenuItems(runMenu, menuCreators, MENU_LOCATION.RUN);
> }
>
> private void makeEditMenu() {
> @@ -439,6 +486,8 @@ public class JMeterMenuBar extends JMenu
> // From the Java Look and Feel Guidelines: If all items in a menu
> // are disabled, then disable the menu. Makes sense.
> editMenu.setEnabled(false);
> +
> + addPluginsMenuItems(editMenu, menuCreators, MENU_LOCATION.EDIT);
> }
>
> private void makeFileMenu() {
> @@ -492,6 +541,9 @@ public class JMeterMenuBar extends JMenu
> for(JComponent jc : file_load_recent_files){
> fileMenu.add(jc);
> }
> +
> + addPluginsMenuItems(fileMenu, menuCreators, MENU_LOCATION.FILE);
> +
> fileMenu.add(file_exit);
> }
>
> @@ -501,11 +553,32 @@ public class JMeterMenuBar extends JMenu
>
> JMenuItem search = makeMenuItemRes("menu_search", 'F', ActionNames.SEARCH_TREE, KeyStrokes.SEARCH_TREE); //$NON-NLS-1$
> searchMenu.add(search);
> - searchMenu.setEnabled(true);
> + search.setEnabled(true);
Why was this changed?
Is it part of the new feature, or was it a separate bug fix?
>
> JMenuItem searchReset = makeMenuItemRes("menu_search_reset", ActionNames.SEARCH_RESET); //$NON-NLS-1$
> searchMenu.add(searchReset);
> - searchMenu.setEnabled(true);
> + searchReset.setEnabled(true);
> +
As above.
> + addPluginsMenuItems(searchMenu, menuCreators, MENU_LOCATION.SEARCH);
> + }
> +
> + /**
> + * @param menu
> + * @param menuCreators
> + * @param location
> + */
> + protected void addPluginsMenuItems(JMenu menu, List<MenuCreator> menuCreators, MENU_LOCATION location) {
Could this be private?
> + boolean addedSeparator = false;
> + for (MenuCreator menuCreator : menuCreators) {
> + JMenuItem[] menuItems = menuCreator.getMenuItemsAtLocation(location);
> + for (JMenuItem jMenuItem : menuItems) {
> + if(!addedSeparator) {
> + menu.addSeparator();
> + addedSeparator = true;
> + }
> + menu.add(jMenuItem);
> + }
> + }
> }
>
> public void setRunning(boolean running, String host) {
> @@ -589,6 +662,9 @@ public class JMeterMenuBar extends JMenu
> updateMenuElement(runMenu);
> updateMenuElement(optionsMenu);
> updateMenuElement(helpMenu);
> + for (MenuCreator creator : menuCreators) {
> + creator.localeChanged();
> + }
> }
>
> /**
> @@ -618,6 +694,11 @@ public class JMeterMenuBar extends JMenu
> Component component = menu.getComponent();
> final String compName = component.getName();
> if (compName != null) {
> + for (MenuCreator menuCreator : menuCreators) {
> + if(menuCreator.localeChanged(menu)) {
> + return;
> + }
> + }
> if (component instanceof JMenu) {
> final JMenu jMenu = (JMenu) component;
> if (isResource(jMenu.getActionCommand())){
>
> Modified: jmeter/trunk/xdocs/changes.xml
> URL: http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1498398&r1=1498397&r2=1498398&view=diff
> ==============================================================================
> --- jmeter/trunk/xdocs/changes.xml (original)
> +++ jmeter/trunk/xdocs/changes.xml Mon Jul 1 12:06:02 2013
> @@ -221,6 +221,7 @@ Transaction Controller now sets Response
> <li><bugzilla>54945</bugzilla> - Add Shutdown Hook to enable trapping kill or CTRL+C signals</li>
> <li><bugzilla>54990</bugzilla> - Download large files avoiding outOfMemory</li>
> <li><bugzilla>55085</bugzilla> - UX Improvement : Ability to create New Test Plan from Templates</li>
> +<li><bugzilla>55172</bugzilla> - Provide plugins a way to add Top Menu and menu items</li>
> </ul>
>
> <h2>Non-functional changes</h2>
>
>
Re: svn commit: r1498398 - in /jmeter/trunk: src/core/org/apache/jmeter/gui/action/
src/core/org/apache/jmeter/gui/plugin/ src/core/org/apache/jmeter/gui/util/ xdocs/
Posted by sebb <se...@gmail.com>.
On 1 July 2013 16:02, Philippe Mouawad <ph...@gmail.com> wrote:
> My detailed answers on your comments.
>
> On Mon, Jul 1, 2013 at 4:39 PM, sebb <se...@gmail.com> wrote:
>
>> On 1 July 2013 13:06, <pm...@apache.org> wrote:
>> > Author: pmouawad
>> > Date: Mon Jul 1 12:06:02 2013
>> > New Revision: 1498398
>> >
>> > URL: http://svn.apache.org/r1498398
>> > Log:
>> > Bug 55172 - Provide plugins a way to add Top Menu and menu items
>> > Bugzilla Id: 55172
>>
>> -1
>>
>> Please can we have some discussion of new features before they are added?
>>
>> For example, there are perhaps better ways of providing the
>> functionality that don't involve class scanning.
>>
>> I'm not saying this is a bad feature, but we do need to consider the
>> effect on the rest of JMeter before adding more code.
>>
>> > Added:
>> > jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/
>> > jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
>> (with props)
>> > Modified:
>> > jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
>> > jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
>> > jmeter/trunk/xdocs/changes.xml
>> >
>> > Modified:
>> jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
>> > URL:
>> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java?rev=1498398&r1=1498397&r2=1498398&view=diff
>> >
>> ==============================================================================
>> > --- jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
>> (original)
>> > +++ jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
>> Mon Jul 1 12:06:02 2013
>> > @@ -261,8 +261,8 @@ public final class ActionRouter implemen
>> > false, // innerClasses - should we include inner
>> classes?
>> > // contains - classname should contain this string
>> > // This was added in r325814 as part of changes for
>> the reporting tool
>> > - "org.apache.jmeter.gui", // $NON-NLS-1$
>> > - null, // notContains - classname should not contain
>> this string
>> > + null, // contains - classname should contain this
>> string
>> > + "org.apache.jmeter.report.gui", // $NON-NLS-1$ //
>> notContains - classname should not contain this string
>>
>> That change looks wrong.
>>
>
> No I don't think it is, as it was, it was impossible for any plugin to
> register Commands due to this. As per the comment , it says (This was added
> in r325814 as part of changes for the reporting tool) , I think it's aim
> was not to scan report related commands but the way it was done only
> allowed for commands located in org.apache.jmeter.gui.
I've now had time to look in more detail, and I agree, the change is OK.
>
>> > false); // annotations - true if classnames are
>> annotations
>> > commands = new HashMap<String,
>> Set<Command>>(listClasses.size());
>> > if (listClasses.isEmpty()) {
>> >
>> > Added:
>> jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
>> > URL:
>> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java?rev=1498398&view=auto
>> >
>> ==============================================================================
>> > --- jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
>> (added)
>> > +++ jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
>> Mon Jul 1 12:06:02 2013
>> > @@ -0,0 +1,60 @@
>> > +/*
>> > + * Licensed to the Apache Software Foundation (ASF) under one or more
>> > + * contributor license agreements. See the NOTICE file distributed with
>> > + * this work for additional information regarding copyright ownership.
>> > + * The ASF licenses this file to You under the Apache License, Version
>> 2.0
>> > + * (the "License"); you may not use this file except in compliance with
>> > + * the License. You may obtain a copy of the License at
>> > + *
>> > + * http://www.apache.org/licenses/LICENSE-2.0
>> > + *
>> > + * Unless required by applicable law or agreed to in writing, software
>> > + * distributed under the License is distributed on an "AS IS" BASIS,
>> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> implied.
>> > + * See the License for the specific language governing permissions and
>> > + * limitations under the License.
>> > + *
>> > + */
>> > +
>> > +package org.apache.jmeter.gui.plugin;
>> > +
>> > +import javax.swing.JMenu;
>> > +import javax.swing.JMenuItem;
>> > +import javax.swing.MenuElement;
>> > +
>> > +/**
>> > + * @since 2.10
>> > + */
>> > +public interface MenuCreator {
>> > + public enum MENU_LOCATION {
>> > + FILE,
>> > + EDIT,
>> > + RUN,
>> > + OPTIONS,
>> > + HELP,
>> > + SEARCH
>> > + }
>> > +
>> > + /**
>> > + * MenuItems to be added in location menu
>> > + * @param location in top menu
>> > + * @return array of {@link JMenuItem}
>> > + */
>> > + JMenuItem[] getMenuItemsAtLocation(MENU_LOCATION location);
>> > +
>> > + /**
>> > + * @return array of JMenu to be put as top level menu between
>> Options and Help
>> > + */
>> > + JMenu[] getTopLevelMenus();
>> > +
>> > + /**
>> > + * @param menu MenuElement
>> > + * @return true if menu was concerned by Locale change
>> > + */
>> > + boolean localeChanged(MenuElement menu);
>> > +
>> > + /**
>> > + * Update Top Level menu on Locale Change
>> > + */
>> > + void localeChanged();
>> > +}
>> > \ No newline at end of file
>>
>> Please ensure files have final EOL
>>
> DOne
>
>>
>> >
>> > Propchange:
>> jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
>> >
>> ------------------------------------------------------------------------------
>> > svn:mime-type = text/plain
>>
>> Also svn:eol-style native.
>>
>> > Modified:
>> jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
>> > URL:
>> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java?rev=1498398&r1=1498397&r2=1498398&view=diff
>> >
>> ==============================================================================
>> > --- jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
>> (original)
>> > +++ jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
>> Mon Jul 1 12:06:02 2013
>> > @@ -20,6 +20,8 @@ package org.apache.jmeter.gui.util;
>> >
>> > import java.awt.Component;
>> > import java.awt.event.KeyEvent;
>> > +import java.io.IOException;
>> > +import java.lang.reflect.Modifier;
>> > import java.util.ArrayList;
>> > import java.util.Collection;
>> > import java.util.Iterator;
>> > @@ -43,11 +45,14 @@ import org.apache.jmeter.gui.action.Acti
>> > import org.apache.jmeter.gui.action.ActionRouter;
>> > import org.apache.jmeter.gui.action.KeyStrokes;
>> > import org.apache.jmeter.gui.action.LoadRecentProject;
>> > +import org.apache.jmeter.gui.plugin.MenuCreator;
>> > +import org.apache.jmeter.gui.plugin.MenuCreator.MENU_LOCATION;
>> > import org.apache.jmeter.util.JMeterUtils;
>> > import org.apache.jmeter.util.LocaleChangeEvent;
>> > import org.apache.jmeter.util.LocaleChangeListener;
>> > import org.apache.jmeter.util.SSLManager;
>> > import org.apache.jorphan.logging.LoggingManager;
>> > +import org.apache.jorphan.reflect.ClassFinder;
>> > import org.apache.jorphan.util.JOrphanUtils;
>> > import org.apache.log.Logger;
>> >
>> > @@ -134,6 +139,8 @@ public class JMeterMenuBar extends JMenu
>> >
>> > private JMenu searchMenu;
>> >
>> > + private ArrayList<MenuCreator> menuCreators;
>> > +
>> > public static final String SYSTEM_LAF = "System"; // $NON-NLS-1$
>> >
>> > public static final String CROSS_PLATFORM_LAF = "CrossPlatform"; //
>> $NON-NLS-1$
>> > @@ -229,6 +236,32 @@ public class JMeterMenuBar extends JMenu
>> > * should be defined in a file somewhere, but that is for later.
>> > */
>> > public void createMenuBar() {
>> > + this.menuCreators = new ArrayList<MenuCreator>();
>> > + try {
>> > + List<String> listClasses =
>> ClassFinder.findClassesThatExtend(
>> > + JMeterUtils.getSearchPaths(),
>> > + new Class[] {MenuCreator.class });
>> > + for (String strClassName : listClasses) {
>> > + try {
>> > + if(log.isDebugEnabled()) {
>> > + log.debug("Loading menu creator class: "+
>> strClassName);
>> > + }
>> > + Class<?> commandClass = Class.forName(strClassName);
>> > + if
>> (!Modifier.isAbstract(commandClass.getModifiers())) {
>> > + if(log.isDebugEnabled()) {
>> > + log.debug("Instantiating: "+
>> commandClass.getName());
>> > + }
>> > + MenuCreator creator = (MenuCreator)
>> commandClass.newInstance();
>> > + menuCreators.add(creator);
>> > + }
>> > + } catch (Exception e) {
>> > + log.error("Exception registering
>> "+MenuCreator.class.getName() + " with implementation:"+strClassName, e);
>> > + }
>> > + }
>> > + } catch (IOException e) {
>> > + log.error("Exception finding implementations of
>> "+MenuCreator.class, e);
>> > + }
>> > +
>> > makeFileMenu();
>> > makeEditMenu();
>> > makeRunMenu();
>> > @@ -240,6 +273,13 @@ public class JMeterMenuBar extends JMenu
>> > this.add(searchMenu);
>> > this.add(runMenu);
>> > this.add(optionsMenu);
>> > + for (Iterator iterator = menuCreators.iterator();
>> iterator.hasNext();) {
>> > + MenuCreator menuCreator = (MenuCreator) iterator.next();
>> > + JMenu[] topLevelMenus = menuCreator.getTopLevelMenus();
>> > + for (JMenu topLevelMenu : topLevelMenus) {
>> > + this.add(topLevelMenu);
>> > + }
>> > + }
>> > this.add(helpMenu);
>> > }
>> >
>> > @@ -265,6 +305,9 @@ public class JMeterMenuBar extends JMenu
>> > helpMenu.add(setDebug);
>> > helpMenu.add(resetDebug);
>> > helpMenu.add(heapDump);
>> > +
>> > + addPluginsMenuItems(helpMenu, menuCreators, MENU_LOCATION.HELP);
>> > +
>> > helpMenu.addSeparator();
>> > helpMenu.add(help_about);
>> > }
>> > @@ -307,6 +350,8 @@ public class JMeterMenuBar extends JMenu
>> >
>> > JMenuItem expand = makeMenuItemRes("menu_expand_all",
>> ActionNames.EXPAND_ALL, KeyStrokes.EXPAND_ALL); //$NON-NLS-1$
>> > optionsMenu.add(expand);
>> > +
>> > + addPluginsMenuItems(optionsMenu, menuCreators,
>> MENU_LOCATION.OPTIONS);
>> > }
>> >
>> > private static class LangMenuHelper{
>> > @@ -430,6 +475,8 @@ public class JMeterMenuBar extends JMenu
>> > runMenu.addSeparator();
>> > runMenu.add(run_clear);
>> > runMenu.add(run_clearAll);
>> > +
>> > + addPluginsMenuItems(runMenu, menuCreators, MENU_LOCATION.RUN);
>> > }
>> >
>> > private void makeEditMenu() {
>> > @@ -439,6 +486,8 @@ public class JMeterMenuBar extends JMenu
>> > // From the Java Look and Feel Guidelines: If all items in a
>> menu
>> > // are disabled, then disable the menu. Makes sense.
>> > editMenu.setEnabled(false);
>> > +
>> > + addPluginsMenuItems(editMenu, menuCreators, MENU_LOCATION.EDIT);
>> > }
>> >
>> > private void makeFileMenu() {
>> > @@ -492,6 +541,9 @@ public class JMeterMenuBar extends JMenu
>> > for(JComponent jc : file_load_recent_files){
>> > fileMenu.add(jc);
>> > }
>> > +
>> > + addPluginsMenuItems(fileMenu, menuCreators, MENU_LOCATION.FILE);
>> > +
>> > fileMenu.add(file_exit);
>> > }
>> >
>> > @@ -501,11 +553,32 @@ public class JMeterMenuBar extends JMenu
>> >
>> > JMenuItem search = makeMenuItemRes("menu_search", 'F',
>> ActionNames.SEARCH_TREE, KeyStrokes.SEARCH_TREE); //$NON-NLS-1$
>> > searchMenu.add(search);
>> > - searchMenu.setEnabled(true);
>> > + search.setEnabled(true);
>>
>> Why was this changed?
>> Is it part of the new feature, or was it a separate bug fix?
>>
>> >
>> > JMenuItem searchReset = makeMenuItemRes("menu_search_reset",
>> ActionNames.SEARCH_RESET); //$NON-NLS-1$
>> > searchMenu.add(searchReset);
>> > - searchMenu.setEnabled(true);
>> > + searchReset.setEnabled(true);
>> > +
>>
>> As above.
>>
>
> I think original code was wrong so I fixed it. Maybe should have done it
> outside of this commit
Yes, please only fix related changes in a single commit.
Otherwise it's much more difficult to review the commit message, and
harder to revert if part of the change is wrong.
>>
>> > + addPluginsMenuItems(searchMenu, menuCreators,
>> MENU_LOCATION.SEARCH);
>> > + }
>> > +
>> > + /**
>> > + * @param menu
>> > + * @param menuCreators
>> > + * @param location
>> > + */
>> > + protected void addPluginsMenuItems(JMenu menu, List<MenuCreator>
>> menuCreators, MENU_LOCATION location) {
>>
>> Could this be private?
>>
>> Fixed, thanks
>
>> > + boolean addedSeparator = false;
>> > + for (MenuCreator menuCreator : menuCreators) {
>> > + JMenuItem[] menuItems =
>> menuCreator.getMenuItemsAtLocation(location);
>> > + for (JMenuItem jMenuItem : menuItems) {
>> > + if(!addedSeparator) {
>> > + menu.addSeparator();
>> > + addedSeparator = true;
>> > + }
>> > + menu.add(jMenuItem);
>> > + }
>> > + }
>> > }
>> >
>> > public void setRunning(boolean running, String host) {
>> > @@ -589,6 +662,9 @@ public class JMeterMenuBar extends JMenu
>> > updateMenuElement(runMenu);
>> > updateMenuElement(optionsMenu);
>> > updateMenuElement(helpMenu);
>> > + for (MenuCreator creator : menuCreators) {
>> > + creator.localeChanged();
>> > + }
>> > }
>> >
>> > /**
>> > @@ -618,6 +694,11 @@ public class JMeterMenuBar extends JMenu
>> > Component component = menu.getComponent();
>> > final String compName = component.getName();
>> > if (compName != null) {
>> > + for (MenuCreator menuCreator : menuCreators) {
>> > + if(menuCreator.localeChanged(menu)) {
>> > + return;
>> > + }
>> > + }
>> > if (component instanceof JMenu) {
>> > final JMenu jMenu = (JMenu) component;
>> > if (isResource(jMenu.getActionCommand())){
>> >
>> > Modified: jmeter/trunk/xdocs/changes.xml
>> > URL:
>> http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1498398&r1=1498397&r2=1498398&view=diff
>> >
>> ==============================================================================
>> > --- jmeter/trunk/xdocs/changes.xml (original)
>> > +++ jmeter/trunk/xdocs/changes.xml Mon Jul 1 12:06:02 2013
>> > @@ -221,6 +221,7 @@ Transaction Controller now sets Response
>> > <li><bugzilla>54945</bugzilla> - Add Shutdown Hook to enable trapping
>> kill or CTRL+C signals</li>
>> > <li><bugzilla>54990</bugzilla> - Download large files avoiding
>> outOfMemory</li>
>> > <li><bugzilla>55085</bugzilla> - UX Improvement : Ability to create New
>> Test Plan from Templates</li>
>> > +<li><bugzilla>55172</bugzilla> - Provide plugins a way to add Top Menu
>> and menu items</li>
>> > </ul>
>> >
>> > <h2>Non-functional changes</h2>
>> >
>> >
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.
Re: svn commit: r1498398 - in /jmeter/trunk: src/core/org/apache/jmeter/gui/action/
src/core/org/apache/jmeter/gui/plugin/ src/core/org/apache/jmeter/gui/util/ xdocs/
Posted by Philippe Mouawad <ph...@gmail.com>.
My detailed answers on your comments.
On Mon, Jul 1, 2013 at 4:39 PM, sebb <se...@gmail.com> wrote:
> On 1 July 2013 13:06, <pm...@apache.org> wrote:
> > Author: pmouawad
> > Date: Mon Jul 1 12:06:02 2013
> > New Revision: 1498398
> >
> > URL: http://svn.apache.org/r1498398
> > Log:
> > Bug 55172 - Provide plugins a way to add Top Menu and menu items
> > Bugzilla Id: 55172
>
> -1
>
> Please can we have some discussion of new features before they are added?
>
> For example, there are perhaps better ways of providing the
> functionality that don't involve class scanning.
>
> I'm not saying this is a bad feature, but we do need to consider the
> effect on the rest of JMeter before adding more code.
>
> > Added:
> > jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/
> > jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> (with props)
> > Modified:
> > jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
> > jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
> > jmeter/trunk/xdocs/changes.xml
> >
> > Modified:
> jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java?rev=1498398&r1=1498397&r2=1498398&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
> (original)
> > +++ jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
> Mon Jul 1 12:06:02 2013
> > @@ -261,8 +261,8 @@ public final class ActionRouter implemen
> > false, // innerClasses - should we include inner
> classes?
> > // contains - classname should contain this string
> > // This was added in r325814 as part of changes for
> the reporting tool
> > - "org.apache.jmeter.gui", // $NON-NLS-1$
> > - null, // notContains - classname should not contain
> this string
> > + null, // contains - classname should contain this
> string
> > + "org.apache.jmeter.report.gui", // $NON-NLS-1$ //
> notContains - classname should not contain this string
>
> That change looks wrong.
>
No I don't think it is, as it was, it was impossible for any plugin to
register Commands due to this. As per the comment , it says (This was added
in r325814 as part of changes for the reporting tool) , I think it's aim
was not to scan report related commands but the way it was done only
allowed for commands located in org.apache.jmeter.gui.
> > false); // annotations - true if classnames are
> annotations
> > commands = new HashMap<String,
> Set<Command>>(listClasses.size());
> > if (listClasses.isEmpty()) {
> >
> > Added:
> jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java?rev=1498398&view=auto
> >
> ==============================================================================
> > --- jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> (added)
> > +++ jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> Mon Jul 1 12:06:02 2013
> > @@ -0,0 +1,60 @@
> > +/*
> > + * Licensed to the Apache Software Foundation (ASF) under one or more
> > + * contributor license agreements. See the NOTICE file distributed with
> > + * this work for additional information regarding copyright ownership.
> > + * The ASF licenses this file to You under the Apache License, Version
> 2.0
> > + * (the "License"); you may not use this file except in compliance with
> > + * the License. You may obtain a copy of the License at
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + *
> > + */
> > +
> > +package org.apache.jmeter.gui.plugin;
> > +
> > +import javax.swing.JMenu;
> > +import javax.swing.JMenuItem;
> > +import javax.swing.MenuElement;
> > +
> > +/**
> > + * @since 2.10
> > + */
> > +public interface MenuCreator {
> > + public enum MENU_LOCATION {
> > + FILE,
> > + EDIT,
> > + RUN,
> > + OPTIONS,
> > + HELP,
> > + SEARCH
> > + }
> > +
> > + /**
> > + * MenuItems to be added in location menu
> > + * @param location in top menu
> > + * @return array of {@link JMenuItem}
> > + */
> > + JMenuItem[] getMenuItemsAtLocation(MENU_LOCATION location);
> > +
> > + /**
> > + * @return array of JMenu to be put as top level menu between
> Options and Help
> > + */
> > + JMenu[] getTopLevelMenus();
> > +
> > + /**
> > + * @param menu MenuElement
> > + * @return true if menu was concerned by Locale change
> > + */
> > + boolean localeChanged(MenuElement menu);
> > +
> > + /**
> > + * Update Top Level menu on Locale Change
> > + */
> > + void localeChanged();
> > +}
> > \ No newline at end of file
>
> Please ensure files have final EOL
>
DOne
>
> >
> > Propchange:
> jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> >
> ------------------------------------------------------------------------------
> > svn:mime-type = text/plain
>
> Also svn:eol-style native.
>
> > Modified:
> jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java?rev=1498398&r1=1498397&r2=1498398&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
> (original)
> > +++ jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
> Mon Jul 1 12:06:02 2013
> > @@ -20,6 +20,8 @@ package org.apache.jmeter.gui.util;
> >
> > import java.awt.Component;
> > import java.awt.event.KeyEvent;
> > +import java.io.IOException;
> > +import java.lang.reflect.Modifier;
> > import java.util.ArrayList;
> > import java.util.Collection;
> > import java.util.Iterator;
> > @@ -43,11 +45,14 @@ import org.apache.jmeter.gui.action.Acti
> > import org.apache.jmeter.gui.action.ActionRouter;
> > import org.apache.jmeter.gui.action.KeyStrokes;
> > import org.apache.jmeter.gui.action.LoadRecentProject;
> > +import org.apache.jmeter.gui.plugin.MenuCreator;
> > +import org.apache.jmeter.gui.plugin.MenuCreator.MENU_LOCATION;
> > import org.apache.jmeter.util.JMeterUtils;
> > import org.apache.jmeter.util.LocaleChangeEvent;
> > import org.apache.jmeter.util.LocaleChangeListener;
> > import org.apache.jmeter.util.SSLManager;
> > import org.apache.jorphan.logging.LoggingManager;
> > +import org.apache.jorphan.reflect.ClassFinder;
> > import org.apache.jorphan.util.JOrphanUtils;
> > import org.apache.log.Logger;
> >
> > @@ -134,6 +139,8 @@ public class JMeterMenuBar extends JMenu
> >
> > private JMenu searchMenu;
> >
> > + private ArrayList<MenuCreator> menuCreators;
> > +
> > public static final String SYSTEM_LAF = "System"; // $NON-NLS-1$
> >
> > public static final String CROSS_PLATFORM_LAF = "CrossPlatform"; //
> $NON-NLS-1$
> > @@ -229,6 +236,32 @@ public class JMeterMenuBar extends JMenu
> > * should be defined in a file somewhere, but that is for later.
> > */
> > public void createMenuBar() {
> > + this.menuCreators = new ArrayList<MenuCreator>();
> > + try {
> > + List<String> listClasses =
> ClassFinder.findClassesThatExtend(
> > + JMeterUtils.getSearchPaths(),
> > + new Class[] {MenuCreator.class });
> > + for (String strClassName : listClasses) {
> > + try {
> > + if(log.isDebugEnabled()) {
> > + log.debug("Loading menu creator class: "+
> strClassName);
> > + }
> > + Class<?> commandClass = Class.forName(strClassName);
> > + if
> (!Modifier.isAbstract(commandClass.getModifiers())) {
> > + if(log.isDebugEnabled()) {
> > + log.debug("Instantiating: "+
> commandClass.getName());
> > + }
> > + MenuCreator creator = (MenuCreator)
> commandClass.newInstance();
> > + menuCreators.add(creator);
> > + }
> > + } catch (Exception e) {
> > + log.error("Exception registering
> "+MenuCreator.class.getName() + " with implementation:"+strClassName, e);
> > + }
> > + }
> > + } catch (IOException e) {
> > + log.error("Exception finding implementations of
> "+MenuCreator.class, e);
> > + }
> > +
> > makeFileMenu();
> > makeEditMenu();
> > makeRunMenu();
> > @@ -240,6 +273,13 @@ public class JMeterMenuBar extends JMenu
> > this.add(searchMenu);
> > this.add(runMenu);
> > this.add(optionsMenu);
> > + for (Iterator iterator = menuCreators.iterator();
> iterator.hasNext();) {
> > + MenuCreator menuCreator = (MenuCreator) iterator.next();
> > + JMenu[] topLevelMenus = menuCreator.getTopLevelMenus();
> > + for (JMenu topLevelMenu : topLevelMenus) {
> > + this.add(topLevelMenu);
> > + }
> > + }
> > this.add(helpMenu);
> > }
> >
> > @@ -265,6 +305,9 @@ public class JMeterMenuBar extends JMenu
> > helpMenu.add(setDebug);
> > helpMenu.add(resetDebug);
> > helpMenu.add(heapDump);
> > +
> > + addPluginsMenuItems(helpMenu, menuCreators, MENU_LOCATION.HELP);
> > +
> > helpMenu.addSeparator();
> > helpMenu.add(help_about);
> > }
> > @@ -307,6 +350,8 @@ public class JMeterMenuBar extends JMenu
> >
> > JMenuItem expand = makeMenuItemRes("menu_expand_all",
> ActionNames.EXPAND_ALL, KeyStrokes.EXPAND_ALL); //$NON-NLS-1$
> > optionsMenu.add(expand);
> > +
> > + addPluginsMenuItems(optionsMenu, menuCreators,
> MENU_LOCATION.OPTIONS);
> > }
> >
> > private static class LangMenuHelper{
> > @@ -430,6 +475,8 @@ public class JMeterMenuBar extends JMenu
> > runMenu.addSeparator();
> > runMenu.add(run_clear);
> > runMenu.add(run_clearAll);
> > +
> > + addPluginsMenuItems(runMenu, menuCreators, MENU_LOCATION.RUN);
> > }
> >
> > private void makeEditMenu() {
> > @@ -439,6 +486,8 @@ public class JMeterMenuBar extends JMenu
> > // From the Java Look and Feel Guidelines: If all items in a
> menu
> > // are disabled, then disable the menu. Makes sense.
> > editMenu.setEnabled(false);
> > +
> > + addPluginsMenuItems(editMenu, menuCreators, MENU_LOCATION.EDIT);
> > }
> >
> > private void makeFileMenu() {
> > @@ -492,6 +541,9 @@ public class JMeterMenuBar extends JMenu
> > for(JComponent jc : file_load_recent_files){
> > fileMenu.add(jc);
> > }
> > +
> > + addPluginsMenuItems(fileMenu, menuCreators, MENU_LOCATION.FILE);
> > +
> > fileMenu.add(file_exit);
> > }
> >
> > @@ -501,11 +553,32 @@ public class JMeterMenuBar extends JMenu
> >
> > JMenuItem search = makeMenuItemRes("menu_search", 'F',
> ActionNames.SEARCH_TREE, KeyStrokes.SEARCH_TREE); //$NON-NLS-1$
> > searchMenu.add(search);
> > - searchMenu.setEnabled(true);
> > + search.setEnabled(true);
>
> Why was this changed?
> Is it part of the new feature, or was it a separate bug fix?
>
> >
> > JMenuItem searchReset = makeMenuItemRes("menu_search_reset",
> ActionNames.SEARCH_RESET); //$NON-NLS-1$
> > searchMenu.add(searchReset);
> > - searchMenu.setEnabled(true);
> > + searchReset.setEnabled(true);
> > +
>
> As above.
>
I think original code was wrong so I fixed it. Maybe should have done it
outside of this commit
>
> > + addPluginsMenuItems(searchMenu, menuCreators,
> MENU_LOCATION.SEARCH);
> > + }
> > +
> > + /**
> > + * @param menu
> > + * @param menuCreators
> > + * @param location
> > + */
> > + protected void addPluginsMenuItems(JMenu menu, List<MenuCreator>
> menuCreators, MENU_LOCATION location) {
>
> Could this be private?
>
> Fixed, thanks
> > + boolean addedSeparator = false;
> > + for (MenuCreator menuCreator : menuCreators) {
> > + JMenuItem[] menuItems =
> menuCreator.getMenuItemsAtLocation(location);
> > + for (JMenuItem jMenuItem : menuItems) {
> > + if(!addedSeparator) {
> > + menu.addSeparator();
> > + addedSeparator = true;
> > + }
> > + menu.add(jMenuItem);
> > + }
> > + }
> > }
> >
> > public void setRunning(boolean running, String host) {
> > @@ -589,6 +662,9 @@ public class JMeterMenuBar extends JMenu
> > updateMenuElement(runMenu);
> > updateMenuElement(optionsMenu);
> > updateMenuElement(helpMenu);
> > + for (MenuCreator creator : menuCreators) {
> > + creator.localeChanged();
> > + }
> > }
> >
> > /**
> > @@ -618,6 +694,11 @@ public class JMeterMenuBar extends JMenu
> > Component component = menu.getComponent();
> > final String compName = component.getName();
> > if (compName != null) {
> > + for (MenuCreator menuCreator : menuCreators) {
> > + if(menuCreator.localeChanged(menu)) {
> > + return;
> > + }
> > + }
> > if (component instanceof JMenu) {
> > final JMenu jMenu = (JMenu) component;
> > if (isResource(jMenu.getActionCommand())){
> >
> > Modified: jmeter/trunk/xdocs/changes.xml
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1498398&r1=1498397&r2=1498398&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/xdocs/changes.xml (original)
> > +++ jmeter/trunk/xdocs/changes.xml Mon Jul 1 12:06:02 2013
> > @@ -221,6 +221,7 @@ Transaction Controller now sets Response
> > <li><bugzilla>54945</bugzilla> - Add Shutdown Hook to enable trapping
> kill or CTRL+C signals</li>
> > <li><bugzilla>54990</bugzilla> - Download large files avoiding
> outOfMemory</li>
> > <li><bugzilla>55085</bugzilla> - UX Improvement : Ability to create New
> Test Plan from Templates</li>
> > +<li><bugzilla>55172</bugzilla> - Provide plugins a way to add Top Menu
> and menu items</li>
> > </ul>
> >
> > <h2>Non-functional changes</h2>
> >
> >
>
--
Cordialement.
Philippe Mouawad.
Re: svn commit: r1498398 - in /jmeter/trunk: src/core/org/apache/jmeter/gui/action/
src/core/org/apache/jmeter/gui/plugin/ src/core/org/apache/jmeter/gui/util/ xdocs/
Posted by Philippe Mouawad <ph...@gmail.com>.
Hello sebb,
I like to have discussion based on code, so it is not subject to
(mis)interpretation.
Also I didn't consider this enhancement needed it, in the past we made many
enhancements without previous discussion right ?
Also there are many items on which I asked questions without answers for
now (lack of time I suppose):
- AccessLogSampler & Bug 53748
- Apache Excalibur Logger
- Groovy
- Strange Behaviour with JavaImpl & Load Balancer
- htmlParser.className default value
- Support of websocket ?
Furthermore I don't feel what has been commited has a huge impact and it
seems to me it answers to a lack in JMeter Plugin mechanism.
In my understanding, plugin mechanism is based on class scanning which is
great as there is no need to additional configuration.
In this case unless I am wrong class scanning occurs at startup, so the
impact on performances is low right ?
If you feel there is a better way feel free to propose it and implement it .
Regards
Philippe
On Mon, Jul 1, 2013 at 4:39 PM, sebb <se...@gmail.com> wrote:
> On 1 July 2013 13:06, <pm...@apache.org> wrote:
> > Author: pmouawad
> > Date: Mon Jul 1 12:06:02 2013
> > New Revision: 1498398
> >
> > URL: http://svn.apache.org/r1498398
> > Log:
> > Bug 55172 - Provide plugins a way to add Top Menu and menu items
> > Bugzilla Id: 55172
>
> -1
>
> Please can we have some discussion of new features before they are added?
>
> For example, there are perhaps better ways of providing the
> functionality that don't involve class scanning.
>
> I'm not saying this is a bad feature, but we do need to consider the
> effect on the rest of JMeter before adding more code.
>
> > Added:
> > jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/
> > jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> (with props)
> > Modified:
> > jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
> > jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
> > jmeter/trunk/xdocs/changes.xml
> >
> > Modified:
> jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java?rev=1498398&r1=1498397&r2=1498398&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
> (original)
> > +++ jmeter/trunk/src/core/org/apache/jmeter/gui/action/ActionRouter.java
> Mon Jul 1 12:06:02 2013
> > @@ -261,8 +261,8 @@ public final class ActionRouter implemen
> > false, // innerClasses - should we include inner
> classes?
> > // contains - classname should contain this string
> > // This was added in r325814 as part of changes for
> the reporting tool
> > - "org.apache.jmeter.gui", // $NON-NLS-1$
> > - null, // notContains - classname should not contain
> this string
> > + null, // contains - classname should contain this
> string
> > + "org.apache.jmeter.report.gui", // $NON-NLS-1$ //
> notContains - classname should not contain this string
>
> That change looks wrong.
>
> > false); // annotations - true if classnames are
> annotations
> > commands = new HashMap<String,
> Set<Command>>(listClasses.size());
> > if (listClasses.isEmpty()) {
> >
> > Added:
> jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java?rev=1498398&view=auto
> >
> ==============================================================================
> > --- jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> (added)
> > +++ jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> Mon Jul 1 12:06:02 2013
> > @@ -0,0 +1,60 @@
> > +/*
> > + * Licensed to the Apache Software Foundation (ASF) under one or more
> > + * contributor license agreements. See the NOTICE file distributed with
> > + * this work for additional information regarding copyright ownership.
> > + * The ASF licenses this file to You under the Apache License, Version
> 2.0
> > + * (the "License"); you may not use this file except in compliance with
> > + * the License. You may obtain a copy of the License at
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + *
> > + */
> > +
> > +package org.apache.jmeter.gui.plugin;
> > +
> > +import javax.swing.JMenu;
> > +import javax.swing.JMenuItem;
> > +import javax.swing.MenuElement;
> > +
> > +/**
> > + * @since 2.10
> > + */
> > +public interface MenuCreator {
> > + public enum MENU_LOCATION {
> > + FILE,
> > + EDIT,
> > + RUN,
> > + OPTIONS,
> > + HELP,
> > + SEARCH
> > + }
> > +
> > + /**
> > + * MenuItems to be added in location menu
> > + * @param location in top menu
> > + * @return array of {@link JMenuItem}
> > + */
> > + JMenuItem[] getMenuItemsAtLocation(MENU_LOCATION location);
> > +
> > + /**
> > + * @return array of JMenu to be put as top level menu between
> Options and Help
> > + */
> > + JMenu[] getTopLevelMenus();
> > +
> > + /**
> > + * @param menu MenuElement
> > + * @return true if menu was concerned by Locale change
> > + */
> > + boolean localeChanged(MenuElement menu);
> > +
> > + /**
> > + * Update Top Level menu on Locale Change
> > + */
> > + void localeChanged();
> > +}
> > \ No newline at end of file
>
> Please ensure files have final EOL
>
> >
> > Propchange:
> jmeter/trunk/src/core/org/apache/jmeter/gui/plugin/MenuCreator.java
> >
> ------------------------------------------------------------------------------
> > svn:mime-type = text/plain
>
> Also svn:eol-style native.
>
> > Modified:
> jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java?rev=1498398&r1=1498397&r2=1498398&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
> (original)
> > +++ jmeter/trunk/src/core/org/apache/jmeter/gui/util/JMeterMenuBar.java
> Mon Jul 1 12:06:02 2013
> > @@ -20,6 +20,8 @@ package org.apache.jmeter.gui.util;
> >
> > import java.awt.Component;
> > import java.awt.event.KeyEvent;
> > +import java.io.IOException;
> > +import java.lang.reflect.Modifier;
> > import java.util.ArrayList;
> > import java.util.Collection;
> > import java.util.Iterator;
> > @@ -43,11 +45,14 @@ import org.apache.jmeter.gui.action.Acti
> > import org.apache.jmeter.gui.action.ActionRouter;
> > import org.apache.jmeter.gui.action.KeyStrokes;
> > import org.apache.jmeter.gui.action.LoadRecentProject;
> > +import org.apache.jmeter.gui.plugin.MenuCreator;
> > +import org.apache.jmeter.gui.plugin.MenuCreator.MENU_LOCATION;
> > import org.apache.jmeter.util.JMeterUtils;
> > import org.apache.jmeter.util.LocaleChangeEvent;
> > import org.apache.jmeter.util.LocaleChangeListener;
> > import org.apache.jmeter.util.SSLManager;
> > import org.apache.jorphan.logging.LoggingManager;
> > +import org.apache.jorphan.reflect.ClassFinder;
> > import org.apache.jorphan.util.JOrphanUtils;
> > import org.apache.log.Logger;
> >
> > @@ -134,6 +139,8 @@ public class JMeterMenuBar extends JMenu
> >
> > private JMenu searchMenu;
> >
> > + private ArrayList<MenuCreator> menuCreators;
> > +
> > public static final String SYSTEM_LAF = "System"; // $NON-NLS-1$
> >
> > public static final String CROSS_PLATFORM_LAF = "CrossPlatform"; //
> $NON-NLS-1$
> > @@ -229,6 +236,32 @@ public class JMeterMenuBar extends JMenu
> > * should be defined in a file somewhere, but that is for later.
> > */
> > public void createMenuBar() {
> > + this.menuCreators = new ArrayList<MenuCreator>();
> > + try {
> > + List<String> listClasses =
> ClassFinder.findClassesThatExtend(
> > + JMeterUtils.getSearchPaths(),
> > + new Class[] {MenuCreator.class });
> > + for (String strClassName : listClasses) {
> > + try {
> > + if(log.isDebugEnabled()) {
> > + log.debug("Loading menu creator class: "+
> strClassName);
> > + }
> > + Class<?> commandClass = Class.forName(strClassName);
> > + if
> (!Modifier.isAbstract(commandClass.getModifiers())) {
> > + if(log.isDebugEnabled()) {
> > + log.debug("Instantiating: "+
> commandClass.getName());
> > + }
> > + MenuCreator creator = (MenuCreator)
> commandClass.newInstance();
> > + menuCreators.add(creator);
> > + }
> > + } catch (Exception e) {
> > + log.error("Exception registering
> "+MenuCreator.class.getName() + " with implementation:"+strClassName, e);
> > + }
> > + }
> > + } catch (IOException e) {
> > + log.error("Exception finding implementations of
> "+MenuCreator.class, e);
> > + }
> > +
> > makeFileMenu();
> > makeEditMenu();
> > makeRunMenu();
> > @@ -240,6 +273,13 @@ public class JMeterMenuBar extends JMenu
> > this.add(searchMenu);
> > this.add(runMenu);
> > this.add(optionsMenu);
> > + for (Iterator iterator = menuCreators.iterator();
> iterator.hasNext();) {
> > + MenuCreator menuCreator = (MenuCreator) iterator.next();
> > + JMenu[] topLevelMenus = menuCreator.getTopLevelMenus();
> > + for (JMenu topLevelMenu : topLevelMenus) {
> > + this.add(topLevelMenu);
> > + }
> > + }
> > this.add(helpMenu);
> > }
> >
> > @@ -265,6 +305,9 @@ public class JMeterMenuBar extends JMenu
> > helpMenu.add(setDebug);
> > helpMenu.add(resetDebug);
> > helpMenu.add(heapDump);
> > +
> > + addPluginsMenuItems(helpMenu, menuCreators, MENU_LOCATION.HELP);
> > +
> > helpMenu.addSeparator();
> > helpMenu.add(help_about);
> > }
> > @@ -307,6 +350,8 @@ public class JMeterMenuBar extends JMenu
> >
> > JMenuItem expand = makeMenuItemRes("menu_expand_all",
> ActionNames.EXPAND_ALL, KeyStrokes.EXPAND_ALL); //$NON-NLS-1$
> > optionsMenu.add(expand);
> > +
> > + addPluginsMenuItems(optionsMenu, menuCreators,
> MENU_LOCATION.OPTIONS);
> > }
> >
> > private static class LangMenuHelper{
> > @@ -430,6 +475,8 @@ public class JMeterMenuBar extends JMenu
> > runMenu.addSeparator();
> > runMenu.add(run_clear);
> > runMenu.add(run_clearAll);
> > +
> > + addPluginsMenuItems(runMenu, menuCreators, MENU_LOCATION.RUN);
> > }
> >
> > private void makeEditMenu() {
> > @@ -439,6 +486,8 @@ public class JMeterMenuBar extends JMenu
> > // From the Java Look and Feel Guidelines: If all items in a
> menu
> > // are disabled, then disable the menu. Makes sense.
> > editMenu.setEnabled(false);
> > +
> > + addPluginsMenuItems(editMenu, menuCreators, MENU_LOCATION.EDIT);
> > }
> >
> > private void makeFileMenu() {
> > @@ -492,6 +541,9 @@ public class JMeterMenuBar extends JMenu
> > for(JComponent jc : file_load_recent_files){
> > fileMenu.add(jc);
> > }
> > +
> > + addPluginsMenuItems(fileMenu, menuCreators, MENU_LOCATION.FILE);
> > +
> > fileMenu.add(file_exit);
> > }
> >
> > @@ -501,11 +553,32 @@ public class JMeterMenuBar extends JMenu
> >
> > JMenuItem search = makeMenuItemRes("menu_search", 'F',
> ActionNames.SEARCH_TREE, KeyStrokes.SEARCH_TREE); //$NON-NLS-1$
> > searchMenu.add(search);
> > - searchMenu.setEnabled(true);
> > + search.setEnabled(true);
>
> Why was this changed?
> Is it part of the new feature, or was it a separate bug fix?
>
> >
> > JMenuItem searchReset = makeMenuItemRes("menu_search_reset",
> ActionNames.SEARCH_RESET); //$NON-NLS-1$
> > searchMenu.add(searchReset);
> > - searchMenu.setEnabled(true);
> > + searchReset.setEnabled(true);
> > +
>
> As above.
>
> > + addPluginsMenuItems(searchMenu, menuCreators,
> MENU_LOCATION.SEARCH);
> > + }
> > +
> > + /**
> > + * @param menu
> > + * @param menuCreators
> > + * @param location
> > + */
> > + protected void addPluginsMenuItems(JMenu menu, List<MenuCreator>
> menuCreators, MENU_LOCATION location) {
>
> Could this be private?
>
> > + boolean addedSeparator = false;
> > + for (MenuCreator menuCreator : menuCreators) {
> > + JMenuItem[] menuItems =
> menuCreator.getMenuItemsAtLocation(location);
> > + for (JMenuItem jMenuItem : menuItems) {
> > + if(!addedSeparator) {
> > + menu.addSeparator();
> > + addedSeparator = true;
> > + }
> > + menu.add(jMenuItem);
> > + }
> > + }
> > }
> >
> > public void setRunning(boolean running, String host) {
> > @@ -589,6 +662,9 @@ public class JMeterMenuBar extends JMenu
> > updateMenuElement(runMenu);
> > updateMenuElement(optionsMenu);
> > updateMenuElement(helpMenu);
> > + for (MenuCreator creator : menuCreators) {
> > + creator.localeChanged();
> > + }
> > }
> >
> > /**
> > @@ -618,6 +694,11 @@ public class JMeterMenuBar extends JMenu
> > Component component = menu.getComponent();
> > final String compName = component.getName();
> > if (compName != null) {
> > + for (MenuCreator menuCreator : menuCreators) {
> > + if(menuCreator.localeChanged(menu)) {
> > + return;
> > + }
> > + }
> > if (component instanceof JMenu) {
> > final JMenu jMenu = (JMenu) component;
> > if (isResource(jMenu.getActionCommand())){
> >
> > Modified: jmeter/trunk/xdocs/changes.xml
> > URL:
> http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1498398&r1=1498397&r2=1498398&view=diff
> >
> ==============================================================================
> > --- jmeter/trunk/xdocs/changes.xml (original)
> > +++ jmeter/trunk/xdocs/changes.xml Mon Jul 1 12:06:02 2013
> > @@ -221,6 +221,7 @@ Transaction Controller now sets Response
> > <li><bugzilla>54945</bugzilla> - Add Shutdown Hook to enable trapping
> kill or CTRL+C signals</li>
> > <li><bugzilla>54990</bugzilla> - Download large files avoiding
> outOfMemory</li>
> > <li><bugzilla>55085</bugzilla> - UX Improvement : Ability to create New
> Test Plan from Templates</li>
> > +<li><bugzilla>55172</bugzilla> - Provide plugins a way to add Top Menu
> and menu items</li>
> > </ul>
> >
> > <h2>Non-functional changes</h2>
> >
> >
>
--
Cordialement.
Philippe Mouawad.