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.

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

Separate the video name and its filepath columns in VideoTablesModel #2052

Merged
merged 18 commits into from
Dec 18, 2024
Merged
Changes from 7 commits
Commits
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
111 changes: 108 additions & 3 deletions sleap/gui/dataviews.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ class GenericTableView(QtWidgets.QTableView):
name_prefix: str = "selected_"
is_activatable: bool = False
is_sortable: bool = False
show_video_name: bool = False

def __init__(
self,
Expand All @@ -305,12 +306,17 @@ def __init__(
):
super(GenericTableView, self).__init__()

model.show_video_name = self.show_video_name

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use setter method to update model properties

Directly assigning model.show_video_name = self.show_video_name does not update the model's properties or refresh the layout. Instead, you should use the set_show_video_name method to ensure the model updates correctly.

Apply this diff to fix the issue:

-            model.show_video_name = self.show_video_name
+            model.set_show_video_name(self.show_video_name)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
model.show_video_name = self.show_video_name
model.set_show_video_name(self.show_video_name)

self.state = state or GuiState()
self.row_name = row_name or self.row_name
self.name_prefix = name_prefix if name_prefix is not None else self.name_prefix
self.is_sortable = is_sortable or self.is_sortable
self.is_activatable = is_activatable or self.is_activatable
self.multiple_selection = multiple_selection
self.options = {
"Show Video Name": self.show_video_name,
}

self.setModel(model)

Expand Down Expand Up @@ -384,12 +390,111 @@ def getSelectedRowItem(self) -> Any:
return None
return self.model().original_items[idx.row()]

def mousePressEvent(self, event) -> None:
"""Only for right-click on VideosTableView.

Args:
event (QMouseEvent): The mouse event.
"""
if event.button() == QtCore.Qt.RightButton and isinstance(
self.model(), VideosTableModel
):
self.show_context_menu(event)
super().mousePressEvent(event)

def show_context_menu(self, event):
"""Show context menu for VideosTableView.

Args:
event (QMouseEvent): The mouse event.
"""
menu = QtWidgets.QMenu(self)

# Add actions to the menu
for option, is_checked in self.options.items():
action = QtWidgets.QAction(option, self)
action.setCheckable(True)
action.setChecked(is_checked)
action.triggered.connect(self.create_checked_lambda(option))
menu.addAction(action)

# Show the context menu
menu.exec_(event.globalPos())

def create_checked_lambda(self, option):
"""Callback for context menu actions.

Args:
option (dict): The option to toggle.

Returns:
function: The callback function.
"""
return lambda checked: self.toggle_option(option, checked)

def toggle_option(self, option, checked):
"""Toggle the option in the context menu.

Args:
option (str): The option to toggle.
checked (bool): The new value for the option.
"""
self.options[option] = checked
model = self.model()
if isinstance(model, VideosTableModel):
if option == "Show Video Name":
model.set_show_video_name(self.options["Show Video Name"])


# TODO: Fix the error in the Pytest.
class VideosTableModel(GenericTableModel):
properties = ("filename", "frames", "height", "width", "channels")
def __init__(self, items, show_video_name=False, **kwargs):
super().__init__(**kwargs)
self.items = items
self.show_video_name = show_video_name
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure proper initialization by passing items to the superclass constructor

In the __init__ method of VideosTableModel, items is set directly with self.items = items, but it is not passed to the superclass constructor. To ensure that the superclass GenericTableModel initializes properly and handles the items correctly, consider passing items to the superclass __init__ method.

Apply this diff to fix the issue:

     def __init__(self, items, show_video_name=False, **kwargs):
-        super().__init__(**kwargs)
-        self.items = items
+        super().__init__(items=items, **kwargs)
         self.show_video_name = show_video_name
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def __init__(self, items, show_video_name=False, **kwargs):
super().__init__(**kwargs)
self.items = items
self.show_video_name = show_video_name
def __init__(self, items, show_video_name=False, **kwargs):
super().__init__(items=items, **kwargs)
self.show_video_name = show_video_name

self.properties = (
"filepath",
"name",
"frames",
"height",
"width",
"channels",
)
# TODO: Always with filepath and name (no filename)

def headerData(self, section, orientation, role=QtCore.Qt.DisplayRole):
if orientation == QtCore.Qt.Horizontal and role == QtCore.Qt.DisplayRole:
return self.properties[section].title()
return super().headerData(section, orientation, role)

def _get_properties(self):
"""Return properties based on the show_video_name flag."""
if self.show_video_name:
return ["filepath", "name", "frames", "height", "width", "channels"]
return ["filename", "frames", "height", "width", "channels"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want "filename" or "name" to be the column name?


def set_show_video_name(self, show_video_name: bool):
"""Set whether to show video name in table."""
if self.show_video_name == show_video_name:
return

def item_to_data(self, obj, item):
return {key: getattr(item, key) for key in self.properties}
# Reset the table so that new columns are added
self.show_video_name = show_video_name
self.properties = self._get_properties()
self.beginResetModel()
self.endResetModel()

def item_to_data(self, obj, item: "Video"):
data = {}
for property in self.properties:
if property == "name":
data[property] = getattr(item, "filename").split("/")[-1]
7174Andy marked this conversation as resolved.
Show resolved Hide resolved
elif property == "filepath":
splitted = getattr(item, "filename").split("/")[:-1]
7174Andy marked this conversation as resolved.
Show resolved Hide resolved
data[property] = "/".join(splitted)
else:
data[property] = getattr(item, property)
return data
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use os.path functions for path manipulations to ensure cross-platform compatibility

Currently, the code splits the file path using split("/"), which may not work correctly on all operating systems (e.g., Windows uses backslashes). To ensure cross-platform compatibility, consider using os.path.basename and os.path.dirname for handling file paths.

Apply this diff to update the code:

 import os  # Ensure this import is at the top of the file

 def item_to_data(self, obj, item: "Video"):
     data = {}
     for property in self.properties:
         if property == "name":
-            data[property] = getattr(item, "filename").split("/")[-1]
+            data[property] = os.path.basename(item.filename)
         elif property == "filepath":
-            splitted = getattr(item, "filename").split("/")[:-1]
-            data[property] = "/".join(splitted)
+            data[property] = os.path.dirname(item.filename)
         else:
             data[property] = getattr(item, property)
     return data

This change ensures that file path manipulations are handled correctly across different operating systems.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def item_to_data(self, obj, item: "Video"):
data = {}
for property in self.properties:
if property == "name":
data[property] = getattr(item, "filename").split("/")[-1]
elif property == "filepath":
splitted = getattr(item, "filename").split("/")[:-1]
data[property] = "/".join(splitted)
else:
data[property] = getattr(item, property)
return data
def item_to_data(self, obj, item: "Video"):
data = {}
for property in self.properties:
if property == "name":
data[property] = os.path.basename(item.filename)
elif property == "filepath":
data[property] = os.path.dirname(item.filename)
else:
data[property] = getattr(item, property)
return data



class SkeletonNodesTableModel(GenericTableModel):
Expand Down
Loading