Bump Flutter packages #3719
Bump Flutter packages #3719
Conversation
Reviewer's Guide by SourceryThis pull request introduces several new features and enhancements across multiple components. Key additions include advanced tooltip configurations for charts, new event handling for markdown selection changes, and improved image handling in markdown controls. Additionally, the audio recorder functionality has been extended with a cancel recording feature, and several dependencies have been updated. 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: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -69,8 +75,111 @@ class MarkdownControl extends StatelessWidget with FletStoreMixin { | |||
| codeTheme.toLowerCase(), mdStyleSheet, selectable), | |||
| }, | |||
| styleSheet: mdStyleSheet, | |||
| imageBuilder: (Uri uri, String? title, String? alt) { | |||
There was a problem hiding this comment.
suggestion (performance): Consider caching decoded images for performance
The image rendering logic is comprehensive, but decoding images, especially base64 encoded ones, can be expensive. Consider implementing a caching mechanism for decoded images to improve performance for repeated renders.
| imageBuilder: (Uri uri, String? title, String? alt) { | |
| imageBuilder: (Uri uri, String? title, String? alt) { | |
| final cache = <String, Widget>{}; | |
| if (cache.containsKey(uri.toString())) { | |
| return cache[uri.toString()]!; | |
| } | |
| Widget? image; | |
| String s = uri.toString(); | |
| // Your existing image decoding logic here | |
| cache[uri.toString()] = image!; | |
| return image; |
| "stop_recording", | ||
| wait_for_result=True, | ||
| wait_timeout=wait_timeout, | ||
| ) | ||
| return out if out is not None else None | ||
|
|
||
| def cancel_recording(self, wait_timeout: Optional[float] = 5) -> None: |
There was a problem hiding this comment.
suggestion: Add documentation for cancel_recording method
The new cancel_recording method is a good addition for user control, but it lacks documentation. Add a docstring explaining its purpose, parameters, and behavior to improve code maintainability and usability.
| def cancel_recording(self, wait_timeout: Optional[float] = 5) -> None: | |
| def cancel_recording(self, wait_timeout: Optional[float] = 5) -> None: | |
| """ | |
| Cancels the ongoing recording. | |
| Parameters: | |
| wait_timeout (Optional[float]): The time to wait for the recording to cancel. Default is 5 seconds. | |
| Returns: | |
| None | |
| """ |
| @@ -43,6 +44,16 @@ def __init__( | |||
| baseline_y: OptionalNumber = None, | |||
| min_y: OptionalNumber = None, | |||
| max_y: OptionalNumber = None, | |||
| tooltip_rounded_radius: OptionalNumber = None, | |||
There was a problem hiding this comment.
issue (complexity): Consider encapsulating the tooltip properties into a separate TooltipConfig class.
The new code introduces a significant number of new attributes related to tooltips, which increases complexity and maintenance overhead. The increased number of attributes and corresponding methods adds to the cognitive load and potential for errors. To simplify, consider encapsulating the tooltip properties into a separate TooltipConfig class. This reduces the number of attributes in the LineChart class, making the code more modular and easier to maintain. Future changes to tooltip properties can be made in the TooltipConfig class without affecting the LineChart class, reducing the risk of introducing bugs. This approach should make the code cleaner and more maintainable.
| @@ -109,6 +153,14 @@ def __init__( | |||
| data=data, | |||
| ) | |||
|
|
|||
| self.__on_selection_change = EventHandler( | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the new code to reduce complexity.
The new code introduces several complexities that could be streamlined. Here are some observations:
-
Increased Number of Imports: The new code adds several new imports, which can make the module harder to understand and maintain. Each new import adds to the cognitive load of understanding dependencies and their interactions.
-
New Classes and Enums: The addition of
MarkdownSelectionChangedCauseandMarkdownSelectionChangeEventclasses increases complexity. While these might be necessary, they add to the number of components a developer needs to understand. -
Event Handling: The new event handling logic for
on_selection_changeadds more layers of abstraction. The use ofEventHandlerand the custom event classMarkdownSelectionChangeEventintroduces additional complexity in understanding how events are processed and handled. -
Additional Properties and Methods: The new properties (
shrink_wrap,fit_content,soft_line_break,img_error_content,on_tap_text,on_selection_change) and their corresponding getter and setter methods increase the size and complexity of theMarkdownclass. This makes the class harder to read and maintain. -
Redundant Code: There seems to be some redundancy in the
on_selection_changeproperty and setter methods, which can lead to confusion and potential bugs.
Suggested Simplifications:
- Remove Redundant Code: Eliminate redundant
on_selection_changeproperty and setter methods. - Simplify Event Handling: Keep the event handling logic but make it more straightforward.
- Consolidate Imports: Reduce the number of imports to only those necessary for the new functionality.
These changes should help maintain the new functionality while reducing overall complexity, making the code easier to read and maintain.
…, md_style_sheet), MarkdownCodeTheme enum (for code_theme)
There was a problem hiding this comment.
Hey @ndonkoHenri - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider splitting large PRs like this into smaller, more focused changes in the future for easier review and integration.
- Ensure clear documentation and migration guides are provided for deprecated features, such as the
code_styleproperty in the Markdown control.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 4 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
|
||
|
|
||
| @dataclass | ||
| class MarkdownStyleSheet: |
There was a problem hiding this comment.
suggestion: Consider breaking down MarkdownStyleSheet into smaller, more focused classes
While the comprehensive nature of MarkdownStyleSheet allows for fine-grained control, it might be overwhelming for users. Consider creating sub-classes for different aspects of styling (e.g., TextStyles, Alignments, Decorations) to improve readability and maintainability.
| class MarkdownStyleSheet: | |
| @dataclass | |
| class TextStyles: | |
| a_text_style: Optional[TextStyle] = None | |
| p_text_style: Optional[TextStyle] = None | |
| @dataclass | |
| class MarkdownStyleSheet: | |
| text_styles: TextStyles = TextStyles() |
| return markdownStyleSheetFromJson(theme, j); | ||
| } | ||
|
|
||
| MarkdownStyleSheet markdownStyleSheetFromJson( |
There was a problem hiding this comment.
suggestion: Consider adding error handling for invalid JSON input
The markdownStyleSheetFromJson function assumes the input JSON is well-formed and contains all expected fields. Consider adding error handling to gracefully handle cases where the JSON input is malformed or missing expected fields.
| MarkdownStyleSheet markdownStyleSheetFromJson( | |
| MarkdownStyleSheet markdownStyleSheetFromJson( | |
| ThemeData theme, Map<String, dynamic> j) { | |
| if (j == null || j.isEmpty) { | |
| throw ArgumentError('Invalid JSON input'); | |
| } | |
| TextStyle? parseTextStyle(String propName) { |
| @@ -39,6 +47,16 @@ def __init__( | |||
| baseline_y: OptionalNumber = None, | |||
| min_y: OptionalNumber = None, | |||
| max_y: OptionalNumber = None, | |||
| tooltip_rounded_radius: OptionalNumber = None, | |||
There was a problem hiding this comment.
issue (complexity): Consider using a single dictionary (tooltip_config) to store all tooltip-related settings.
The recent changes to the BarChart class have increased its complexity. Specifically, the addition of numerous tooltip-related attributes and their corresponding getter and setter methods has made the class more verbose and harder to maintain. To simplify, consider using a single dictionary (tooltip_config) to store all tooltip-related settings. This approach reduces the number of attributes and methods, making the class more concise and easier to manage. Additionally, the before_update method can be streamlined to handle this dictionary, further reducing boilerplate code. This change maintains functionality while significantly improving code readability and maintainability.
| tooltip_tooltip_border_side: Optional[BorderSide] = None, | ||
| tooltip_fit_inside_horizontally: Optional[bool] = None, | ||
| tooltip_fit_inside_vertically: Optional[bool] = None, | ||
| tooltip_show_on_top_of_chart_box_area: Optional[bool] = None, |
There was a problem hiding this comment.
issue (complexity): Consider using a dictionary to store tooltip options to reduce redundancy and make the code more maintainable.
The new code introduces a significant amount of complexity due to the addition of numerous tooltip-related attributes and methods. This increases cognitive load and potential for errors. Consider using a dictionary to store tooltip options, which would reduce redundancy and make the code more maintainable. Here's a simplified approach:
class LineChart(ConstrainedControl):
def __init__(
self,
data_series: Optional[List[LineChartData]] = None,
animate: AnimationValue = None,
interactive: Optional[bool] = None,
point_line_start: OptionalNumber = None,
left_axis: Optional[ChartAxis] = None,
top_axis: Optional[ChartAxis] = None,
right_axis: Optional[ChartAxis] = None,
bottom_axis: Optional[ChartAxis] = None,
baseline_x: OptionalNumber = None,
min_x: OptionalNumber = None,
max_x: OptionalNumber = None,
baseline_y: OptionalNumber = None,
min_y: OptionalNumber = None,
max_y: OptionalNumber = None,
tooltip_options: Optional[dict] = None,
on_chart_event: OptionalEventCallable["LineChartEvent"] = None,
ref: Optional[Ref] = None,
width: OptionalNumber = None,
height: OptionalNumber = None,
left: OptionalNumber = None,
top: OptionalNumber = None,
right: OptionalNumber = None,
bottom: OptionalNumber = None,
**kwargs
):
super().__init__(**kwargs)
self.data_series = data_series
self.animate = animate
self.interactive = interactive
self.point_line_start = point_line_start
self.left_axis = left_axis
self.top_axis = top_axis
self.right_axis = right_axis
self.bottom_axis = bottom_axis
self.baseline_x = baseline_x
self.baseline_y = baseline_y
self.min_x = min_x
self.max_x = max_x
self.min_y = min_y
self.max_y = max_y
self.on_chart_event = on_chart_event
self.tooltip_options = tooltip_options or {}
def _get_control_name(self):
return "linechart"
def before_update(self):
super().before_update()
self._set_attr_json("horizontalGridLines", self.__horizontal_grid_lines)
self._set_attr_json("verticalGridLines", self.__vertical_grid_lines)
self._set_attr_json("animate", self.__animate)
self._set_attr_json("border", self.__border)
self._set_attr_json("tooltipOptions", self.tooltip_options)
def _get_children(self):
children = []
for ds in self.__data_series:
children.append(ds)
if self.__left_axis:
self.__left_axis._set_attr_internal("n", "l")
children.append(self.__left_axis)
if self.__top_axis:
self.__top_axis._set_attr_internal("n", "t")
children.append(self.__top_axis)
return children
@property
def tooltip_options(self) -> dict:
return self._tooltip_options
@tooltip_options.setter
def tooltip_options(self, value: dict):
self._tooltip_options = value
# on_chart_event
@property
def on_chart_event(self):
return self.__on_chart_event
@on_chart_event.setter
def on_chart_event(self, handler: OptionalEventCallable["LineChartEvent"]):
self.__on_chart_event.subscribe(handler)
self._set_attr("onChartEvent", True if handler is not None else None)This approach reduces redundancy, makes the code easier to maintain, and keeps the initialization cleaner.
| f"code_style is deprecated since version 0.24.0 " | ||
| f"and will be removed in version 0.27.0. Use code_style_sheet.code_text_style instead.", |
There was a problem hiding this comment.
suggestion (code-quality): Replace f-string with no interpolated values with string (remove-redundant-fstring)
| f"code_style is deprecated since version 0.24.0 " | |
| f"and will be removed in version 0.27.0. Use code_style_sheet.code_text_style instead.", | |
| 'code_style is deprecated since version 0.24.0 and will be removed in version 0.27.0. Use code_style_sheet.code_text_style instead.', |
| f"code_style is deprecated since version 0.24.0 " | ||
| f"and will be removed in version 0.27.0. Use code_style_sheet.code_text_style instead.", |
There was a problem hiding this comment.
suggestion (code-quality): Replace f-string with no interpolated values with string (remove-redundant-fstring)
| f"code_style is deprecated since version 0.24.0 " | |
| f"and will be removed in version 0.27.0. Use code_style_sheet.code_text_style instead.", | |
| 'code_style is deprecated since version 0.24.0 and will be removed in version 0.27.0. Use code_style_sheet.code_text_style instead.', |
Closes #2158 #2160
Summary by Sourcery
This pull request introduces several new features and enhancements to the Markdown, LineChart, and BarChart controls, including new event handling, detailed styling options, and tooltip customization. It also includes significant refactoring and updates to dependencies.