OLEDDisplay.cpp -> fillCircle() fixed

ThingPulse / esp8266-oled-ssd1306

Driver for the SSD1306 and SH1106 based 128x64, 128x32, 64x48 pixel OLED display running on ESP8266/ESP32

https://thingpulse.com

Other

1.99k stars 637 forks source link

OLEDDisplay.cpp -> fillCircle() fixed #279

Closed NoobTracker closed 4 years ago

NoobTracker commented 4 years ago

Circles created with fillCircle() were one pixel taller than wide.

marcelstoer commented 4 years ago

Great to see this PR, well done!

However, you moved the .cpp file out of src/ into the root folder instead of editing the existing file. Please fix that in your circles-fixed branch and commit the change.

NoobTracker commented 4 years ago

Oh, sorry. I'll fix that.

NoobTracker commented 4 years ago

Okay, done.

marcelstoer commented 4 years ago

Check the "Files changed" tab: https://github.com/ThingPulse/esp8266-oled-ssd1306/pull/279/files

If you changed a single line then we would expect to see a single line in the diff.

NoobTracker commented 4 years ago

Ah, I've added a line (drawVerticalLine, line ~300) and maybe removed an empty line.

NoobTracker commented 4 years ago

I don't now how GitHub handles that. I only changed fillCircle().

NoobTracker commented 4 years ago

Yes, it looks like that's the reason.

NoobTracker commented 4 years ago

Ooops, the actual reason is that I downloaded the files and replaced them on GitHub.

marcelstoer commented 4 years ago

You created a bit of a mess here because A) this one is "broken" as you realized and B) you were branching your other two branches off of this one rather than master. Hence, we see these commit in the other PRs, too.

Here's my proposal for a way forward:

fix this PR by e.g.
- git checkout circles-fixed
- git reset --hard origin/master // you will loose all local changes!
- change that one(?) line that needs fixing
- git commit -am "whatever message"
- git push --force
then EITHER wait for this PR to land and proceed likewise for the other branches
OR proceed with the other branches right now and deal with potential merge conflicts later

NoobTracker commented 4 years ago

The problem with this is that the newer version with the ring functions all changes the circular functions. In addition, the header file must also change. When I try to do this on GitHub, it is much harder to test the program. I would have to download it then. And the latest version (keywords) contains all changes to circles-fixed and circles-improved-rings. And where can I use this command line?