1j01 / jspaint

🎨 Classic MS Paint, REVIVED + ✨Extras
https://jspaint.app/about
MIT License
7.25k stars 568 forks source link

issue 160 fix #173

Closed mwoodsy closed 4 years ago

mwoodsy commented 4 years ago

Non-rounded rectangle line width is broken #160

Fixed this bug. https://github.com/1j01/jspaint/issues/160

Line width was not being set for rectangles so it was defaulting to 1 pixel. Setting the stroke size resolved this issue.

1j01 commented 4 years ago

Hey, thanks for contributing.

For regressions, I always like to pinpoint where something was broken, so I can mention in the commit why the fix was needed. It's also good because sometimes you can find other things that were broken at the same time, or another way to fix it, or old code that's no longer needed if the newer fix is a better way to do it.

git bisect is the tool for this. It uses a binary search so you can find it by looking at way less commits than the range of commits it might be within. Have you used git bisect before? Do you want to learn? (jspaint is a really good project to git bisect in, because with live-server and no compile step, it's really smooth stepping thru commits.)

mwoodsy commented 4 years ago

Thanks for informing me about the git bisect tool. I was able to find a good version of the project and using bisect I was able to determine that the following commit caused the bug:

561d089f4500515f349c0e6ca9d7d551d399e582 is the first bad commit commit 561d089f4500515f349c0e6ca9d7d551d399e582 Author: Isaiah Odhner isaiahodhner@gmail.com Date: Thu Dec 5 20:53:43 2019 -0500

Use helper layer for previewing shape tools

- For Multitools, makes the tools render on top of each other like you'd probably expect.
- May improve multi-user experience slightly

src/app.js | 13 ------------- src/tools.js | 29 ++++++++++++++++++++++++++--- 2 files changed, 26 insertions(+), 16 deletions(-)

mwoodsy commented 4 years ago

I corrected my original fix so that it lines up better with the code that caused the bug in the first place.

1j01 commented 4 years ago

Thanks! Yeah, that looks like a more appropriate fix. 👍