chore: remove (biased) event-handler subscription, preventing an event of having more than 1 handlers #3808
Conversation
Reviewer's Guide by SourceryThis pull request implements a significant change in the event handling mechanism across multiple components in the Flet core library. The main changes involve replacing the subscription-based event handling with a single handler approach, updating type hints, and refactoring related methods and properties. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ndonkoHenri - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 5 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -141,12 +141,12 @@ def resize_interval(self, value: OptionalNumber): | |||
|
|
|||
| # on_resize | |||
| @property | |||
| def on_resize(self): | |||
| return self.__on_resize | |||
| def on_resize(self) -> OptionalEventCallable["CanvasResizeEvent"]: | |||
There was a problem hiding this comment.
issue (complexity): Consider maintaining the subscription pattern for event handling.
The new implementation of the on_resize property introduces additional complexity by directly accessing the handler attribute of __on_resize. This approach violates encapsulation and increases coupling, making the code harder to maintain and understand. The original subscription model was more straightforward and intuitive. Consider maintaining the subscription pattern, which is likely more maintainable and aligns with common event handling practices. Additionally, ensure there is a clear way to unsubscribe handlers if needed. This will help keep the codebase clean and easier to work with in the future.
| @@ -407,12 +407,12 @@ def tooltip_direction(self, value: Optional[TooltipDirection]): | |||
|
|
|||
| # on_chart_event | |||
| @property | |||
| def on_chart_event(self): | |||
| return self.__on_chart_event | |||
| def on_chart_event(self) -> OptionalEventCallable["BarChartEvent"]: | |||
There was a problem hiding this comment.
issue (complexity): Consider using methods to manage the event handler.
The recent code change introduces additional complexity by directly accessing the handler attribute of __on_chart_event. This approach violates encapsulation and increases coupling, as it exposes internal details and ties the code to the specific implementation of __on_chart_event. It also reduces clarity compared to the previous method-based approach for managing event handlers. To simplify the code and maintain encapsulation, consider using get_handler and set_handler methods to manage the event handler. This will help preserve clarity and make the code easier to maintain.
| @@ -168,12 +168,12 @@ def animate(self, value: AnimationValue): | |||
|
|
|||
| # on_chart_event | |||
| @property | |||
| def on_chart_event(self): | |||
| return self.__on_chart_event | |||
| def on_chart_event(self) -> OptionalEventCallable["PieChartEvent"]: | |||
There was a problem hiding this comment.
issue (complexity): Consider maintaining the original approach to manage event handlers.
The new code introduces additional complexity by directly accessing self.__on_chart_event.handler, which violates encapsulation and increases coupling. This approach exposes internal details of __on_chart_event, making the code harder to maintain and understand. The original method of using subscribe to manage event handlers was more descriptive and clear. To reduce complexity, consider maintaining the original approach or using a method like get_handler to retrieve the current handler, which keeps the internal structure hidden and maintains a clear interface.
| def on_selection_change( | ||
| self, | ||
| ) -> OptionalEventCallable[MarkdownSelectionChangeEvent]: | ||
| return self.__on_selection_change.handler |
There was a problem hiding this comment.
issue (complexity): Consider refactoring to maintain the original event handler pattern.
The new implementation of the on_selection_change property introduces additional complexity by using self.__on_selection_change.handler and accessing private attributes. This can make the code harder to understand and maintain. Consider refactoring to maintain the original pattern with _get_event_handler and _add_event_handler for consistency and simplicity. This approach avoids direct access to private attributes and aligns with the existing event handler management style. Here's a suggested refactoring:
@property
def on_selection_change(self) -> OptionalEventCallable[MarkdownSelectionChangeEvent]:
return self._get_event_handler("selection_change")
@on_selection_change.setter
def on_selection_change(self, handler: OptionalEventCallable[MarkdownSelectionChangeEvent]):
self._add_event_handler("selection_change", handler)This keeps the codebase consistent and reduces unnecessary complexity.
There was a problem hiding this comment.
@FeodorFitsner, any thoughts on how we could make these "custom" events similar to the "normal" ones?
There was a problem hiding this comment.
@ndonkoHenri, the suggestion I provided aims to maintain consistency with the existing event handler pattern in the codebase. By using _get_event_handler and _add_event_handler, we can ensure that the custom events are managed in the same way as the normal ones, which helps in keeping the codebase uniform and easier to maintain.
If you want to make custom events similar to the normal ones, you can follow the same pattern for all event handlers. This means defining a getter and setter for each event handler property, using the _get_event_handler and _add_event_handler methods. This approach abstracts away the direct access to private attributes and provides a consistent interface for event handling.
Here's an example of how you can refactor the on_selection_change property to align with the normal event handler pattern:
@property
def on_selection_change(self) -> OptionalEventCallable[MarkdownSelectionChangeEvent]:
return self._get_event_handler("selection_change")
@on_selection_change.setter
def on_selection_change(self, handler: OptionalEventCallable[MarkdownSelectionChangeEvent]):
self._add_event_handler("selection_change", handler)By applying this pattern to all custom events, you can achieve a consistent and maintainable event handling system. If you have any specific concerns or scenarios where this pattern might not fit, please let me know, and we can discuss further adjustments.
| def on_scroll(self): | ||
| return self.__on_scroll | ||
| def on_scroll(self) -> OptionalEventCallable["OnScrollEvent"]: | ||
| return self.__on_scroll.handler |
There was a problem hiding this comment.
issue (complexity): Consider using a method like 'subscribe' to manage event handlers.
The new implementation introduces increased complexity by directly accessing __on_scroll.handler, which violates encapsulation and increases coupling to the internal structure of the __on_scroll object. This makes the code more fragile and harder to maintain. Consider using a method like subscribe to manage event handlers, which would preserve encapsulation and provide a clearer, more maintainable approach. This would also make the code less dependent on the internal details of __on_scroll, reducing the risk of future issues if its implementation changes.
| @@ -8,11 +8,11 @@ | |||
| class EventHandler: | |||
| def __init__(self, result_converter=None) -> None: | |||
| self.__result_converter = result_converter | |||
| self.__handlers = {} | |||
| self.handler: OptionalControlEventCallable = None | |||
|
|
|||
| def get_handler(self): | |||
| async def fn(e: ControlEvent): | |||
There was a problem hiding this comment.
issue (code-quality): Add guard clause (last-if-guard)
There was a problem hiding this comment.
What do you mean? Explain detaily.
There was a problem hiding this comment.
Certainly! The suggestion to "Add guard clause" refers to a common refactoring technique where you handle special cases or error conditions early in a function, allowing the main logic to be more readable and less nested.
In this context, it means you should add a check at the beginning of the get_handler function to handle any conditions that might prevent the function from proceeding normally. For example, if self.handler is None, you could return early or raise an exception. This makes the code cleaner and easier to understand.
Here's an example of how you might implement a guard clause in the get_handler function:
def get_handler(self):
if self.handler is None:
raise ValueError("Handler is not set")
async def fn(e: ControlEvent):
await self.handler(e)
return fnIn this example, if self.handler is None, the function raises an exception immediately, preventing any further execution. This ensures that the main logic of the function (the fn definition and return) is not nested inside an if statement, making it clearer and more straightforward.
Summary by Sourcery
Refactor the event handler system to replace the subscription model with a direct assignment model, ensuring that each event can have only one handler at a time.
Enhancements: