Skip to content

Fix/423 playback fps label#877

Open
Tushar7012 wants to merge 2 commits intoneuroinformatics-unit:mainfrom
Tushar7012:fix/423-playback-fps-label
Open

Fix/423 playback fps label#877
Tushar7012 wants to merge 2 commits intoneuroinformatics-unit:mainfrom
Tushar7012:fix/423-playback-fps-label

Conversation

@Tushar7012
Copy link
Contributor

Description

What is this PR?

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

Users of the napari plugin were confused between two different FPS values:

  • Acquisition FPS — shown in the "Load poses" panel, represents the fps
    at which video/pose data was recorded
  • Playback FPS — the fps at which napari actually plays back the video

There was no visual indicator of the current playback fps, causing users to
mistake the acquisition fps as the playback speed. This was raised in Issue #423
and sparked from a discussion during PR #393 review.

What does this PR do?

Adds a new read-only PlaybackFPSWidget to the napari plugin that:

  • Reads get_settings().application.playback_fps on init to display the
    current playback FPS
  • Subscribes to get_settings().application.events.playback_fps to
    reactively update the label whenever the user changes playback speed
  • Exposes a QLabel with objectName="playback_fps_label" for testability
  • Is embedded as a non-collapsible second section in MovementMetaWidget

Note: The issue template suggested viewer.dims.fps — this does not
exist in the current napari version. The correct public API is
get_settings().application.playback_fps.


References


How has this PR been tested?

Two new unit tests added in test_data_loader_widget.py:

Test What it checks
test_playback_fps_widget_instantiation Label shows correct initial FPS from get_settings() on widget creation
test_playback_fps_label_updates_on_fps_change Setting playback_fps fires the event and label text updates correctly

Full verification results:

Check Result
ruff check movement/napari/ ✅ All checks passed
pytest -k "fps" (7 tests) ✅ All passed
pytest tests/test_unit/test_napari_plugin/ (102 tests) ✅ All passed, exit code 0
No private napari attributes used ✅ Only public get_settings() API

Is this a breaking change?

No. This PR only adds a new widget. No existing functionality has been
modified or removed. The acquisition FPS spinbox in the Load panel remains
completely unchanged.


Does this PR require an update to the documentation?

Yes. The user guide should be updated to:

  • Mention the new Playback FPS label in the plugin UI
  • Clarify the difference between acquisition FPS and playback FPS
    more prominently so users are not confused

Files Changed

File Change
movement/napari/loader_widgets.py Added PlaybackFPSWidget class
movement/napari/meta_widget.py Imported and integrated PlaybackFPSWidget
tests/test_unit/test_napari_plugin/test_data_loader_widget.py Added 2 new fps tests
tests/test_unit/test_napari_plugin/test_meta_widget.py Updated widget count assertion to len == 2

Checklist

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

Copilot AI review requested due to automatic review settings March 8, 2026 06:45
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a read-only Playback FPS indicator to the napari plugin UI so users can distinguish napari’s playback speed from the acquisition FPS shown elsewhere, and also introduces new Zarr I/O documentation/tests alongside a small integration-test fixture refactor.

Changes:

  • Add PlaybackFPSWidget that displays get_settings().application.playback_fps and updates reactively on settings changes.
  • Integrate the widget into MovementMetaWidget as a second (non-collapsible) section.
  • Add Zarr save/load integration tests and corresponding user-guide documentation; refactor shared integration fixtures into tests/test_integration/conftest.py.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
movement/napari/loader_widgets.py Adds PlaybackFPSWidget that subscribes to napari settings events and updates a label.
movement/napari/meta_widget.py Embeds PlaybackFPSWidget into the plugin meta container.
tests/test_unit/test_napari_plugin/test_data_loader_widget.py Adds unit tests for Playback FPS label initialization and reactive updates.
tests/test_integration/test_zarr.py Adds integration coverage for Zarr roundtrip I/O via xarray.
tests/test_integration/test_netcdf.py Removes locally-defined fixtures in favor of shared integration fixtures.
tests/test_integration/conftest.py Adds shared integration fixtures (processed/derived/datetime-index datasets).
docs/source/user_guide/input_output.md Adds a new user-guide section describing Zarr save/load usage.
docs/source/conf.py Updates Sphinx linkcheck ignore list for Zarr/xarray docs domains.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +951 to +956
new_fps = 42
get_settings().application.playback_fps = new_fps

label = widget.findChild(QLabel, "playback_fps_label")
assert label is not None, "playback_fps_label widget not found."
assert label.text() == f"Playback FPS: {new_fps}"
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

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

This test mutates the global napari setting get_settings().application.playback_fps but never restores the previous value. That can make other tests order-dependent (they may see a non-default playback_fps). Save the original playback_fps at the start of the test and restore it in a finally block (or use a fixture) after the assertion.

Suggested change
new_fps = 42
get_settings().application.playback_fps = new_fps
label = widget.findChild(QLabel, "playback_fps_label")
assert label is not None, "playback_fps_label widget not found."
assert label.text() == f"Playback FPS: {new_fps}"
original_fps = get_settings().application.playback_fps
new_fps = 42
try:
get_settings().application.playback_fps = new_fps
label = widget.findChild(QLabel, "playback_fps_label")
assert label is not None, "playback_fps_label widget not found."
assert label.text() == f"Playback FPS: {new_fps}"
finally:
get_settings().application.playback_fps = original_fps

Copilot uses AI. Check for mistakes.
periods=ds.sizes["time"],
freq=pd.Timedelta(seconds=1),
)
ds.assign_coords(time=timestamps)
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

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

xarray.Dataset.assign_coords(...) returns a new Dataset and does not modify ds in-place. As written, this fixture returns the original dataset without a DateTimeIndex time coordinate, so the integration tests are not actually exercising the intended datetime-index case. Reassign the result (e.g., ds = ds.assign_coords(...)) before returning.

Suggested change
ds.assign_coords(time=timestamps)
ds = ds.assign_coords(time=timestamps)

Copilot uses AI. Check for mistakes.
Comment on lines +455 to +462
(target-zarr)=
## Saving and loading with Zarr

[Zarr](https://zarr.readthedocs.io/en/stable/) is an open format for storing
chunked, compressed N-dimensional arrays. It is particularly well-suited
for large datasets and cloud or remote storage.
Like netCDF, Zarr is natively supported by xarray.

Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

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

This PR is scoped/described as adding a napari Playback FPS label, but this hunk introduces a new Zarr I/O section in the user guide. If Zarr documentation is intended, it should be called out explicitly in the PR description/title (or split into a separate PR) to keep review and release notes accurate.

Copilot uses AI. Check for mistakes.
@codecov
Copy link

codecov bot commented Mar 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (8d49d6a) to head (47f7e6c).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #877   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           38        38           
  Lines         2284      2297   +13     
=========================================
+ Hits          2284      2297   +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Tushar7012 Tushar7012 force-pushed the fix/423-playback-fps-label branch from 03541c0 to c3102bf Compare March 8, 2026 07:51
@Tushar7012 Tushar7012 force-pushed the fix/423-playback-fps-label branch from c3102bf to 36f0383 Compare March 8, 2026 08:07
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider showing playback fps on top of the video in the napari plugin

2 participants