gh-91167: Fix cloned turtle pen not clearing completely#145406
gh-91167: Fix cloned turtle pen not clearing completely#145406yukarikaname wants to merge 1 commit intopython:mainfrom
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
There was a problem hiding this comment.
Pull request overview
Fixes RawTurtle.clone() so that calling clear() on a cloned turtle does not delete drawings owned by the source turtle, by ensuring the clone tracks its own drawing items instead of inheriting source-owned canvas item ids.
Changes:
- Reinitialize
currentLineanditemsfor cloned turtles afterdeepcopy()to avoid shared canvas-item ownership. - Add a regression test ensuring
clone.clear()does not delete items owned by the source turtle.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
Lib/turtle.py |
Adjusts clone initialization so the clone’s drawing state (currentLine, items) starts fresh and is independent from the source. |
Lib/test/test_turtle.py |
Adds a targeted regression test for clone-vs-source item deletion behavior during clear(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| screen._shapes[self.turtle.shapeIndex]._data] | ||
| q.currentLineItem = screen._createline() | ||
| q.currentLine = [q._position] if q._drawing else [] | ||
| q.items = [q.currentLineItem] |
There was a problem hiding this comment.
clone() still leaves other screen-item ownership state pointing at the source turtle: drawingLineItem is deep-copied (so animated motion can reuse the source’s transient line item), stampItems is deep-copied (so clone.clear()/clearstamps() can delete stamps created by the source), and undobuffer is deep-copied (so clone.undo() can replay/delete the source’s canvas items). Consider reinitializing these fields for q (and resetting any fill-related item ids like _fillitem/_fillpath if filling) so the clone can’t mutate/delete the source turtle’s canvas items via non-clear() operations.
| q.items = [q.currentLineItem] | |
| q.items = [q.currentLineItem] | |
| # Reset ownership/undo state so the clone cannot affect the source | |
| # turtle's existing canvas items via non-clear operations. | |
| # The transient animation line item will be recreated on demand. | |
| if hasattr(q, "drawingLineItem"): | |
| q.drawingLineItem = None | |
| # The clone should start with no stamps of its own. | |
| if hasattr(q, "stampItems"): | |
| q.stampItems = [] | |
| # Clear the clone's undo history so q.undo() cannot replay or | |
| # delete items created before cloning. | |
| if hasattr(q, "undobuffer"): | |
| reset = getattr(q.undobuffer, "reset", None) | |
| if callable(reset): | |
| reset() | |
| # If a fill operation is active on the source, avoid sharing the | |
| # same fill item/path ids with the clone. | |
| if hasattr(q, "_fillitem") and getattr(self, "_fillitem", None) is not None: | |
| q._fillitem = None | |
| if hasattr(q, "_fillpath") and getattr(self, "_fillitem", None) is not None: | |
| q._fillpath = [] |
| def test_clone_clear_does_not_delete_source_items(self): | ||
| screen = self.turtle.screen | ||
| screen._delete.reset_mock() | ||
| screen._createline.side_effect = lambda: object() | ||
|
|
||
| self.turtle.circle(20) | ||
| clone = self.turtle.clone() | ||
| source_items = set(self.turtle.items) | ||
|
|
||
| clone.forward(50) | ||
| clone.clear() | ||
|
|
||
| deleted_items = {call.args[0] for call in screen._delete.call_args_list} | ||
| self.assertFalse(source_items & deleted_items) | ||
|
|
There was a problem hiding this comment.
This test only asserts that clone.clear() doesn’t delete items from self.turtle.items, but the bug class here also involves other clone-owned collections (notably stampItems and undobuffer) that can still contain source-owned canvas ids after deepcopy(). Extending the test to cover stamps (e.g., source stamp() then clone.clear()/clearstamps()) and undo behavior (e.g., clone.undo() doesn’t touch source drawings) would prevent regressions once clone() is updated to reinitialize those fields too.
When calling clear() on a cloned turtle, it incorrectly cleared the source turtle's drawings instead of its own. Fix by reinitializing the clone's currentLine and items to track only its own drawing state.
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
When calling clear() on a cloned turtle, it incorrectly cleared the source
turtle's drawings instead of its own.
The issue was in RawTurtle.clone(): after deepcopy(), the clone inherited
the source turtle's currentLine and items lists, causing the clone's
clear() to delete items owned by the source turtle.
Fix by reinitializing currentLine and items for the clone to track only
its own drawing state.