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