New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Edit menu allows ZAP mode change this closes zaproxy/zaproxy#2375 #2837

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
3 participants
@rajiv65
Copy link
Contributor

rajiv65 commented Aug 30, 2016

  • Edit menu now has an option to change ZAP mode. Mode can now be changed even if the main toolbar is hidden.
  • Used checkboxes for mode selection instead of a radio box to maintain GUI consistency.
  • There's one issue, however. The display in the MainToolBar panel which shows the ZAP mode and allows for user to select from a drop down list doesn't change when the user changes the mode from the Edit menu. I couldn't to do this as I wasn't able to figure out how to get a reference to the toolbar from the ExtensionState.java class. If anyone could help me out with that, I am willing to finish that as well.

Please review the PR and suggest changes if any. Thanks.

}
modeCount = 0;
for(Mode modeType : Mode.values()){
addZAPModeEventListeners(modeType,menuItemsMode[modeCount],menuItemsMode);

This comment has been minimized.

@kingthorin

kingthorin Aug 30, 2016

Member

Could this not be done inside the loop above? Instead of looping through the same elements twice?

This comment has been minimized.

@rajiv65

rajiv65 Aug 30, 2016

Contributor

I need all elements of my array of checkboxes i.e. menuItemsMode[] to be set before I pass the array as an argument to my addZAPModeEventListeners() function. This is because whenever I click on either of the modes, I am first deselecting all the checkboxes and then checking the correct one. So, I need all my menu items to be there before I add any eventListener and hence, before adding event listeners, I am first adding all the checkbox items and then I am looping again to add eventListeners. It can be done easily with a radio button type of menu but then the GUI would look different compared to other menu items in ZAP.

This comment has been minimized.

@kingthorin

kingthorin Aug 30, 2016

Member

Fair enough.

This comment has been minimized.

@thc202

thc202 Aug 31, 2016

Member

It could be used a ButtonGroup instead, which already takes care to keep just one of the buttons selected.


private void addZAPModeEventListeners(final Mode modeType, final JCheckBoxMenuItem checkBox, final JCheckBoxMenuItem[] menuItemsMode){
checkBox.addActionListener(new java.awt.event.ActionListener() {
public void actionPerformed(java.awt.event.ActionEvent evt) {

This comment has been minimized.

@kingthorin

kingthorin Aug 30, 2016

Member

Should have an @Override annotation?

This comment has been minimized.

@rajiv65

rajiv65 Aug 30, 2016

Contributor

Sorry but I couldn't get you. As far as I know, @override is used when we override a parent class method. I don't think I have overriden any other parent method here. Correct me if I am wrong. Thanks.

This comment has been minimized.

@kingthorin

kingthorin Aug 30, 2016

Member

I don't have an IDE in front of me. But, aren't you overriding actionPerformed on the checkbox? I'm kind of trusting google here (since no IDE).... http://www.codejava.net/java-se/swing/jcheckbox-basic-tutorial-and-examples#AddListeners

@kingthorin

This comment has been minimized.

Copy link
Member

kingthorin commented Aug 30, 2016

Please ensure you change is documented with a //ZAP comment since this modifies a Paros source file (ExtensionState.java).
Per https://github.com/zaproxy/zaproxy/wiki/DevGuidelines .

As for updating the MainToolBar....I'm guessing: Check View.isInitialised(), then View.getSingleton().getMainFrame(), get the ToolbarPanel (

public MainToolbarPanel getMainToolbarPanel() {
) get the toolbar and setMode on it? (
public void setMode (final Mode mode) {
)

@rajiv65

This comment has been minimized.

Copy link
Contributor

rajiv65 commented Aug 30, 2016

I will look into the toolbar as you have suggested and also document the change of the Paros source file and get back to you if there's any issue. Thanks.

int modeCount = 0;
for (Mode modeType : Mode.values()) {
menuItemsMode[modeCount] = addZAPModeMenuItem(modeType);
menuZAPMode.add(menuItemsMode[modeCount]);

This comment has been minimized.

@kingthorin

kingthorin Aug 30, 2016

Member

Something going on with the indentation here...

This comment has been minimized.

@rajiv65

rajiv65 Aug 30, 2016

Contributor

Didn't look like that in my local copy. Not sure how that happened. Guess my text editor is at fault here. I'll change this.

This comment has been minimized.

@kingthorin

kingthorin Aug 30, 2016

Member

Could be spaces vs tabs within the file, depending on your editor's setting.

@rajiv65

This comment has been minimized.

Copy link
Contributor

rajiv65 commented Aug 30, 2016

Please review the latest changes. Thanks.

  • Added the override annotation for the actionPerformed() method in the event listener.
  • Documented the change in the ExtensionState.java file as per the ZAP guidelines for a change in Paros source files.
  • The ZAP mode change from edit menu is reflected in the toolbar as well now.
  • Fixed indentation issues.
@rajiv65

This comment has been minimized.

Copy link
Contributor

rajiv65 commented Aug 31, 2016

@kingthorin Anything else you want me to add or change?

@kingthorin

This comment has been minimized.

Copy link
Member

kingthorin commented Aug 31, 2016

One minor thing, the indentation of the loops, the loops should actually "contain" the code....know what I mean?

Then would ask that you do an interactive rebase and fix-up or Squash into a single "atomic" commit: https://github.com/zaproxy/zaproxy/blob/develop/CONTRIBUTING.md#what-we-zap-team-expect-from-you .

@psiinon, @thc202 do you see anything that needs to be changed/addressed?

@rajiv65

This comment has been minimized.

Copy link
Contributor

rajiv65 commented Aug 31, 2016

Yes. I got what you said. Will indent the code inside the loops. And sorry about all these things that I am overlooking. I am relatively new to open source contribution and am still learning. Thanks for pointing out my mistakes. :)

}

private JCheckBoxMenuItem addZAPModeMenuItem(Mode modeType){
final JCheckBoxMenuItem modeItem = new JCheckBoxMenuItem(modeType.name().substring(0,1).toUpperCase()+modeType.name().substring(1)+" Mode");

This comment has been minimized.

@thc202

thc202 Aug 31, 2016

Member

This should be internationalised, since there are already resource messages (e.g. "view.toolbar.mode.safe.select") it could use those.

@thc202

This comment has been minimized.

Copy link
Member

thc202 commented Aug 31, 2016

I don't see any problem using JRadioButtonMenuItem, that's the expected UI component when only one of the options can be selected at any given time.

The selected state of the modes in the edit menu does not reflect the correct mode when changed through the main tool bar.

Why ExtensionState? That extension is about/for session (cookie) management, which is unrelated to the Mode, moreover if the extension is disabled the option would not be shown. Since the Mode is a built-in core functionality it should probably be added by the View (or other "core" class).

@thc202

This comment has been minimized.

Copy link
Member

thc202 commented Aug 31, 2016

Also, the class CoreAPI might need some changes (when processing the endpoint ACTION_SET_MODE).
The ExtensionUiUtils could be changed to update the state of the UI components when the Mode changes (instead of having some of the code conditionally calling the UI components directly).

@thc202 thc202 added the Type-Task label Aug 31, 2016

@kingthorin

This comment has been minimized.

Copy link
Member

kingthorin commented Aug 31, 2016

@rajiv65 no need to apologize at all. Lots of details and it always takes time to get used to.

As you can see form @thc202's comments there are things I missed too 😄

@@ -52,6 +60,8 @@
private JCheckBoxMenuItem menuSessionTrackingEnable = null;

private ZapMenuItem menuResetSessionState = null;
private JCheckBoxMenuItem[] menuItemsMode = new JCheckBoxMenuItem[Mode.values().length];

This comment has been minimized.

@thc202

thc202 Aug 31, 2016

Member

UI related components should not be initialised unless ZAP has GUI (there's the initView method for these kind of initialisations or in the hook method, if there's a view).

EDIT: Although this is not really needed if using a ButtonGroup.


package org.parosproxy.paros.extension.state;

import javax.swing.JCheckBoxMenuItem;
import javax.swing.JOptionPane;
import javax.swing.JMenu;
import javax.swing.event.MenuEvent;

This comment has been minimized.

@thc202

thc202 Aug 31, 2016

Member

Unused imports should be removed or not introduced (MenuEvent, MenuListener and the added Control.Mode).

@rajiv65

This comment has been minimized.

Copy link
Contributor

rajiv65 commented Aug 31, 2016

Okay. So I have got rid of the redundant checkboxes and used radio buttons and button group as @thc202 suggested. I also incorporated the other changes as suggested. Just one more thing.
Will ExtensionEdit.java be a good place to add this change ZAP mode menu item? I see that the Find option of the Edit menu is added here, which I believe is also a built-in core functionality just like ZAP mode. If not, where do you suggest I add the code for this feature.

@thc202

This comment has been minimized.

Copy link
Member

thc202 commented Aug 31, 2016

I'd add that in MainMenuBar (as the combo box is directly added to/by the MainToolbarPanel), that prevents removing/hiding the mode menu by just disabling an extension.

@rajiv65

This comment has been minimized.

Copy link
Contributor

rajiv65 commented Sep 1, 2016

Please review the latest commit @thc202 @kingthorin . I'll squash all the commits into one once I incorporate any other suggested changes given by you, if any. Thanks.

  • Shifted code to add "change ZAP mode" option of edit menu to MainMenuBar.java
  • Used radio buttons and button group instead of checkboxes
  • Selected state of the modes in the edit menu now reflects the correct mode when changed through the main tool bar.
  • Edited CoreAPI class for processing the endpoint ACTION_SET_MODE
  • Internationalised menu items names using resource messages
  • Removed unnecessary imports
  • Documented the change in the MainMenuBar.java file as per the ZAP guidelines for a change in Paros source files.
@@ -52,7 +52,7 @@
private JCheckBoxMenuItem menuSessionTrackingEnable = null;

private ZapMenuItem menuResetSessionState = null;

This comment has been minimized.

@kingthorin

kingthorin Sep 1, 2016

Member

If you could revert these formatting changes that'd be great.

https://github.com/zaproxy/zaproxy/wiki/DevGuidelines

@@ -71,7 +71,6 @@ public void hook(ExtensionHook extensionHook) {
extensionHook.getHookMenu().addEditMenuItem(getMenuResetSessionState());
}
}

This comment has been minimized.

@kingthorin
@rajiv65

This comment has been minimized.

Copy link
Contributor

rajiv65 commented Sep 1, 2016

Reverted the formatting changes. Please review the remaining code and let me know if any changes are to be done. @thc202 @kingthorin

@@ -75,6 +87,8 @@
private JMenu menuHelp = null;
private ZapMenuItem menuHelpAbout = null;
private JMenu menuAnalyse = null;
private JMenu menuZAPMode = null;
ButtonGroup menuZAPModeGroup = null;

This comment has been minimized.

@kingthorin

kingthorin Sep 2, 2016

Member

private ?

This comment has been minimized.

@rajiv65

rajiv65 Sep 2, 2016

Contributor

Forgot to add. Did it now.

}

private JRadioButtonMenuItem addZAPModeMenuItem(final Mode modeType){
final JRadioButtonMenuItem modeItem = new JRadioButtonMenuItem(""+Constant.messages.getString("view.toolbar.mode."+modeType.name()+".select"));

This comment has been minimized.

@kingthorin

kingthorin Sep 2, 2016

Member

Is the leading empty string necessary in the concatenation?

This comment has been minimized.

@rajiv65

rajiv65 Sep 2, 2016

Contributor

Not necessary. Removed it now.

@@ -178,6 +178,9 @@ public void actionPerformed(java.awt.event.ActionEvent e) {
default: return; // Not recognised
}
Control.getSingleton().setMode(mode);
if (View.isInitialised()){
View.getSingleton().getMainFrame().getMainMenuBar().setMode(mode);

This comment has been minimized.

@kingthorin

kingthorin Sep 2, 2016

Member

This should be indented

This comment has been minimized.

@rajiv65

rajiv65 Sep 2, 2016

Contributor

Done.

}



This comment has been minimized.

@kingthorin

kingthorin Sep 2, 2016

Member

Two of these lines could be removed

This comment has been minimized.

@rajiv65

rajiv65 Sep 2, 2016

Contributor

Removed the unnecessary lines.

@rajiv65

This comment has been minimized.

Copy link
Contributor

rajiv65 commented Sep 2, 2016

Added the necessary changes. Anything else that needs to be addressed? Thanks.

@kingthorin

This comment has been minimized.

Copy link
Member

kingthorin commented Sep 2, 2016

Just committing those changes and fixing this up into a single commit.

#2837 (comment)

@rajiv65 rajiv65 force-pushed the rajiv65:develop branch from 14d6a05 to ad37fe8 Sep 2, 2016

@rajiv65

This comment has been minimized.

Copy link
Contributor

rajiv65 commented Sep 2, 2016

@thc202 @kingthorin Squashed all changes into one commit. Please review it and let me know. Thanks.


import org.parosproxy.paros.control.Control.Mode;
import org.parosproxy.paros.view.View;
import org.parosproxy.paros.control.Control;

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

This class was already imported.

@@ -114,10 +128,57 @@ private void initialize() {
menuEdit = new javax.swing.JMenu();
menuEdit.setText(Constant.messages.getString("menu.edit")); // ZAP: i18n
menuEdit.setMnemonic(Constant.messages.getChar("menu.edit.mnemonic"));
menuEdit.add(getMenuEditZAPMode());

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

Might worth add a separator after this entry.

if (menuZAPMode == null) {
menuZAPMode = new JMenu(Constant.messages.getString("menu.edit.zapmode"));
menuZAPModeGroup = new ButtonGroup();
JRadioButtonMenuItem newButton = null;

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

The null initialisation can be removed.

menuZAPModeGroup = new ButtonGroup();
JRadioButtonMenuItem newButton = null;
for (Mode modeType : Mode.values()) {
newButton = addZAPModeMenuItem(modeType);

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

In this and following two lines there are some spaces at the beginning of the statements and between the tabs.

final JRadioButtonMenuItem modeItem = new JRadioButtonMenuItem(Constant.messages.getString("view.toolbar.mode."+modeType.name()+".select"));
modeItem.addActionListener(new java.awt.event.ActionListener() {
@Override
public void actionPerformed(java.awt.event.ActionEvent evt) {

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

This method should be indented one more level to the right.

modeItem.addActionListener(new java.awt.event.ActionListener() {
@Override
public void actionPerformed(java.awt.event.ActionEvent evt) {
Control.getSingleton().setMode(modeType);

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

Same comment regarding the spaces between the tabs.

@Override
public void actionPerformed(java.awt.event.ActionEvent evt) {
Control.getSingleton().setMode(modeType);
if (View.isInitialised()){

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

The if statement can be removed, the view is initialised (it's view code that will call this method).

@Override
public void run() {
Enumeration<AbstractButton> elements = menuZAPModeGroup.getElements();
AbstractButton button = null;

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

The null initialisation can be removed.

Enumeration<AbstractButton> elements = menuZAPModeGroup.getElements();
AbstractButton button = null;
while (elements.hasMoreElements()) {
button = elements.nextElement();

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

Mixed indentation chars (tabs and spaces).

AbstractButton button = null;
while (elements.hasMoreElements()) {
button = elements.nextElement();
if (button.getText().toLowerCase().contains(mode.name().toString().toLowerCase())) {

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

This will not work properly when the modes are translated (already are in some of the languages).

This comment has been minimized.

@rajiv65

rajiv65 Sep 30, 2016

Contributor

@thc202 Any suggestions on how do I go about correcting this?

This comment has been minimized.

@thc202

thc202 Oct 3, 2016

Member

It could use a Map<Mode, JRadioButtonMenuItem>, then it just needs to get the menu (instead of iterate the menus).

@@ -429,6 +429,7 @@ public void run() {
Mode mode = Mode.valueOf(params.getString(PARAM_MODE).toLowerCase());
if (View.isInitialised()) {
View.getSingleton().getMainFrame().getMainToolbarPanel().setMode(mode);
View.getSingleton().getMainFrame().getMainMenuBar().setMode(mode);

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

This has side effect of setting the mode twice (same when changing the mode through the Edit menu). The method Control.setMode(Mode) could be changed to ignore the mode (e.g. not notify of the change nor persist...) when already set.

@@ -178,6 +178,9 @@ public void actionPerformed(java.awt.event.ActionEvent e) {
default: return; // Not recognised
}
Control.getSingleton().setMode(mode);
if (View.isInitialised()){

This comment has been minimized.

@thc202

thc202 Sep 7, 2016

Member

The if statement can be removed, the view is initialised (it's view code that will call this method).

@thc202

This comment has been minimized.

Copy link
Member

thc202 commented Sep 7, 2016

Comments added, sorry for the delay.

@thc202

This comment has been minimized.

Copy link
Member

thc202 commented Sep 7, 2016

btw, the commit message still has redundant/unnecessary info (e.g. one close is enough, some info is not interesting for the history).

@kingthorin

This comment has been minimized.

Copy link
Member

kingthorin commented Nov 24, 2016

@rajiv65 are you able to address @thc202's feedback?

@kingthorin

This comment has been minimized.

Copy link
Member

kingthorin commented Dec 23, 2016

Could you set this PR (branch) to allow modification by maintainers?
https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

@kingthorin

This comment has been minimized.

Copy link
Member

kingthorin commented Feb 11, 2017

@rajiv65 if you're unable to address the feedback could you please set this PR (branch) to allow modification by maintainers?
https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

@kingthorin

This comment has been minimized.

Copy link
Member

kingthorin commented Apr 19, 2017

I guess this is abandon?

@kingthorin kingthorin self-assigned this Jun 2, 2017

@kingthorin

This comment has been minimized.

Copy link
Member

kingthorin commented Jun 2, 2017

Tried to address conflict on this, couldn't push the changes back to the user repo. Have sent an email.

@thc202

This comment has been minimized.

Copy link
Member

thc202 commented Jun 26, 2017

@kingthorin open a new pull request with the latest changes.

@thc202 thc202 closed this Jun 26, 2017

kingthorin added a commit to kingthorin/zaproxy that referenced this pull request Jun 26, 2017

Edit menu allows ZAP mode change
This is a core dev pickup of
zaproxy#2837 which has been abandon and
we've been unable to get in-touch with the original submitter
(@rajiv65).

Edit menu now has an option to change ZAP mode. Mode can now be changed
even if the main toolbar is hidden.

kingthorin added a commit to kingthorin/zaproxy that referenced this pull request Jun 26, 2017

Edit menu allows ZAP mode change
This is a core dev pickup of
zaproxy#2837 which has been abandon and
we've been unable to get in-touch with the original submitter
(@rajiv65).

Edit menu now has an option to change ZAP mode. Mode can now be changed
even if the main toolbar is hidden.

kingthorin added a commit to kingthorin/zaproxy that referenced this pull request Jun 26, 2017

Edit menu allows ZAP mode change
This is a core dev pickup of
zaproxy#2837 which has been abandon and
we've been unable to get in-touch with the original submitter
(@rajiv65).

Edit menu now has an option to change ZAP mode. Mode can now be changed
even if the main toolbar is hidden.

kingthorin added a commit to kingthorin/zaproxy that referenced this pull request Jun 26, 2017

Edit menu allows ZAP mode change
This is a core dev pickup of
zaproxy#2837 which has been abandon and
we've been unable to get in-touch with the original submitter
(@rajiv65).

Edit menu now has an option to change ZAP mode. Mode can now be changed
even if the main toolbar is hidden.

kingthorin added a commit to kingthorin/zaproxy that referenced this pull request Jun 28, 2017

Edit menu allows ZAP mode change
This is a core dev pickup of
zaproxy#2837 which has been abandon and
we've been unable to get in-touch with the original submitter
(@rajiv65).

Edit menu now has an option to change ZAP mode. Mode can now be changed
even if the main toolbar is hidden.

kingthorin added a commit to kingthorin/zaproxy that referenced this pull request Jun 28, 2017

Edit menu allows ZAP mode change
This is a core dev pickup of
zaproxy#2837 which has been abandon and
we've been unable to get in-touch with the original submitter
(@rajiv65).

Edit menu now has an option to change ZAP mode. Mode can now be changed
even if the main toolbar is hidden.

Fixes zaproxy#2375

juhakivekas added a commit to juhakivekas/zaproxy that referenced this pull request Sep 29, 2017

Edit menu allows ZAP mode change
This is a core dev pickup of
zaproxy#2837 which has been abandon and
we've been unable to get in-touch with the original submitter
(@rajiv65).

Edit menu now has an option to change ZAP mode. Mode can now be changed
even if the main toolbar is hidden.

Fixes zaproxy#2375
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment