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

Fixing select agent in drawing protocol where the second byte of the … #42

Merged
merged 2 commits into from
Jun 1, 2015

Conversation

pmacalpine
Copy link
Contributor

…command was not being evaluated

@Gama11
Copy link
Member

Gama11 commented May 31, 2015

Have you used the Eclipse source formatter file that's mentioned here? https://github.com/Gama11/RoboViz/blob/master/CONTRIBUTING.md

Some of the code you added uses tabs instead of spaces.

@@ -13,7 +14,17 @@
public Control(ByteBuffer buf, Viewer viewer) {
super();
this.viewer = viewer;
agent = Command.readAgent(buf, viewer.getWorldModel());

int type = ByteUtil.uValue(buf.get());
Copy link
Member

Choose a reason for hiding this comment

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

I guess this line is the one that actually fixes the issue since it increments the byte position? I thought we could get away checking the type here at all, similar to https://github.com/Gama11/RoboViz/blob/master/src/rv/comm/drawing/commands/DrawOption.java that also only has one command, guess not. :)

Also, I suppose it's preferable from a user perspective to print an error instead of ignoring the type like DrawOption does - maybe we should print an error there as well?

Copy link
Member

Choose a reason for hiding this comment

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

This is the sort of thing where unit tests would be really helpful (#43).

@pmacalpine
Copy link
Contributor Author

This fix was basically done through pattern matching of https://github.com/Gama11/RoboViz/blob/master/src/rv/comm/drawing/commands/DrawAnnotation.java. The command in DrawOption.java works because it uses buf.get() to move on to the next byte in the buffer. I suppose we don't have to check the value of the second byte as there is only one option. Checking the value of the second byte in DrawOption.java, and printing an error if it is wrong, is fine to do but will not be backwards compatible for those who might have been using the command without the correct value for the second byte (I don't see a real problem with this).

@Gama11
Copy link
Member

Gama11 commented May 31, 2015

I think breaking backwards compatibility here is fine, since technically the drawing API does not allow this and it would break anyway if another DrawOption command is added in the future. Plus since it prints an error the issue is simple enough to find.

@pmacalpine
Copy link
Contributor Author

The formatting should now be fixed to match that of the Eclipse source formatter file.

Gama11 added a commit that referenced this pull request Jun 1, 2015
Fixing select agent in drawing protocol where the second byte of the …
@Gama11 Gama11 merged commit efe050e into magmaOffenburg:master Jun 1, 2015
@pmacalpine pmacalpine deleted the draw_command_selection_bug branch June 1, 2015 19:43
@Gama11
Copy link
Member

Gama11 commented Jun 1, 2015

Thanks!

Btw, it seems like you didn't specifiy an email in your Git config, at least it looks like some default value (patmac@patmac-MacBookPro.(none)). GitHub tries to link commits to GitHub profiles by comparing the account's email to the email in the commit's author field.

Nevermind if not specifying your email here is intentional, just thought I'd point it out in case it wasn't. :)

Gama11 added a commit that referenced this pull request Jun 3, 2015
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