jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Mouawad <philippe.moua...@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/
Date Mon, 01 Jul 2013 15:02:33 GMT
My detailed answers on your comments.

On Mon, Jul 1, 2013 at 4:39 PM, sebb <sebbaz@gmail.com> wrote:

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

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.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message