Mailing-List: contact dev-help@jmeter.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@jmeter.apache.org
Received-SPF: pass (nike.apache.org: domain of sebbaz@gmail.com designates
 74.125.82.43 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <20130701120603.21F3823889D5@eris.apache.org>
References: <20130701120603.21F3823889D5@eris.apache.org>
Date: Mon, 1 Jul 2013 15:39:54 +0100
Message-ID: 
 <CAOGo0VYoqr_iuC9Tn+_pxdUV-pW6DPFP-WtGFxKz=ffRvKArYg@mail.gmail.com>
Subject: 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/
From: sebb <sebbaz@gmail.com>
To: dev@jmeter.apache.org
Content-Type: text/plain; charset=ISO-8859-1

On 1 July 2013 13:06,  <pmouawad@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>
>
>