Add RecordingSessions table to SessionsDock
#1654
Conversation
WalkthroughThe recent code changes focused on enhancing the GUI functionality and data handling in the Sleap application. These updates include improvements such as session management, data presentation refinements, and adjustments to the camera handling logic, all aimed at enhancing the user experience and overall system robustness. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
roomrys
changed the title
RecordingSessions table to SessionsDock
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## liezl/add-gui-elements-for-sessions #1654 +/- ##
=======================================================================
- Coverage 73.70% 73.70% -0.01%
=======================================================================
Files 135 135
Lines 24540 24578 +38
=======================================================================
+ Hits 18086 18114 +28
- Misses 6454 6464 +10 ☔ View full report in Codecov by Sentry. |
GUI status
|
roomrys
merged commit 90a2c0e
into
talmolab:liezl/add-gui-elements-for-sessions
![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: 7
Configuration used: CodeRabbit UI
Files selected for processing (8)
- sleap/gui/app.py (4 hunks)
- sleap/gui/commands.py (5 hunks)
- sleap/gui/dataviews.py (2 hunks)
- sleap/gui/widgets/docks.py (5 hunks)
- sleap/io/cameras.py (3 hunks)
- sleap/io/dataset.py (2 hunks)
- tests/gui/test_dataviews.py (2 hunks)
- tests/gui/widgets/test_docks.py (2 hunks)
Additional comments: 13
tests/gui/test_dataviews.py (1)
- 73-98: The test
test_sessions_tableis well-structured and covers the basic functionality of adding and removing videos from a session. However, it's important to ensure that the test also verifies the UI updates accordingly after these operations. Consider adding assertions or checks to confirm that the table view reflects the changes made to the session's videos.tests/gui/widgets/test_docks.py (2)
- 113-120: The test
test_sessions_dockcorrectly initializes theSessionsDockand verifies its basic properties. This is a good practice for ensuring the dock is correctly set up. However, it would be beneficial to also test the interactions within the dock, such as button clicks and table selections, to ensure they trigger the expected actions.- 123-144: The test
test_sessions_dock_session_tableeffectively checks if the sessions table loads correctly and attempts to test the removal of a session. However, since the "Remove Session" functionality is noted as not yet implemented, this test might fail or not behave as expected. It's important to either implement the removal functionality or mark this test as expected to fail or skip until the functionality is available.sleap/gui/widgets/docks.py (3)
- 577-579: The initialization of
SessionsDockwithSessionsTableModelas themodel_typeis correctly done. This ensures that the dock will use the appropriate model for displaying session data. Good use of inheritance to streamline the creation of specific dock types.- 612-622: The
create_tablesmethod properly sets up thesessions_tablewith the necessary state and model. This method ensures that the table is ready to display session data. It's well-implemented and follows the pattern established by other docks in the application. No changes are necessary here.- 637-647: The layout setup in
lay_everything_outmethod is clear and well-organized, ensuring that the sessions table and edit buttons are added to the dock's layout. Including triangulation options is a thoughtful addition for user convenience. This method effectively organizes the dock's UI components.sleap/io/cameras.py (2)
- 585-587: The addition of the
__bool__method to always returnTrueensures that aRecordingSessioninstance is always considered truthy. This is a reasonable default, but ensure it aligns with the intended use cases ofRecordingSessioninstances.- 587-587: Adding
self.camera_cluster.add_session(self)in__attrs_post_init__is a good practice to ensure theRecordingSessionis registered with itsCameraClusterupon creation. This change enhances the maintainability and consistency of session management.sleap/gui/app.py (2)
- 1095-1095: The implementation for checking the presence of a selected session and subsequently enabling the "remove session" button based on this condition seems logically sound. However, it's important to ensure that the
self.state["selected_session"]is updated appropriately elsewhere in the code to reflect the current GUI state accurately.- 1180-1182: Updating the
sessions_tablemodel items directly withself.labels.sessionsis a straightforward approach to reflect changes in the sessions data. However, consider emitting a signal or using a dedicated method for updating the model to encapsulate the update logic better and potentially handle additional tasks such as refreshing the view or handling data conversion if needed in the future.sleap/gui/commands.py (3)
- 85-85: The addition of the
sessionsenum value toUpdateTopicis a good practice for maintaining a clear and extendable way to signal updates related to session management in the GUI.- 448-450: Adding the
removeSelectedSessionmethod toCommandContextprovides a clear and direct way to remove a session from the project. It's good to see encapsulation of functionality within the context.- 1974-1999: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1992-2010]
The updates to the
AddSessionclass, including handling the addition of a session and resetting the selected session state, are well-implemented. It's good to see that the method properly loads the session from the provided camera calibration file and adds it to the project. The reset of theselected_sessionstate ensures a clean state after adding a session.
| def remove_recording_session(self, session: RecordingSession): | ||
| """Remove a session from self.sessions. | ||
|
|
||
| Args: | ||
| session: `RecordingSession` instance | ||
| """ | ||
| if session in self._sessions: | ||
| self._sessions.remove(session) | ||
|
|
There was a problem hiding this comment.
The method remove_recording_session is added to remove a session from self.sessions. This method is straightforward and correctly removes the specified session from the list of sessions. However, it's important to ensure that any references to the removed session in other parts of the Labels class or related classes are also handled appropriately to avoid dangling references.
Consider verifying if there are any additional steps needed when a session is removed, such as updating caches or handling references in other parts of the code.
There was a problem hiding this comment.
Yes, we need to also update the LabelsDataCache._session_by_video dictionary
There was a problem hiding this comment.
@roomrys, you're absolutely right. I'll make sure to update the LabelsDataCache._session_by_video dictionary when a recording session is removed to maintain the integrity of the data structure and ensure correct behavior throughout the codebase. Thank you for pointing this out.
New Learnings Added
User: roomrys
PR: talmolab/sleap#1654
File: sleap/io/dataset.py:1717-1725
Timestamp: 2024-03-15T23:59:34.765Z
Learning: When removing a recording session using the `remove_recording_session` method in the `Labels` class, it's necessary to also update the `LabelsDataCache._session_by_video` dictionary to reflect the removal. This ensures that the mapping between videos and their corresponding recording sessions remains accurate.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure path-based instructions.
Description
The overall goal here is to add a new table to the
SessionsDockthat displays allRecordingSessions in theSessionsDock.main_window.label.sessions: List[RecordingSession]list. The widget should be a table that displays theRecordingSessions hash (eventually name), number ofRecordingSession.cameras: List[Camcorder], and number ofRecordingSession.videos: List[Video](see Figure 1). ThisSessionsTablewill be used to dynamically populate ourCamerasTablelater on.GenericTableModelto create a new model calledSessionsTableModelwith the class attributeproperties = ("id", "videos", "cameras")(similar toVideosTableModel)SessionsTableModel.item_to_data(self, obj: List[RecordingSession], item: RecordingSession)which returns the following dictionary:{id: item.__hash__(), videos: len(item.videos), cameras: len(item.cameras)}(similar toVideosTableModel.item_to_data)sessions_model_type = SessionsTableModelattribute toSessionsDock(similar toSkeletonDock.nodes_model_type) and pass into themodel_typelist forsuper().__init__SessionsDock.create_modelsmethod (or append to it if it already exists) that sets aSessionsDock.sessions_model = SessionsDock.sessions_model_type(items=main_window.state["labels"].sessions, context=main_window.commands)and returns a list of all models created (seeSkeletonDock.create_models)SessionsDock.create_tablesmethod (or append to it if it already exists) that sets aSessionsDock.sessions_table = GenericTableView(state=..., row_name="session", ...)and returns a list of all tables created (seeSkeletonDock.create_tables)CommandContext.add_session(self)RemoveSession(similar toAddSessionwith only ado_actionmethod) that usesLabels.remove_recording_session(session: RecordingSession)(does not exist yet, but it will be named this) to remove theRecordingSessionfrom the labelsCommandContext.remove_session(self, session: RecordingSession)which takes in aRecordingSessionto remove andCommandContext.executes theRemoveSessionclass (similar toCommandContext.add_sessionbut with an additional argument)CommandContext.remove_session(similar to you add sessions button).RecordingSession(see comment onCamerasTablePR)SessionsDock.lay_everything_outmethod, add theSessionsDock.sessions_tableto theSessionsDock.wgt_layout: QVBoxLayout(see theInstancesDock.lay_everything_out).sessionsoption toUpdateTopic: EnumMainWindow.on_data_updateadd (or append to) theif _has_topic(UpdateTopic.sessions)that updates the items to display in theSessionsTable(seevideo_dock.tableupdate)Test the
SessionsTableTests for the
SessionsTablewill be added totests/gui/test_dataviews.py.RecordingSessionis returned when we select a specific row of the table (see existing test for inspiration)RecordingSessioninlabels.sessions, theRecordingSessionin the table is updated as wellTest the
SessionsTableas part of theSessionsDockTests for the
SessionsDockwill be added totests/gui/widgets/test_docks.py.GuiState(see existing test)Figure 1: Depiction of
SessionsTablelayout.Types of changes
Does this address any currently open issues?
[list open issues here]
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
Summary by CodeRabbit
UpdateTopicenum value for sessions.removeSelectedSessionmethod toAppCommandfor session removal.AddSessionclass to handle adding sessions and resetting the selected session state.SessionsTableModelclass with properties forid,videos, andcameras.SessionsDockclass for creating models and tables.find_path_using_pathsfunction for improved file existence checks.