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
Show file tree
Hide file tree
Changes from 2 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
89 changes: 87 additions & 2 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,91 @@ 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):
model.set_show_video_name(self.options["Show Video Name"])
7174Andy marked this conversation as resolved.
Show resolved Hide resolved


class VideosTableModel(GenericTableModel):
properties = ("filename", "frames", "height", "width", "channels")
def __init__(self, show_video_name: bool = False, *args, **kwargs):
super().__init__(*args, **kwargs)
self.show_video_name = show_video_name
self.update_properties()

def update_properties(self):
"""Update properties based on show_video_name attribute."""
if self.show_video_name:
self.properties = (
"filename",
"name",
"frames",
"height",
"width",
"channels",
)
else:
self.properties = ("filename", "frames", "height", "width", "channels")

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
self.show_video_name = show_video_name
self.update_properties()
self.layoutChanged.emit()

def item_to_data(self, obj, item):
return {key: getattr(item, key) for key in self.properties}
return {key: getattr(item, key, None) for key in self.properties}


class SkeletonNodesTableModel(GenericTableModel):
Expand Down
3 changes: 3 additions & 0 deletions sleap/gui/widgets/docks.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,9 @@ def create_tables(self) -> GenericTableView:

return self.table

def create_context_menu(self) -> None:
"""Create the context menu for the dock."""

def create_video_edit_and_nav_buttons(self) -> QWidget:
"""Create the buttons for editing and navigating videos in table."""
main_window = self.main_window
Expand Down
10 changes: 10 additions & 0 deletions sleap/io/video.py
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,11 @@ def dtype(self):
"""See :class:`Video`."""
return self.test_frame.dtype

@property
def name(self):
"""Return the name of the video."""
return os.path.basename(self.filename)

def reset(self, filename: str = None, grayscale: bool = None, bgr: bool = None):
"""Reloads the video."""
if filename is not None:
Expand Down Expand Up @@ -944,6 +949,11 @@ def dtype(self):
"""See :class:`Video`."""
return self.cache_.dtype

@property
def name(self):
"""Name of the video."""
return os.path.basename(self.filename)

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 filename is set when using name property

In SingleImageVideo, the name property returns os.path.basename(self.filename). However, if self.filename is None, this will raise an error. Ensure that self.filename is always set when self.filenames are provided.

Consider modifying the reset method to set self.filename when self.filenames is provided:

     def reset(
         self,
         filename: str = None,
         filenames: List[str] = None,
         height_: int = None,
         width_: int = None,
         channels_: int = None,
         grayscale: bool = None,
     ):
         """Reloads the video."""
         if filename and filenames:
             raise ValueError(
                 f"Cannot specify both filename and filenames for SingleImageVideo."
             )
         elif filename or filenames:
             self.cache_ = dict()
             self.test_frame_ = None
             self.height_ = height_
             self.width_ = width_
             self.channels_ = channels_
 
         if not filename and filenames:
             self.filenames = filenames
             self.filename = filenames[0]
         elif filename and not filenames:
             self.filename = filename
             self.filenames = [filename]
+        else:
+            raise ValueError("Either filename or filenames must be specified.")

Committable suggestion skipped: line range outside the PR's diff.

def reset(
self,
filename: str = None,
Expand Down
Loading