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

Fix segfault in test #128

Closed
wants to merge 19 commits into from
Closed

Fix segfault in test #128

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
756aade
delete mainwindows-
hanjinliu Sep 24, 2023
7005429
switch files
hanjinliu Sep 24, 2023
3940ad1
update conftest
hanjinliu Sep 24, 2023
d04c1d7
update gc
hanjinliu Sep 24, 2023
910e67d
try gc.collect()
hanjinliu Sep 24, 2023
a8d498d
unlink events
hanjinliu Sep 24, 2023
0b07ff9
cleanup on close
hanjinliu Sep 24, 2023
f1457d0
try removing as_toml
hanjinliu Sep 24, 2023
e1bdffd
try removing all the close event
hanjinliu Sep 24, 2023
8ac2fcd
try with close event again
hanjinliu Sep 24, 2023
8c9ffe7
try test_main_window
hanjinliu Sep 24, 2023
b351dff
comment-out test funcs
hanjinliu Sep 24, 2023
090b27a
comment out more
hanjinliu Sep 24, 2023
a2d0889
stop using TableViewerWidget
hanjinliu Sep 24, 2023
266f181
reset deleted files
hanjinliu Sep 24, 2023
d74d313
use make tabulous viewer
hanjinliu Sep 24, 2023
aa229a5
comment out
hanjinliu Sep 24, 2023
c281edb
uncomment
hanjinliu Sep 24, 2023
31652d4
uncomment
hanjinliu Sep 24, 2023
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
2 changes: 1 addition & 1 deletion tabulous/_qt/_mainwindow/_mainwidgets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ def event(self, e: QEvent):
QMainWindow._instances.remove(self)
except ValueError:
pass
get_config().as_toml() # save config
# get_config().as_toml() # save config

elif type in _REORDER_INSTANCES:
# upon activation or raise_, put window at the end of _instances
Expand Down
13 changes: 13 additions & 0 deletions tabulous/_qt/_table/_base/_table_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,12 @@ def dataShape(self) -> tuple[int, int]:
def dataShapeRaw(self) -> tuple[int, int]:
return self.getDataFrame().shape

def connectItemChangedSignal(self, *args, **kwargs) -> None:
pass

def disconnectItemChangedSignal(self):
pass
Comment on lines +233 to +237
Copy link

Choose a reason for hiding this comment

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

The connectItemChangedSignal and disconnectItemChangedSignal methods are defined but do not perform any operations. If these methods are intended to be overridden in subclasses, consider making them abstract methods or adding a comment to clarify their purpose.

from abc import ABC, abstractmethod

class QMutableSimpleTable(ABC):
    # ...

    @abstractmethod
    def connectItemChangedSignal(self, *args, **kwargs) -> None:
        pass

    @abstractmethod
    def disconnectItemChangedSignal(self):
        pass


def zoom(self) -> float:
"""Get current zoom factor."""
return self._qtable_view.zoom()
Expand Down Expand Up @@ -1222,6 +1228,13 @@ def connectItemChangedSignal(
self.evaluatedSignal.connect(slot_eval)
return None

def disconnectItemChangedSignal(self):
self.itemChangedSignal.disconnect()
self.rowChangedSignal.disconnect()
self.columnChangedSignal.disconnect()
self.evaluatedSignal.disconnect()
return None
Comment on lines +1231 to +1236
Copy link

Choose a reason for hiding this comment

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

The disconnectItemChangedSignal method disconnects several signals without checking if they were connected in the first place. This could raise a TypeError if the signal was not connected. Consider adding error handling or checks to ensure that the signals are connected before attempting to disconnect them.

def disconnectItemChangedSignal(self):
    for signal in [self.itemChangedSignal, self.rowChangedSignal, self.columnChangedSignal, self.evaluatedSignal]:
        try:
            signal.disconnect()
        except TypeError:
            pass
    return None


def keyPressEvent(self, e: QtGui.QKeyEvent):
keys = QtKeys(e)
if not self._keymap.press_key(keys):
Expand Down
18 changes: 18 additions & 0 deletions tabulous/widgets/_mainwindow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,9 @@ def paste_data(

def close(self):
"""Close the viewer."""
for table in self.tables:
table.native.disconnectItemChangedSignal()
self.tables.clear()
return self._qwidget.close()

@property
Expand Down Expand Up @@ -581,6 +584,21 @@ def _pass_pytable(src, index: int, dst):
_tablist.events.changed.connect(self.reset_choices)
_tablist.events.renamed.connect(self.reset_choices)

def _unlink_events(self):
_tablist = self.tables
_qtablist = self._qwidget._tablestack

_tablist.events.inserted.disconnect()
_tablist.events.removed.disconnect()
_tablist.events.moved.disconnect()
_tablist.events.renamed.disconnect()

_qtablist.itemMoved.disconnect()
_qtablist.tableRenamed.disconnect()
_qtablist.tableRemoved.disconnect()
_qtablist.tablePassed.disconnect()
_qtablist.itemDropped.disconnect()
Comment on lines +587 to +600
Copy link

Choose a reason for hiding this comment

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

The _unlink_events method has been added to disconnect various events from tables and _qwidget._tablestack objects. This is a good practice for resource management and preventing potential issues such as memory leaks. However, it's important to ensure that this method is called whenever these objects are no longer needed or before they are deleted. Also, consider handling potential exceptions that might occur during disconnection of signals.



class TableViewerWidget(TableViewerBase):
"""The non-main table viewer widget."""
Expand Down
11 changes: 9 additions & 2 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import pytest
from weakref import WeakSet
import gc

@pytest.fixture
def make_tabulous_viewer(qtbot):
Expand All @@ -15,14 +16,16 @@ def factory(show=False):
yield factory

for viewer in viewers:
viewer._unlink_events()
viewer.close()
viewer.native.deleteLater()
Copy link

Choose a reason for hiding this comment

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

The deleteLater() method is called on the native attribute of the viewer object. This assumes that every viewer has a native attribute. If there's a chance that some viewers might not have this attribute, this could raise an AttributeError. Consider adding a check to ensure that the native attribute exists before calling deleteLater().

- viewer.native.deleteLater()
+ if hasattr(viewer, 'native'):
+     viewer.native.deleteLater()



@pytest.fixture(scope="session", autouse=True)
def session():
from tabulous._utils import init_config, update_config, get_config
from tabulous._qt._mainwindow import QMainWindow
import gc
from qtpy.QtWidgets import QApplication
Copy link

Choose a reason for hiding this comment

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

The import statement for QApplication is indented, which is not typical for Python. Imports are usually at the top of the file or function and not indented unless they are inside a conditional block. If this indentation is not intentional, it should be corrected.

-    from qtpy.QtWidgets import QApplication
+from qtpy.QtWidgets import QApplication


with init_config():
cfg = get_config()
Expand All @@ -41,7 +44,11 @@ def session():
instance.close()
instance.deleteLater()

QApplication.closeAllWindows()
QMainWindow._instances.clear()
N_PROCESS_EVENTS = 50
for _ in range(N_PROCESS_EVENTS):
QApplication.processEvents()
gc.collect()
if QMainWindow._instances:
raise RuntimeError("QMainWindow instances not cleaned up!")
QMainWindow._instances.clear()
19 changes: 0 additions & 19 deletions tests/test_auto_completion.py
View file
Open in desktop

This file was deleted.

119 changes: 0 additions & 119 deletions tests/test_cell_eval.py
View file
Open in desktop

This file was deleted.

Loading