Minor RectangleSelector refactors #22146
Conversation
| if self._selection_completed: | ||
| # Call onselect, only when the selection is already existing | ||
| self.onselect(self._eventpress, self._eventrelease) | ||
| self._selection_completed = False | ||
| self.clear() |
There was a problem hiding this comment.
Is this change intended here? It's not a refactoring but changes code, e.g. clear() calls update() and then we call update again below in 3501.
There was a problem hiding this comment.
I don't remember what is the purpose of the visible attribute but another consequence of using clear is that it changes the value of the visible attribute from True to False and I am not sure if this desirable - this would need to be checked.
There was a problem hiding this comment.
For the record: it seems that the visible attribute is only used by the selector itself (and should be private) and it is reset in _press, so the point I made here is not an issue!
There was a problem hiding this comment.
Since this code block is only used in a method which is not very long, I am not convinced that creating a separate method make the code more readable: this is about balancing redirection and adding more method versus shorten the code of where it is used, which in this case, I find subjective!
| if self._selection 8000 _completed: | ||
| # Call onselect, only when the selection is already existing | ||
| self.onselect(self._eventpress, self._eventrelease) | ||
| self._selection_completed = False | ||
| self.clear() |
There was a problem hiding this comment.
I don't remember what is the purpose of the visible attribute but another consequence of using clear is that it changes the value of the visible attribute from True to False and I am not sure if this desirable - this would need to be checked.
| @@ -3005,6 +3005,19 @@ def _press(self, event): | |||
|
|
|||
| return False | |||
|
|
|||
| def _selector_too_small(self): | |||
|
I fully appreciate that the selector are not easy to read and I am really looking forward for when we can remove the |
|
😆 couldn't agree more! |
I'm trying to get my head around some odd failures in #21945. As part of that process, here's some minor refactoring that improves readability of the RectangleSelector code and reduces a bit of duplication.