Add threaded canvas item#131
Conversation
This hack is being removed because it is not compatible with upcoming changes. The hack was used to work around a problem of not receiving mouse exit messages in canvas items. This seems to have been fixed via other mechanisms.
This eliminates the assumption that the 'root container' is a RootCanvasItem; and instead introduces a base container protocol for more flexibility in how canvas item hierarchies are contained.
cmeyer
requested review from
Brow71189,
KRLango,
MattRoyle,
Tiomat85 and
lisham2000
lisham2000
left a comment
lisham2000
left a comment
There was a problem hiding this comment.
Comment landscape makes the code really easy to follow and decisions are clear
| super().__init__() | ||
| self.__threaded_canvas_item = threaded_canvas_item | ||
|
|
||
| def _updated(self) -> None: |
There was a problem hiding this comment.
Should _updated call update. That seems wrong to me. If it is correct, then I think a comment is needed to explain it to anyone else who looks at this.
There was a problem hiding this comment.
I read this as it trying to chain the updates, so _updated is called after an update on this object is complete, then it chains down to start the update on the child object. I had assumed this was intentional to do it's own update before the child update.
There was a problem hiding this comment.
I will add a comment about this. Basically there are updates coming from canvas items "below" the threaded canvas item, and updates that need to go to canvas items "above" the threaded canvas item. update is a request from anywhere to update the canvas item on which it is called. _update is called when the item has actually been updated. In this case, it is used to tell the canvas items "above" that they now need to update. Explained another way: the canvas items in the thread get updated; when the top level item (the wrapper) receives _updated message it will have a valid drawing context for everything "below"; now it needs to tell the threaded canvas item that it needs to update so that it gets redrawn using the drawing context that just became became valid.
| traceback.print_exc() | ||
| traceback.print_stack() | ||
|
|
||
| def mouse_clicked(self, x: int, y: int, modifiers: UserInterface.KeyboardModifiers) -> bool: |
There was a problem hiding this comment.
What is the benefit of having a public function (e.g. mouse_clicked) that just passes the arguments to a private function (e.g. __mouse_clicked) over just a public function?
There was a problem hiding this comment.
This is a bit subtle, but my intention is to merge Layer/Root/Threaded canvas items. This is separate because I initially did that but backed away since it was too many changes at once. With the 'root' variant, the mouse events (and other events) come from the widget rather than the inherited mouse methods. So I left them separate in this version.
Tiomat85
left a comment
Tiomat85
left a comment
There was a problem hiding this comment.
Looks ok, though I am a bit unclear on _request_base_focus.
Me too. I left this alone to match the existing code in Layer/Root. I expect to refactor this in an upcoming PR. |
Add threaded canvas item comments.
|
Merged. 7652233 |
4 participants
Each commit has been audited by copilot.