Skip to content
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

Kent branch Add more functionality on right click popup menu #22

Open
wants to merge 5 commits into
base: develop_smarttool
Choose a base branch
from

Conversation

kent0318
Copy link
Member

No description provided.

panel.repaint();
}

@Override
public String locationString() {
// TODO Auto-generated method stub
return ActionsMenuBarTitles.Data().Remove().toString(); }

if(panel.getSelectTool() instanceof SmartTool)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach (using instanceof) is dangerous, Please Comment on these two lines on the reason for selection.

Instanceof makes code unmaintanable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make a comment on the approach taken,
Include the following line:

//TODO RECONSIDER THIS DESIGN

Copy link
Member

@UltimatePea UltimatePea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reconsider the design.

There are two broad categories of paint actions, the menu ones and the non-menu ones. I suggest make 4 methods in the PaintAction abstract class.

public boolean isMenuAction();
public boolean isPopupAction();
public String getLocationStringForMenu();
public String getLocationStringForPopup();

An action may be both, either or neither of the above mentioned actions.

@UltimatePea UltimatePea changed the base branch from develop to develop_smarttool April 22, 2017 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants