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 <[email protected]> wrote: > On 1 July 2013 13:06, <[email protected]> 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.
