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

add support for setting saved view in set-view operator #4159

Merged
merged 1 commit into from Mar 15, 2024

Conversation

imanjra
Copy link
Contributor

@imanjra imanjra commented Mar 14, 2024

What changes are proposed in this pull request?

add support for setting a saved view with the set_view built-in operator

Usage: setting a view using the name of a saved view

def execute(self, ctx):
    ctx.trigger("set_view", {"name": "my-saved-view-name"})

How is this patch tested? If it is not, please explain why.

Using an example operator with a snippet above

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release
    notes for FiftyOne users.

add support for setting a saved view with the set_view built-in operator

What areas of FiftyOne does this PR affect?

  • App: FiftyOne application changes
  • Build: Build and test infrastructure changes
  • Core: Core fiftyone Python library changes
  • Documentation: FiftyOne documentation changes
  • Other

@imanjra imanjra requested a review from a team March 14, 2024 13:37
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 41.30435% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 16.00%. Comparing base (aecd864) to head (b3cdf12).
Report is 18 commits behind head on release/v0.23.7.

❗ Current head b3cdf12 differs from pull request most recent head d1864ed. Consider uploading reports for the commit d1864ed to get more accurate results

Files Patch % Lines
.../src/components/Sidebar/Entries/PathValueEntry.tsx 0.00% 18 Missing ⚠️
app/packages/operators/src/operators.ts 0.00% 14 Missing ⚠️
app/packages/operators/src/built-in-operators.ts 0.00% 12 Missing ⚠️
...pp/packages/looker/src/elements/common/controls.ts 50.00% 2 Missing ⚠️
app/packages/flashlight/src/constants.ts 0.00% 1 Missing ⚠️
app/packages/flashlight/src/index.ts 0.00% 1 Missing ⚠️
app/packages/flashlight/src/section.ts 0.00% 1 Missing ⚠️
app/packages/flashlight/src/state.ts 0.00% 1 Missing ⚠️
app/packages/flashlight/src/util.ts 0.00% 1 Missing ⚠️
app/packages/flashlight/src/zooming.ts 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@                 Coverage Diff                 @@
##           release/v0.23.7    #4159      +/-   ##
===================================================
- Coverage            16.03%   16.00%   -0.04%     
===================================================
  Files                  733      734       +1     
  Lines                82012    82229     +217     
  Branches              1118     1119       +1     
===================================================
+ Hits                 13153    13159       +6     
- Misses               68859    69070     +211     
Flag Coverage Δ
app 16.00% <41.30%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

manivoxel51
manivoxel51 previously approved these changes Mar 14, 2024
Copy link
Contributor

@manivoxel51 manivoxel51 left a comment

Choose a reason for hiding this comment

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

Nice work! 🍨

I didn't pull and test this one. curious if the URL slug also changes when setting view name? 🤔

Comment on lines +473 to +476
if (params.view) {
hooks.setView(params.view);
} else if (params.name) {
hooks.setViewName(params.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

cool! does the order matter?
example: you have set a view (skip-10) and then try to load by name. will this work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The order indeed introduces some precedence. I.e. if you execute this operator with both the view and name param, the view will take precedence. If you execute this operator multiple times, the most recent will be the final result.

@imanjra
Copy link
Contributor Author

imanjra commented Mar 14, 2024

Thanks @manivoxel51! Yes, the URL slug as well as the saved view dropdown selected state changes when this operator is invoked

Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

@imanjra can you target this at release/v0.23.7 instead so we can release it on Wednesday? 😄

@imanjra imanjra changed the base branch from develop to release/v0.23.7 March 15, 2024 12:15
@imanjra imanjra dismissed manivoxel51’s stale review March 15, 2024 12:15

The base branch was changed.

Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

LGTM

@brimoor brimoor merged commit 1a34df6 into release/v0.23.7 Mar 15, 2024
@brimoor brimoor deleted the feat/set-view-by-name branch March 15, 2024 21:08
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.

None yet

3 participants