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

Master is broken right now #54

Closed
EyLuismi opened this issue Nov 12, 2018 · 2 comments
Closed

Master is broken right now #54

EyLuismi opened this issue Nov 12, 2018 · 2 comments

Comments

@EyLuismi
Copy link
Contributor

EyLuismi commented Nov 12, 2018

Hi! I have attempted to try the last changes and I think Master is broken. I have tried to fix it, but I have not time right now :(

I think it broke going from 8ac582c (Fixes Bugs Of #41 (#53)) to 9289167 (Testing args OOP way (#52))

@shiffman
Copy link
Member

Yes I think both #55 and #56 address all the broken things!

shiffman pushed a commit that referenced this issue Nov 13, 2018
* Fix all the broken things...

Man! This was so spaghetti'd I'm now Italian...

~ Removed a break from the switch statement in the constructor of CommandExecutor which meant that float values were pushed to the argument list twice (once as a float and then again as a command).

~ In Parser.parse() moved the call to nextToken() out of the else condition so that it is always executed.
  The intended functionality is to call parseExpression() when the argument type is int or float.
  After this change nextToken() was always executed regardless, then parseExpression was executed as well if the expression was an int or float, causing arguments to get skipped resulting in attempts to invoke numbers as if they were commands.

~ Renamed COMMAND_TYPES to ARGUMENT_TYPES but not everywhere causing ReferenceErrors

~ #30 Removed calls to parseInt() and parseFloat() in the CommandExecutor constructor because an argument may be an Expression object. #52 Added them back causing Expression objects to become NaN

~ Missing brace at end of parser.js

~ #50 also removed the calls to drawingBounds.reset() and drawingBounds.move(turtle.x, turtle.y) which obviously broke the autofit-drawing-to-canvas feature

~ The autofit-drawing-to-canvas feature was also broken because someone removed the call to the function ¯\_(ツ)_/¯

* Remove bounding box debug rect()

* Switch statements should have breaks, not bugs 🐜

* Changed variable name back

In a previous commit changed 'str' to 'arg' when arg could be a mix of int-as-string/float-as-string/Expression

#55 Changed this and now it will always be string so 'str' is more appropriate.

* [Bugfix] Can't reselect default drawing

Looks like selecting the 'Default' item in the dropdown used to clear the textarea and the drawing until [this](https://github.com/CodingTrain/Logo/pull/51/files#diff-9f2094443505273ff51ea1d1702e4367L101) commit removed it.
So now the drawing remains unchanged when selecting the default item.
It then resets the camera position and calls goTurtle() to redraw which causes the view to jump off-center.

Instead it should put the default Logo in the textarea and call scaleToFitBoundingBox() to redraw _and_ center the drawing. This commit makes this change.
@shiffman
Copy link
Member

Should be working now, re-open if any new issues!

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

No branches or pull requests

2 participants