Skip to content

Refactor Operation Interface #586

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 23, 2016
Merged

Refactor Operation Interface #586

merged 4 commits into from
May 23, 2016

Conversation

@JLLeitschuh
Copy link
Member

@JLLeitschuh JLLeitschuh commented May 19, 2016

  • Operations now use local state variables for input and output sockets rather than regenerating them on the fly
  • Moved name, description, category, icon, and aliases to a separate description object
  • Socket classes are now interfaces
  • Refactors Generated Operations to be in source code
  • Changed socket get methods to return List instead of FooSocket<?>[]

SamCarlberg
SamCarlberg reviewed May 19, 2016
View reviewed changes
build.gradle Outdated
removeExisting true
}
generateCodeFromSource.onlyIf { !project.hasProperty('skipGenerate') }
//generateCodeFromSource.onlyIf { false }
Copy link
Member

@SamCarlberg SamCarlberg May 19, 2016

Choose a reason for hiding this comment

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

Remove this line

@JLLeitschuh JLLeitschuh force-pushed the refactor/operation-api branch from 7529a23 to 347db13 Compare May 19, 2016 16:15
SamCarlberg
SamCarlberg reviewed May 19, 2016
View reviewed changes
...in/java/edu/wpi/grip/core/operations/opencv/templated/FiveSourceOneDestinationOperation.java Outdated
import java.util.List;


public class FiveSourceOneDestinationOperation<T1, T2, T3, T4, T5, R> implements Operation {
Copy link
Member

@SamCarlberg SamCarlberg May 19, 2016

Choose a reason for hiding this comment

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

Add documentation for how and when to use this

@JLLeitschuh JLLeitschuh force-pushed the refactor/operation-api branch 2 times, most recently from 8113b8f to d536300 Compare May 19, 2016 21:04
@SamCarlberg
Copy link
Member

SamCarlberg commented May 20, 2016

For some reason, most operations in the palette don't display their images.

SamCarlberg
SamCarlberg reviewed May 20, 2016
View reviewed changes
core/src/main/java/edu/wpi/grip/core/operations/Operations.java Outdated
new OperationMetaData(NewPointOperation.DESCRIPTION, () -> new NewPointOperation(isf, osf)),
new OperationMetaData(NewSizeOperation.DESCRIPTION, () -> new NewSizeOperation(isf, osf)),

// Raw OpenCV operations
Copy link
Member

@SamCarlberg SamCarlberg May 20, 2016

Choose a reason for hiding this comment

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

Remove this comment and associated newlines since OpenCV operations are in their own file

@SamCarlberg
Copy link
Member

SamCarlberg commented May 20, 2016

Missing operations:

ROSPublish ContoursReport
ROSPublish BlobsReport
ROSPublish LinesReport
Threshold Moving
CV Erode

@JLLeitschuh JLLeitschuh force-pushed the refactor/operation-api branch 3 times, most recently from 45b9c26 to 6abb294 Compare May 21, 2016 01:43
AustinShalit
AustinShalit reviewed May 21, 2016
View reviewed changes
core/src/main/java/edu/wpi/grip/core/Connection.java Outdated
for (OutputSocket socket : e.getStep().getOutputSockets()) {
if (socket == this.outputSocket) {
this.eventBus.post(new ConnectionRemovedEvent(this));
this.remove();
Copy link
Member

@AustinShalit AustinShalit May 21, 2016

Choose a reason for hiding this comment

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

Can you remove this?

Copy link
Member

@AustinShalit AustinShalit May 21, 2016

Choose a reason for hiding this comment

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

See my note on line 97

AustinShalit
AustinShalit reviewed May 21, 2016
View reviewed changes
core/src/main/java/edu/wpi/grip/core/operations/composite/ValveOperation.java Outdated
public InputSocket<?>[] createInputSockets(EventBus eventBus) {
throw new UnsupportedOperationException("This method should not be used");
public List<InputSocket> getInputSockets() {
return ImmutableList.of(switcherSocket, inputSocket);
Copy link
Member

@AustinShalit AustinShalit May 21, 2016

Choose a reason for hiding this comment

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

Style with new lines

@AustinShalit
Copy link
Member

AustinShalit commented May 21, 2016

Regarding the icon issue: InputStreams can only be read once. Two ways to solve it:

  1. Mark the stream before you read it and then reset it every time you are done reading the stream
  2. Use the stream once to save the information to an Image object

@SamCarlberg
Copy link
Member

SamCarlberg commented May 21, 2016

Huh, I figured that the constructor would close the stream. Should just change the icons utility class to return the icons (and cache them in memory) instead of streams.

@SamCarlberg @JLLeitschuh
Refactor Operation Interface
3cdd6e0 
- Operations now use local state variables for input and output sockets rather than regenerating them on the fly
- Moved name, description, category, icon, and aliases to a separate description object
- Socket classes are now interfaces
- Refactors Generated Operations to be in source code
- Changed socket get methods to return List<FooSocket> instead of FooSocket<?>[]
@JLLeitschuh JLLeitschuh force-pushed the refactor/operation-api branch from 6abb294 to 3cdd6e0 Compare May 21, 2016 21:20
WPIRoboticsProjects-Bot
WPIRoboticsProjects-Bot reviewed May 23, 2016
View reviewed changes
core/src/main/java/edu/wpi/grip/core/operations/network/ros/ROSPublishOperation.java
protected final String getNetworkProtocolNameAcronym() {
return "ROS";
}
private final JavaToMessageConverter<D, ?> converter;
Copy link

@WPIRoboticsProjects-Bot WPIRoboticsProjects-Bot May 23, 2016

Choose a reason for hiding this comment

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

Codacy Issue found: Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes.

@JLLeitschuh JLLeitschuh merged commit dc52336 into WPIRoboticsProjects:master May 23, 2016
@AustinShalit AustinShalit added this to the v2.0.0 milestone Jul 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.5.0

Development

Successfully merging this pull request may close these issues.

4 participants

@JLLeitschuh @SamCarlberg @AustinShalit @WPIRoboticsProjects-Bot