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

Saving shapes #26

Merged
merged 8 commits into from Mar 3, 2019

Conversation

@vlogozzo
Copy link
Collaborator

commented Mar 2, 2019

fixes issue 25 and issue 22

there is also a commit that removes the class files mentioned in issue 23.

vlogozzo added 5 commits Feb 28, 2019
changed inital creation for new constructors with hard coded names

updated for the reading and writting from .txt

with constructor changes hard coded names into constructors
Copy link
Owner

left a comment

It looks like what you did will work, but I don't think we need/want to change the default constructor. Since this issue is just addressing the save/load functions, we should only need to update the save method and the load constructor. That way we can avoid having adverse effects in other parts of the code (we shouldn't need to update the createShape() method in canvasArea at all.)

Please revert the changes to the default constructor and use the load constructor to overwrite the name data member. If you have any questions or this doesn't work like I think it does, please let me know.

The change from absolute path to relative path looks good, and should be fine to incorporate in with this pull request. Good catch on that!

@vlogozzo

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 3, 2019

should backwards compatibility be handled? what if someone brings in a save file that doesnt have names for the shapes

@vlogozzo vlogozzo force-pushed the vlogozzo:saving-shapes branch from b929567 to 72ef49d Mar 3, 2019
@jwilder4690

This comment has been minimized.

Copy link
Owner

commented Mar 3, 2019

should backwards compatibility be handled? what if someone brings in a save file that doesn't have names for the shapes

I am not worried about save files becoming incompatible. I noted that this project is still in active development, and I will make a statement in the next release that save files will not be backward compatible. That way the users will know that the new version will not work with their old save files.

@vlogozzo vlogozzo requested a review from jwilder4690 Mar 3, 2019
Copy link
Owner

left a comment

Polygon needs a change in order to work properly, everything else looks great.

@@ -51,6 +52,7 @@
for(int i = 0; i < Integer.valueOf(input[8]); i++){

This comment has been minimized.

Copy link
@jwilder4690

jwilder4690 Mar 3, 2019

Owner

Because polygons can have any number of vertices, there is no guarantee that index 14 will actually hold the name of the Polygon. I recommend moving the name to just before the vertex loop at index 8, and increasing the next two indices to 9 and 10. Then update the save method to match.

These are the changes I mentioned in my review.
@jwilder4690

This comment has been minimized.

Copy link
Owner

commented Mar 3, 2019

So I tried to make the changes that I requested from you as a pull request to your branch, but I got a 404 error. But it did allow me to commit the changes directly to your branch, which I found surprising. Maybe that is because it is a fork of my project? Anyway, with the changes incorporated I can approve this enhancement. Thanks for your hard work!

@vlogozzo

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 3, 2019

the changes you proposed have a slight bug, im about to push the fix

Copy link
Owner

left a comment

I was able to add a commit to fix the polygon issue.

@vlogozzo

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 3, 2019

correction the changes i put in had a slight bug, yours works fine :^)

@vlogozzo

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 3, 2019

ready for merge

@jwilder4690 jwilder4690 merged commit f2dc291 into jwilder4690:master Mar 3, 2019
@vlogozzo vlogozzo deleted the vlogozzo:saving-shapes branch Mar 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.