Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: remove (biased) event-handler subscription, preventing an event of having more than 1 handlers #3808

Merged
merged 1 commit into from
Aug 20, 2024
Merged

chore: remove (biased) event-handler subscription, preventing an event of having more than 1 handlers #3808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions sdk/python/packages/flet-core/src/flet_core/auto_complete.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def __init__(
self,
suggestions: Optional[List[AutoCompleteSuggestion]] = None,
suggestions_max_height: OptionalNumber = None,
on_select=None,
on_select: OptionalEventCallable["AutoCompleteSelectEvent"] = None,
#
# Control
#
Expand Down Expand Up @@ -87,12 +87,12 @@ def suggestions(self, value: Optional[List[str]]):

# on_select
@property
def on_select(self):
return self._get_event_handler("select")
def on_select(self) -> OptionalEventCallable["AutoCompleteSelectEvent"]:
return self.__on_select.handler

@on_select.setter
def on_select(self, handler: OptionalEventCallable["AutoCompleteSelectEvent"]):
self.__on_select.subscribe(handler)
self.__on_select.handler = handler


class AutoCompleteSelectEvent(ControlEvent):
Expand Down
6 changes: 3 additions & 3 deletions sdk/python/packages/flet-core/src/flet_core/canvas/canvas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return self.__on_resize.handler

@on_resize.setter
def on_resize(self, handler: OptionalEventCallable["CanvasResizeEvent"]):
self.__on_resize.subscribe(handler)
self.__on_resize.handler = handler
self._set_attr("onresize", True if handler is not None else None)


Expand Down
6 changes: 3 additions & 3 deletions sdk/python/packages/flet-core/src/flet_core/charts/bar_chart.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return self.__on_chart_event.handler

@on_chart_event.setter
def on_chart_event(self, handler: OptionalEventCallable["BarChartEvent"]):
self.__on_chart_event.subscribe(handler)
self.__on_chart_event.handler = handler
self._set_attr("onChartEvent", True if handler is not None else None)


Expand Down
6 changes: 3 additions & 3 deletions sdk/python/packages/flet-core/src/flet_core/charts/line_chart.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,12 +453,12 @@ def tooltip_border_side(self, value: Optional[BorderSide]):

# on_chart_event
@property
def on_chart_event(self):
return self.__on_chart_event
def on_chart_event(self) -> OptionalEventCallable["LineChartEvent"]:
return self.__on_chart_event.handler

@on_chart_event.setter
def on_chart_event(self, handler: OptionalEventCallable["LineChartEvent"]):
self.__on_chart_event.subscribe(handler)
self.__on_chart_event.handler = handler
self._set_attr("onChartEvent", True if handler is not None else None)


Expand Down
6 changes: 3 additions & 3 deletions sdk/python/packages/flet-core/src/flet_core/charts/pie_chart.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return self.__on_chart_event.handler

@on_chart_event.setter
def on_chart_event(self, handler: OptionalEventCallable["PieChartEvent"]):
self.__on_chart_event.subscribe(handler)
self.__on_chart_event.handler = handler
self._set_attr("onChartEvent", True if handler is not None else None)


Expand Down
10 changes: 4 additions & 6 deletions sdk/python/packages/flet-core/src/flet_core/container.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,9 +295,7 @@ def blend_mode(self) -> Optional[BlendMode]:
@blend_mode.setter
def blend_mode(self, value: Optional[BlendMode]):
self.__blend_mode = value
self._set_attr(
"blendMode", value.value if isinstance(value, BlendMode) else value
)
self._set_enum_attr("blendMode", value, BlendMode)

# blur
@property
Expand Down Expand Up @@ -569,12 +567,12 @@ def on_click(self, handler: OptionalControlEventCallable):

# on_tap_down
@property
def on_tap_down(self):
return self.__on_tap_down
def on_tap_down(self) -> OptionalEventCallable["ContainerTapEvent"]:
return self.__on_tap_down.handler

@on_tap_down.setter
def on_tap_down(self, handler: OptionalEventCallable["ContainerTapEvent"]):
self.__on_tap_down.subscribe(handler)
self.__on_tap_down.handler = handler
self._set_attr("onTapDown", True if handler is not None else None)

# on_long_press
Expand Down
4 changes: 2 additions & 2 deletions sdk/python/packages/flet-core/src/flet_core/control.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def __init__(
self.data = data
self.rtl = rtl

self.__event_handlers: Dict[str, OptionalEventCallable] = {}
self.__event_handlers: Dict[str, OptionalControlEventCallable] = {}
self.parent: Optional[Control] = None

def is_isolated(self) -> bool:
Expand Down Expand Up @@ -103,7 +103,7 @@ def _get_control_name(self) -> str:
)

def _add_event_handler(
self, event_name: str, handler: OptionalEventCallable
self, event_name: str, handler: OptionalControlEventCallable
) -> None:
self.__event_handlers[event_name] = handler

Expand Down
16 changes: 8 additions & 8 deletions sdk/python/packages/flet-core/src/flet_core/datatable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,12 @@ def tooltip(self, value: Optional[str]):

# on_sort
@property
def on_sort(self):
return self.__on_sort
def on_sort(self) -> OptionalEventCallable["DataColumnSortEvent"]:
return self.__on_sort.handler

@on_sort.setter
def on_sort(self, handler: Optional[Callable[[DataColumnSortEvent], None]]):
self.__on_sort.subscribe(handler)
def on_sort(self, handler: OptionalEventCallable["DataColumnSortEvent"]):
self.__on_sort.handler = handler
self._set_attr("onSort", True if handler is not None else None)


Expand Down Expand Up @@ -218,12 +218,12 @@ def on_tap_cancel(self, handler: OptionalControlEventCallable):

# on_tap_down
@property
def on_tap_down(self):
return self.__on_tap_down
def on_tap_down(self) -> OptionalEventCallable[TapEvent]:
return self.__on_tap_down.handler

@on_tap_down.setter
def on_tap_down(self, handler: Optional[Callable[[TapEvent], None]]):
self.__on_tap_down.subscribe(handler)
def on_tap_down(self, handler: OptionalEventCallable[TapEvent]):
self.__on_tap_down.handler = handler
self._set_attr("onTapDown", True if handler is not None else None)


Expand Down
10 changes: 6 additions & 4 deletions sdk/python/packages/flet-core/src/flet_core/date_picker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,11 +378,13 @@ def on_dismiss(self, handler: OptionalControlEventCallable):

# on_entry_mode_change
@property
def on_entry_mode_change(self):
return self.__on_entry_mode_change
def on_entry_mode_change(
self,
) -> OptionalEventCallable[DatePickerEntryModeChangeEvent]:
return self.__on_entry_mode_change.handler

@on_entry_mode_change.setter
def on_entry_mode_change(
self, handler: Optional[Callable[[DatePickerEntryModeChangeEvent], None]]
self, handler: OptionalEventCallable[DatePickerEntryModeChangeEvent]
):
self.__on_entry_mode_change.subscribe(handler)
self.__on_entry_mode_change.handler = handler
18 changes: 9 additions & 9 deletions sdk/python/packages/flet-core/src/flet_core/dismissible.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,34 +247,34 @@ def dismiss_thresholds(

# on_dismiss
@property
def on_dismiss(self):
return self._get_event_handler("dismiss")
def on_dismiss(self) -> OptionalEventCallable["DismissibleDismissEvent"]:
return self.__on_dismiss.handler

@on_dismiss.setter
def on_dismiss(self, handler: OptionalEventCallable["DismissibleDismissEvent"]):
self.__on_dismiss.subscribe(handler)
self.__on_dismiss.handler = handler
self._set_attr("onDismiss", True if handler is not None else None)

# on_confirm_dismiss
@property
def on_confirm_dismiss(self):
return self._get_event_handler("confirm_dismiss")
def on_confirm_dismiss(self) -> OptionalEventCallable["DismissibleDismissEvent"]:
return self.__on_confirm_dismiss.handler

@on_confirm_dismiss.setter
def on_confirm_dismiss(
self, handler: OptionalEventCallable["DismissibleDismissEvent"]
):
self.__on_confirm_dismiss.subscribe(handler)
self.__on_confirm_dismiss.handler = handler
self._set_attr("onConfirmDismiss", True if handler is not None else None)

# on_update
@property
def on_update(self):
return self._get_event_handler("update")
def on_update(self) -> OptionalEventCallable["DismissibleUpdateEvent"]:
return self.__on_update.handler

@on_update.setter
def on_update(self, handler: OptionalEventCallable["DismissibleUpdateEvent"]):
self.__on_update.subscribe(handler)
self.__on_update.handler = handler
self._set_attr("onUpdate", True if handler is not None else None)

# on_resize
Expand Down
33 changes: 17 additions & 16 deletions sdk/python/packages/flet-core/src/flet_core/drag_target.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from flet_core.control_event import ControlEvent
from flet_core.event_handler import EventHandler
from flet_core.ref import Ref
from flet_core.types import OptionalControlEventCallable, OptionalEventCallable


class DragTarget(Control):
Expand Down Expand Up @@ -106,10 +107,10 @@ def __init__(
self,
content: Control,
group: Optional[str] = None,
on_will_accept=None,
on_accept=None,
on_leave=None,
on_move=None,
on_will_accept: OptionalControlEventCallable = None,
on_accept: OptionalEventCallable["DragTargetEvent"] = None,
on_leave: OptionalControlEventCallable = None,
on_move: OptionalEventCallable["DragTargetEvent"] = None,
#
# Control
#
Expand Down Expand Up @@ -170,39 +171,39 @@ def content(self, value: Control):

# on_will_accept
@property
def on_will_accept(self):
def on_will_accept(self) -> OptionalControlEventCallable:
return self._get_event_handler("will_accept")

@on_will_accept.setter
def on_will_accept(self, handler):
def on_will_accept(self, handler: OptionalControlEventCallable):
self._add_event_handler("will_accept", handler)

# on_accept
@property
def on_accept(self):
return self.__on_accept
def on_accept(self) -> OptionalEventCallable["DragTargetEvent"]:
return self.__on_accept.handler

@on_accept.setter
def on_accept(self, handler):
self.__on_accept.subscribe(handler)
def on_accept(self, handler: OptionalEventCallable["DragTargetEvent"]):
self.__on_accept.handler = handler

# on_leave
@property
def on_leave(self):
def on_leave(self) -> OptionalControlEventCallable:
return self._get_event_handler("leave")

@on_leave.setter
def on_leave(self, handler):
def on_leave(self, handler: OptionalControlEventCallable):
self._add_event_handler("leave", handler)

# on_move
@property
def on_move(self):
return self.__on_move
def on_move(self) -> OptionalEventCallable["DragTargetEvent"]:
return self.__on_move.handler

@on_move.setter
def on_move(self, handler):
self.__on_move.subscribe(handler)
def on_move(self, handler: OptionalEventCallable["DragTargetEvent"]):
self.__on_move.handler = handler


class DragTargetAcceptEvent(ControlEvent):
Expand Down
23 changes: 6 additions & 17 deletions sdk/python/packages/flet-core/src/flet_core/event_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import asyncio
from typing import Callable
from typing import Callable, Optional

from flet_core.control_event import ControlEvent
from flet_core.types import OptionalControlEventCallable
Expand All @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (code-quality): Add guard clause (last-if-guard)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? Explain detaily.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fn

In 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.

for handler in self.__handlers.keys():
if self.handler is not None:
ce = e
if self.__result_converter is not None:
ce = self.__result_converter(e)
Expand All @@ -24,20 +24,9 @@ async def fn(e: ControlEvent):
ce.page = e.page

if ce is not None:
if asyncio.iscoroutinefunction(handler):
await handler(ce)
if asyncio.iscoroutinefunction(self.handler):
await self.handler(ce)
else:
e.page.run_thread(handler, ce)
e.page.run_thread(self.handler, ce)

return fn

def subscribe(self, handler: OptionalControlEventCallable):
if handler is not None:
self.__handlers[handler] = True

def unsubscribe(self, handler: Callable[[ControlEvent], None]):
if handler in self.__handlers:
self.__handlers.pop(handler)

def count(self) -> int:
return len(self.__handlers)
15 changes: 8 additions & 7 deletions sdk/python/packages/flet-core/src/flet_core/file_picker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from flet_core.control_event import ControlEvent
from flet_core.event_handler import EventHandler
from flet_core.ref import Ref
from flet_core.types import OptionalEventCallable
from flet_core.utils import deprecated

try:
Expand Down Expand Up @@ -329,18 +330,18 @@ def allow_multiple(self, value: Optional[bool]):

# on_result
@property
def on_result(self):
return self.__on_result
def on_result(self) -> OptionalEventCallable[FilePickerResultEvent]:
return self.__on_result.handler

@on_result.setter
def on_result(self, handler: Optional[Callable[[FilePickerResultEvent], None]]):
self.__on_result.subscribe(handler)
def on_result(self, handler: OptionalEventCallable[FilePickerResultEvent]):
self.__on_result.handler = handler

# on_upload
@property
def on_upload(self):
return self.__on_upload
return self.__on_upload.handler

@on_upload.setter
def on_upload(self, handler: Optional[Callable[[FilePickerUploadEvent], None]]):
self.__on_upload.subscribe(handler)
def on_upload(self, handler: OptionalEventCallable[FilePickerUploadEvent]):
self.__on_upload.handler = handler
Loading