jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@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 Fri, 05 Jul 2013 09:53:12 GMT
On 1 July 2013 16:02, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> 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.

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.

Mime
View raw message