Fix in-cell slots #126
Fix in-cell slots #126
Conversation
WalkthroughThis pull request introduces significant changes across the codebase, focusing on enhancing functionality, improving modularity, and optimizing performance. Key updates include modifications to the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 16
Files selected for processing (29)
- tabulous/main.py (1 hunks)
- tabulous/_async_importer.py (2 hunks)
- tabulous/_magicgui/_dialog.py (1 hunks)
- tabulous/_magicgui/_selection.py (1 hunks)
- tabulous/_psygnal/init.py (1 hunks)
- tabulous/_psygnal/_array.py (1 hunks)
- tabulous/_psygnal/_psygnal_compat.py (1 hunks)
- tabulous/_psygnal/_slots.py (1 hunks)
- tabulous/_psygnal/_special_objects.py (1 hunks)
- tabulous/_qt/_plot/_mpl_canvas.py (7 hunks)
- tabulous/_qt/_plot/_widget.py (8 hunks)
- tabulous/_qt/_table/_base/_enhanced_table.py (4 hunks)
- tabulous/_qt/_table/_base/_line_edit.py (2 hunks)
- tabulous/_qt/_table/_base/_table_base.py (2 hunks)
- tabulous/_qt/_titlebar.py (1 hunks)
- tabulous/_qt/_traceback.py (3 hunks)
- tabulous/_range.py (7 hunks)
- tabulous/_selection_op.py (9 hunks)
- tabulous/_slice_op.py (1 hunks)
- tabulous/_text_formatter.py (1 hunks)
- tabulous/commands/_arange.py (2 hunks)
- tabulous/commands/_dialogs.py (6 hunks)
- tabulous/commands/_optimizer.py (3 hunks)
- tabulous/commands/_plot_models.py (16 hunks)
- tabulous/commands/_sklearn/_widget.py (2 hunks)
- tabulous/widgets/_component/_plot.py (1 hunks)
- tabulous/widgets/_component/_ranges.py (3 hunks)
- tabulous/widgets/_table.py (5 hunks)
- tests/test_cell_ref_eval.py (2 hunks)
Files skipped from review due to trivial changes (7)
- tabulous/main.py
- tabulous/_selection_op.py
- tabulous/_text_formatter.py
- tabulous/commands/_dialogs.py
- tabulous/commands/_optimizer.py
- tabulous/_psygnal/_psygnal_compat.py (diff too large)
- tabulous/_psygnal/_slots.py (diff too large)
Additional comments (Suppressed): 60
tabulous/_psygnal/__init__.py (1)
- 1-4: The new hunk introduces
SignalArrayandInCellRangedSlotfrom the_arrayand_slotsmodules respectively. Ensure that these classes are correctly implemented in their respective modules and that they are used appropriately throughout the codebase.tabulous/_magicgui/_dialog.py (1)
- 107-110: The
QtMplPlotCanvasconstructor now includes a new argumentpickable. Ensure that all calls to this constructor throughout the codebase have been updated to include this argument. Also, verify if settingpickable=Falsehere is intended and does not affect any functionality where pickability of the plot canvas is required.tabulous/_qt/_table/_base/_table_base.py (1)
- 24-30: The import statement for
_slice_opas_slis new. Ensure that the functions from this module are correctly used in the rest of the code.tabulous/_qt/_table/_base/_line_edit.py (2)
12-18: The import of
MultiRectRangehas been removed. Ensure that this class is not used elsewhere in the codebase or that it has been properly replaced with an equivalent functionality.422-433: The method
as_iloc_slices()now includes a new argumentfit_shape=False. Verify that this change does not affect the expected behavior of the function and that all calls to this function have been updated accordingly. Also, the assignment ofnew_rangehas changed fromMultiRectRange.from_slices(ranges)to justranges. Make sure that this change doesn't break any existing functionality wherenew_rangeis used.tabulous/_qt/_titlebar.py (1)
- 56-60: The new method
setBold()is introduced to allow the title text to be set as bold. This change enhances the visual customization of the title bar. However, ensure that this change does not affect the readability or layout of the title bar in different contexts where it is used.tabulous/_qt/_table/_base/_enhanced_table.py (4)
123-124: The
_current_drawing_slot_rangesand_additional_rangesare now initialized as empty lists instead ofNone. This change could potentially affect other parts of the code that check forNoneto determine if these variables have been set. Please verify that all such checks have been updated accordingly.210-214: The
_on_movingmethod has been modified to reset_additional_rangesat the start of each call, and to update_need_update_allbased on whether_current_drawing_slot_rangesis non-empty. This change could potentially affect the behavior of the table when moving between cells. Please ensure that this new behavior is intended and does not introduce any unexpected side effects.222-228: The
_on_movingmethod now sets_current_drawing_slot_rangesto an empty list when there is no slot at the destination index, or when the parent table is not aQTableWidget. Previously, it was set toNonein these cases. As mentioned earlier, this change could potentially affect other parts of the code that check forNoneto determine if_current_drawing_slot_rangeshas been set. Please verify that all such checks have been updated accordingly.628-640: The
paintEventmethod now draws rectangles for both_current_drawing_slot_rangesand_additional_ranges, whereas previously it only drew rectangles for_current_drawing_slot_ranges. This change could potentially affect the appearance of the table, especially if_additional_rangescontains many ranges. Please ensure that this new behavior is intended and does not negatively impact the user experience.tabulous/_async_importer.py (2)
1-7: The
threadingmodule is no longer imported, and theconcurrent.futuresmodule is now imported instead. This change suggests a shift from using individual threads to using a thread pool for asynchronous tasks.22-24: The
ignore_errorparameter has been removed from theget()method. This change means that any exceptions raised during the import will be propagated to the caller, which may need to handle them. Ensure that all calls toget()are updated to handle potential exceptions.tabulous/_qt/_traceback.py (4)
4-8: The import of
QtCoreis new in this hunk. Ensure that it's used in the code sections not included in these hunks.50-50: The
_old_focusattribute is newly introduced and stores the widget that currently has focus. This is likely to restore focus after the message box is closed. Make sure that this doesn't interfere with other parts of the UI that might also manipulate focus.63-64: Shortcut "T" is added for the traceback button. Verify if this shortcut conflicts with any existing shortcuts in the application.
67-72: The
exec_()method now checks if_old_focusis not None before callingsetFocus(). This is a good practice as it prevents potential errors when_old_focusis None. However, ensure that the focus restoration works correctly in all scenarios where the error message box is used.tabulous/commands/_plot_models.py (5)
53-56: The
add_datamethod now requires atableparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.88-98: The
add_datamethod now requires atableparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.186-201: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [157-252]
The
add_datamethod now requires atableparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
241-252: The
add_datamethod now requires atableparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.445-451: The
as_iloc_slicesmethod is now called with an additional argumentfit_shape=False. This seems to be a significant change in how slices are created from the data. Please ensure that this change does not introduce any unexpected behavior or bugs in the rest of the codebase.tabulous/_psygnal/_array.py (9)
40-63: The
SignalArrayclass is a 2D-parametric signal for a table widget that extends thepsygnal.Signalclass. It allows partial slot connection, which means you can connect a slot to the whole table or to a specific range of the table. This is a significant enhancement in terms of flexibility and control over slot connections.65-89: The
__get__method has been overloaded to handle different types of instances. The method returns the instance of theSignalArrayif the instance is None, otherwise it creates a newSignalArrayInstanceand sets it as an attribute of the instance before returning it. This ensures that each instance has its ownSignalArrayInstance.95-113: The
SignalArrayInstanceclass is a parametric version ofSignalInstance. It inherits from bothSignalInstanceandTableAnchorBase. The__getitem__method is overridden to return a sub-array reference, which is useful for accessing specific parts of the array.115-198: The
connectmethod is overloaded to handle different parameters and scenarios. It checks whether the slot is callable and whether it's already connected. If the slot is not callable, a TypeError is raised. If the slot is already connected and the unique parameter is set to "raise", a ValueError is raised. Otherwise, the slot is appended to the_slotslist. There's also aconnect_cell_slotmethod specifically for connecting cell slots.200-253: The
emitmethod is overloaded to handle different parameters and scenarios. It checks whether the signal is blocked or paused. If the signal is blocked, the method simply returns None. If the signal is paused, the arguments are appended to the_args_queueand the method returns None. Otherwise, the_run_emit_loopmethod is called to run the emit loop.255-293: The
insert_rows,insert_columns,remove_rows, andremove_columnsmethods are used to modify the rows and columns of the signal array. They update the slot ranges in-place, which means they directly modify the original data structure instead of creating a new one. This can improve performance when dealing with large data structures.295-340: The
_run_emit_loopmethod runs the emit loop for the signal array. It iterates over all slots and calls each slot with the provided arguments. If a slot is not callable or has been garbage collected, it's removed from the_slotslist. If a slot doesn't overlap with the provided range, it's skipped. Any exceptions raised during the execution of a slot are caught and re-raised as anEmitLoopError.342-401: The
_SignalSubArrayRefclass is a reference to a subarray of a signal. It providesconnectandemitmethods that delegate to the parentSignalArrayInstance. This allows you to connect to and emit signals from a specific part of the signal array.403-426: The
_parse_a_keyand_parse_keyfunctions are utility functions for parsing keys. They handle different types of keys, including slices, lists, numpy arrays, and integers. These functions are used to convert keys into a format that can be used to index into the signal array.tabulous/commands/_sklearn/_widget.py (2)
1-7: The import of
pandashas been removed from the top-level imports and moved to a conditional import block underif TYPE_CHECKING:. This change is likely made to improve the startup time of the script by delaying the import ofpandasuntil it's actually needed for type checking. However, ifpandasis used in the runtime code (not just for type annotations), this could lead to NameError exceptions. Please verify thatpandasis not used outside of type annotations.17-25: The
pandasmodule is now imported inside anif TYPE_CHECKING:block. This approach is typically used to avoid circular imports or to reduce the startup time by importing heavy modules only when type checking. Ensure thatpandasis not used in the runtime code, as this will cause aNameError.tabulous/_range.py (6)
5-5: The import statement for
_slice_ophas been changed. Ensure that the_slice_opmodule exists and contains the necessary functions used in this file.39-41: A new method
as_keys()is added to return a list of (row, column) keys. This could be useful for iterating over the range in a more convenient way.56-56: The
_fmt_slicefunction call has been replaced with_sl.fmt. Make sure that the_slice_opmodule (_sl) contains thefmtfunction and it behaves as expected.187-192: Two new methods
as_keys()and__iter__()are added to theNoRangeclass. These methods seem to provide a consistent interface across different range classes, which can improve code readability and maintainability.226-227: The
as_keys()method is added to theMultiRectRangeclass. It uses a generator expression and thesumfunction withstart=[]to concatenate the results from callingas_keys()on each range. This is an efficient way to flatten a list of lists.293-295: The
_parse_slicefunction is unchanged but the_fmt_slicefunction above it has been removed. If_fmt_sliceis no longer needed, this is fine. Otherwise, make sure that its functionality is covered elsewhere.tabulous/widgets/_component/_plot.py (2)
62-68: The
QtMplPlotCanvasconstructor now requires atableparameter. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.66-66: The
set_background_color()method is called with the name of the background color. Make sure that theset_background_color()method can handle color names and not just RGB values or other color formats.tabulous/commands/_arange.py (1)
- 12-18: The import statement for
_slice_opas_slis added. This module provides utility functions for working with slices, which are used in theget_value()method of the_RangeDialogclass.tabulous/_qt/_plot/_widget.py (7)
25-37: The
pickableandtableparameters have been added to theQtMplPlotCanvasconstructor. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.56-82: The connection of signals to slots in the
QtMplPlotCanvasconstructor is now conditional on thepickableparameter. This change could potentially affect the behavior of instances wherepickableis set toFalse. Verify that this change does not introduce unexpected behavior in such cases.90-110: The
_item_clickedand_repaint_rangesmethods have been added, and theaxesproperty has been moved. The addition of these methods and the movement of theaxesproperty do not appear to introduce any issues.130-161: The
_edit_artistmethod has been modified to call_repaint_rangesand possiblyalign_to_table, depending on whether a table is associated with the canvas. Ensure that these changes do not introduce unexpected behavior when editing an artist.173-179: No significant changes are made in this hunk. The import statement for matplotlib has been moved inside the function, which should not cause any issues.
193-200: The
QMplEditorconstructor now sets the window flags toQt.WindowType.Dialog, which makes the editor behave like a dialog window. Ensure that this change does not introduce unexpected behavior in the UI.205-236: Several methods related to mouse events and alignment have been added to the
QMplEditorclass. These additions do not appear to introduce any issues.tests/test_cell_ref_eval.py (2)
308-308: The status message has changed from "df.iloc[0:3, 0]" to "df.iloc[:, 0]". This change implies that the sum operation now includes all rows of the first column instead of just the first three. Please verify if this is the intended behavior.
327-327: Similar to the previous comment, the status message has changed from "df.iloc[0:5, 0]" to "df.iloc[:, 0]". This change implies that the sum operation now includes all rows of the first column instead of just the first five. Please verify if this is the intended behavior.
tabulous/widgets/_component/_ranges.py (3)
13-19: The import statement for
_slice_ophas been added. Ensure that this module exists and is accessible from the current file's location.40-44: The function
_fmt_slice()has been moved to a different module (_slice_op). This change should not affect functionality as long as the function in the new module behaves identically to the old one.168-170: The
_fmt_slice()function has been removed from this file. If it is used elsewhere in this file, those usages need to be updated to use the function from its new location in the_slice_opmodule.tabulous/_qt/_plot/_mpl_canvas.py (2)
12-31: The
FigureCanvasclass is now conditionally imported based on theTYPE_CHECKINGflag. This change seems to be for type hinting purposes and should not affect runtime behavior. However, theitemPickedsignal has been replaced with two new signals:itemClickedanditemDoubleClicked. Ensure that all connections to the olditemPickedsignal have been updated to connect to the appropriate new signal(s).94-98: The
mouseMoveEventmethod now returnsNoneinstead of just returning when certain conditions are met. This change makes the code more explicit and improves readability.tabulous/widgets/_table.py (5)
91-94: The function signature of
__get__in theDataPropertyclass has been changed. Theownerparameter is now optional and defaults toNone. Ensure that this change does not affect any existing calls to this method.106-109: Similar to the previous comment, the function signature of
__get__in theMetadataPropertyclass has also been changed. Theownerparameter is now optional and defaults toNone. Verify that this change does not impact any existing calls to this method.267-271: The
metadataproperty has been moved from theTableBaseclass to theMetadataPropertyclass. This could potentially break code that relies on accessingmetadatadirectly from an instance ofTableBase. Please ensure all such instances have been updated accordingly.284-287: The docstring for the
table_shapeproperty has been modified to indicate that the shape of the table considers filters. If there are any methods or functions that rely on the old behavior (i.e., not considering filters), they may need to be updated.736-739: The way data is appended to the DataFrame has been changed. Instead of directly setting the
dataattribute, thesetDataFramemethod of_qwidgetis now used. This could potentially have implications for how the data is updated and displayed in the UI. Please verify that this change does not introduce any unexpected behavior.
| table = find_current_table(self) | ||
| if table is None: |
There was a problem hiding this comment.
The _find_table method is missing error handling for when table is None. This could lead to a NoneType error if the method is called elsewhere and the return value is used without checking for None. Consider raising an exception or returning a default value when table is None.
def _find_table(self) -> TableBase:
table = find_current_table(self)
if table is None:
+ raise ValueError("No current table found.")| return pd.Index(di_list) | ||
|
|
||
|
|
||
| def _fmt_slice(sl: slice) -> str: | ||
| return f"{sl.start}:{sl.stop}" | ||
|
|
||
|
|
||
| def _selection_to_literal(sel: tuple[slice, slice]) -> str: | ||
| rsel, csel = sel | ||
| rsize = rsel.stop - rsel.start | ||
| csize = csel.stop - csel.start | ||
| if rsize == 1 and csize == 1: | ||
| txt = f"[{rsel.start}, {csel.start}]" | ||
| elif rsize == 1: | ||
| txt = f"[{rsel.start}, {_fmt_slice(csel)}]" | ||
| txt = f"[{rsel.start}, {_sl.fmt(csel)}]" | ||
| elif csize == 1: | ||
| txt = f"[{_fmt_slice(rsel)}, {csel.start}]" | ||
| txt = f"[{_sl.fmt(rsel)}, {csel.start}]" | ||
| else: | ||
| txt = f"[{_fmt_slice(rsel)}, {_fmt_slice(csel)}]" | ||
| txt = f"[{_sl.fmt(rsel)}, {_sl.fmt(csel)}]" | ||
| return txt | ||
|
|
There was a problem hiding this comment.
The function _selection_to_literal has been refactored to use the fmt function from the _slice_op module instead of the previously defined _fmt_slice function. This change should not affect the functionality if the fmt function from _slice_op behaves identically to _fmt_slice.
- txt = f"[{rsel.start}, {_fmt_slice(csel)}]"
+ txt = f"[{rsel.start}, {_sl.fmt(csel)}]"| self._qtable = weakref.ref(qtable) | ||
|
|
||
| def __int__(self) -> int: | ||
| return len(self._qtable().getDataFrame()) |
There was a problem hiding this comment.
The __int__ method is directly accessing the _qtable weak reference without checking if it's still alive. This could lead to a TypeError if the referenced object has been garbage collected. It would be better to first check if the weak reference is still valid before trying to access its methods.
- return len(self._qtable().getDataFrame())
+ qtable = self._qtable()
+ if qtable is None:
+ raise ReferenceError("Referenced QMutableTable object has been destroyed")
+ return len(qtable.getDataFrame())| def __repr__(self) -> str: | ||
| return str(int(self)) | ||
|
|
||
| def __index__(self) -> int: |
There was a problem hiding this comment.
The __index__ method is also directly accessing the _qtable weak reference without checking if it's still alive. This could lead to a TypeError if the referenced object has been garbage collected. It would be better to first check if the weak reference is still valid before trying to access its methods.
- return self.__int__()
+ qtable = self._qtable()
+ if qtable is None:
+ raise ReferenceError("Referenced QMutableTable object has been destroyed")
+ return len(qtable.getDataFrame())| def len_1(sl: slice) -> bool: | ||
| """True if the slice is of length 1.""" | ||
| if sl.start is None: | ||
| return sl.stop == 1 | ||
| elif sl.stop is None: | ||
| return False | ||
| return sl.stop - sl.start == 1 |
There was a problem hiding this comment.
The function len_1 checks if a slice is of length 1. However, it does not consider the step attribute of the slice object. If the step is not None and not equal to 1, the result might be incorrect.
| """True if the range is empty.""" | ||
| r0_s, r1_s = self._rsl.start, self._rsl.stop | ||
| c0_s, c1_s = self._csl.start, self._csl.stop | ||
| if r0_s is None or r1_s is None or c0_s is None or c1_s is None: | ||
| return False | ||
| return self._rsl.start >= self._rsl.stop or self._csl.start >= self._csl.stop | ||
| r0, r1 = self._rsl.start, self._rsl.stop | ||
| c0, c1 = self._csl.start, self._csl.stop | ||
| r_empty = r0 is not None and r1 is not None and r0 >= r1 | ||
| c_empty = c0 is not None and c1 is not None and c0 >= c1 | ||
| return r_empty or c_empty |
There was a problem hiding this comment.
The logic for checking if the range is empty has been modified. The new implementation seems to be more efficient as it directly checks if the start is greater than or equal to the stop for both rows and columns. However, ensure that this change doesn't introduce any unexpected behavior when dealing with ranges where the start or stop is None.
|
|
||
| def get_value(self, df: pd.DataFrame) -> tuple[slice, slice, pd.Index]: | ||
| rsl, csl = self._selection.value.as_iloc_slices(df) | ||
| if csl.start != csl.stop - 1: | ||
| if not _sl.len_1(csl): | ||
| raise ValueError("Selection must be a single column") | ||
| periods = rsl.stop - rsl.start | ||
| start, end, freq = self._get_params() |
There was a problem hiding this comment.
The condition to check if the column slice (csl) represents a single column has been changed from csl.start != csl.stop - 1 to not _sl.len_1(csl). The new condition uses the len_1() function from the _slice_op module, which checks if the length of the slice is 1. This change makes the code more readable and maintainable by encapsulating the logic for checking the length of a slice into a separate function.
- if csl.start != csl.stop - 1:
+ if not _sl.len_1(csl):| sheet = viewer.add_spreadsheet([[0, 1], [0, 2], [0, 3]]) | ||
| sheet.cell[1, 2] = "&=np.sum(df.iloc[:, 1])" | ||
| assert sheet.data.iloc[1, 2] == 6 | ||
| sheet.cell[3, 1] = "4" | ||
| assert sheet.data.iloc[1, 2] == 10 | ||
|
|
||
| # check vector output | ||
| sheet = viewer.add_spreadsheet([[0, 1], [0, 2], [0, 3]]) | ||
| sheet.cell[1, 2] = "&=np.cumsum(df.iloc[:, 1])" | ||
| assert_equal(sheet.data.iloc[:, 2].values, [1, 3, 6]) | ||
| sheet.cell[3, 1] = "4" | ||
| assert_equal(sheet.data.iloc[:, 2].values, [1, 3, 6, 10]) | ||
|
|
||
| def test_N(make_tabulous_viewer): | ||
| viewer: TableViewer = make_tabulous_viewer() | ||
| sheet = viewer.add_spreadsheet([[0, 1], [0, 2], [0, 3]]) | ||
| sheet.cell[1, 2] = "&=np.sum(df.iloc[:, 1])/N" | ||
| assert sheet.data.iloc[1, 2] == 2 | ||
| sheet.cell[3, 1] = "4" | ||
| assert sheet.data.iloc[1, 2] == 2.5 |
There was a problem hiding this comment.
This new test case checks the behavior of scalar and vector outputs when the spreadsheet data is updated. It also tests the use of a special variable N in the expression. The tests seem comprehensive and cover different scenarios. However, it would be beneficial to add comments explaining what each part of the test is checking for better maintainability.
| for container in ax.containers: | ||
| # if an artist is in a container, emit the container instead | ||
| if artist in container: | ||
| self.itemPicked.emit(container) | ||
| artist = container | ||
| break | ||
| if event.mouseevent.dblclick: | ||
| self.itemDoubleClicked.emit(artist) | ||
| else: | ||
| self.itemPicked.emit(artist) | ||
| self.itemClicked.emit(artist) | ||
|
|
There was a problem hiding this comment.
The logic for emitting signals when an artist is picked has changed. Previously, if an artist was in a container, the itemPicked signal would emit the container. Now, the artist itself is emitted instead of the container. This could potentially break any code that expects the container to be emitted when an artist is picked.
|
|
||
| self.figure.tight_layout() | ||
| self.figure.canvas.draw() | ||
| return None | ||
| return super().mouseDoubleClickEvent(event) |
There was a problem hiding this comment.
The mousePressEvent, mouseReleaseEvent, and mouseDoubleClickEvent methods no longer return None explicitly. Instead, they return the result of calling the superclass's corresponding method. This change should not cause any issues as long as the superclass's methods do not return any unexpected values.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- tabulous/_qt/_plot/_mpl_canvas.py (9 hunks)
Additional comments (Suppressed): 5
tabulous/_qt/_plot/_mpl_canvas.py (5)
12-31: The
InteractiveFigureCanvasclass has been modified to emit different signals based on the type of mouse click event. TheitemPickedsignal has been replaced withitemClickedanditemDoubleClicked. Ensure that all parts of the codebase that rely on these signals have been updated accordingly.48-56: The logic for emitting signals when an artist is picked has been changed. Previously, the
itemPickedsignal was emitted with the container as the argument if the artist was in a container. Now, the artist itself is emitted instead. This could potentially affect how the signal is handled elsewhere in the codebase.239-246: The calculation for setting the x limits when the x scale is linear has been simplified. The new calculation directly adds and subtracts the change in x (
dx) from the old limits. This change should not affect the functionality of the code, but it does make the calculation easier to understand.252-259: Similar to the changes made for the x limits, the calculation for setting the y limits when the y scale is linear has been simplified. The new calculation directly adds and subtracts the change in y (
dy) from the old limits. This change should not affect the functionality of the code, but it does make the calculation easier to understand.294-305: A new helper function
_get_xy_data()has been introduced. This function attempts to transform the mouse event coordinates into data coordinates using theAxesobject's transformation. If this fails, it falls back to using the mouse event's data coordinates. This function improves code modularity by encapsulating this logic into a separate function.
| if mouse_event.inaxes: | ||
| self.pressed = mouse_event.button | ||
| self.last_axis = mouse_event.inaxes | ||
| # self.clicked.emit(mouse_event) | ||
| return None | ||
| return super().mousePressEvent(event) | ||
|
|
||
| def mouseMoveEvent(self, event): | ||
| """ | ||
| Translate axes focus while dragging. If there are subplots, only the subplot in which | ||
| cursor exists will be translated. | ||
| Translate axes focus while dragging. If there are subplots, only | ||
| the subplot in which cursor exists will be translated. | ||
| """ | ||
| ax = self.last_axis | ||
| if ( | ||
| self.pressed not in (MouseButton.LEFT, MouseButton.RIGHT) | ||
| or self.lastx_pressed is None | ||
| or not self._interactive | ||
| or not ax | ||
| ): | ||
| if self.lastx_pressed is None or not self._interactive or not ax: | ||
| return | ||
|
|
||
| event = self.get_mouse_event(event) | ||
| x, y = event.xdata, event.ydata | ||
| xdata, ydata = _get_xy_data(ax, event) | ||
|
|
||
| if x is None or y is None: | ||
| if xdata is None or ydata is None: | ||
| return None | ||
|
|
||
| if self.pressed == MouseButton.LEFT: | ||
| _translate_x(ax, self.lastx_pressed, x) | ||
| _translate_y(ax, self.lasty_pressed, y) | ||
| _translate_x(ax, self.lastx_pressed, xdata) | ||
| _translate_y(ax, self.lasty_pressed, ydata) | ||
| elif self.pressed == MouseButton.RIGHT: | ||
| _zoom_x(ax, self.lastx, x) | ||
| _zoom_y(ax, self.lasty, y) | ||
| _zoom_x(ax, self.lastx, xdata) | ||
| _zoom_y(ax, self.lasty, ydata) | ||
| xdata, ydata = _get_xy_data(ax, event) # ticks changed! | ||
| else: | ||
| return None | ||
|
|
||
| self.lastx, self.lasty = x, y | ||
| self.lastx, self.lasty = xdata, ydata | ||
| self.figure.canvas.draw() | ||
| return None | ||
|
|
There was a problem hiding this comment.
The conditions for handling mouse move events have been simplified. Previously, the function would return early if the pressed button was not left or right, or if other conditions were not met. Now, it only returns early if self.lastx_pressed is None, self._interactive is False, or ax is None. This change could potentially allow more mouse move events to be processed than before.
- if (
- self.pressed not in (MouseButton.LEFT, MouseButton.RIGHT)
- or self.lastx_pressed is None
- or not self._interactive
- or not ax
- ):
+ if self.lastx_pressed is None or not self._interactive or not ax:
return
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (5)
- tabulous/main.py (2 hunks)
- tabulous/_psygnal/_slots.py (1 hunks)
- tabulous/_qt/_console/_widget.py (1 hunks)
- tests/test_cell_ref_eval.py (2 hunks)
- tests/test_proxy.py (1 hunks)
Files skipped from review due to trivial changes (4)
- tabulous/main.py
- tests/test_cell_ref_eval.py
- tests/test_proxy.py
- tabulous/_psygnal/_slots.py (diff too large)
Additional comments (Suppressed): 1
tabulous/_qt/_console/_widget.py (1)
- 142-143: The addition of the line magic
%matplotlib inlineis a significant change. This magic command configures the integration of Matplotlib with Jupyter (or IPython) such that the output of plotting commands is displayed inline within the notebook, directly below the code cell that produced it. The resulting plots are stored in the notebook document. Ensure that this change doesn't affect any existing functionality or user experience, especially if there are other parts of the codebase that rely on a different matplotlib backend.
This PR allow using
slice(None)as the in-cell slots.Summary by CodeRabbit
AsyncImporterclass for asynchronous imports, improving application responsiveness.SignalArrayandInCellRangedSlotclasses to enhance signal handling and cell evaluation in tables.pickableparameter toQtMplPlotCanvasconstructor, allowing control over plot element interactivity.as_ilocandas_iloc_slices, simplifying codebase.ThreadPoolExecutorinAsyncImporter, enhancing performance and maintainability.FigureCanvasimport logic to support different backends, improving compatibility.test_cell_ref_eval.pyfor scalar and vector outputs and the use of theNvariable.