Use set_view() in builtin view operations #4960

brimoor · 2024-10-22T02:56:19Z

Change log

Updates the builtin save_view, edit_saved_view_info, and delete_saved_view operators as follows:

save_view: when saving the current view, use ctx.ops.set_view() to update the App to reflect the fact that the current view is now saved
edit_saved_view_info: when renaming the current view, use ctx.ops.set_view() to update the App to reflect the current view's new name
delete_saved_view: when deleting the current view, reset the App to the full dataset

This is consistent with how save_workspace, edit_workspace_info, and delete_workspace work as of #4902.

TODO

Fix this: Use set_view() in builtin view operations #4960 (comment)

coderabbitai · 2024-10-22T02:58:03Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes in this pull request enhance the functionality of the fiftyone/operators/builtin.py file by modifying several operators related to views and workspaces. Key updates include the addition of a view parameter in the SaveView operator, adjustments to the EditSavedViewInfo and DeleteSavedView operators for better context management, and improvements to the LoadWorkspace and SaveWorkspace operators. Additionally, two new utility functions are introduced, and error handling is refined to improve clarity.

Changes

File	Change Summary
fiftyone/operators/builtin.py	- Updated `SaveView`, `EditSavedViewInfo`, `DeleteSavedView`, `LoadWorkspace`, and `SaveWorkspace` operators to handle new parameters for views and workspaces. - Added utility functions `_parse_view` and `_parse_spaces`. - Refined error handling for various operators.

Possibly related PRs

fix save workspace built-in operator #4868: This PR modifies the fiftyone/operators/builtin.py file, specifically enhancing the SaveWorkspace operator, which is directly related to the changes made in the main PR that also involves modifications to operators in the same file.

Suggested reviewers

ritch
benjaminpkane

🐰 In the garden where views grow,
Operators dance, putting on a show.
With spaces saved and names anew,
Flexibility blooms, as we hop and chew!
Error messages clear, like a sunny day,
Let's celebrate changes, hip-hip-hooray! 🌼

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?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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 using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

brimoor · 2024-10-22T03:00:31Z

@imanjra note in the attached video that ctx.ops.set_view(name=name) doesn't immediately work when name is a newly created view. You must refresh the App in order for it to recognize the new view.

Interestingly, the exactly analogous steps work for ctx.ops.set_spaces(name=name) introduced in #4902 does work.

Is it an easy fix to tweak the App so that new saved views can be immediately referenced?

save_view_issues.mov

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c21cc6d and 36a1959.

📒 Files selected for processing (1)

fiftyone/operators/builtin.py (5 hunks)

🧰 Additional context used

🪛 Ruff

fiftyone/operators/builtin.py

1696-1699: Use ternary operator view = ctx.view if curr_view else _parse_view(ctx, view) instead of if-else-block

Replace if-else-block with view = ctx.view if curr_view else _parse_view(ctx, view)

(SIM108)

🔇 Additional comments (2)

fiftyone/operators/builtin.py (2)

1737-1744: Ensure current view is updated after renaming

The code correctly updates the current view if its name has changed, ensuring consistency within the application.

1855-1860: Reset current view after deleting the active saved view

The implementation properly resets the current view to the dataset's default view when the active saved view is deleted.

fiftyone/operators/builtin.py

imanjra · 2024-10-22T12:42:46Z

@brimoor yah, I believe it may require slight refactor. For workspace, I think it does a trip to backend so it can get latest workspaces but for view it does it all in the frontend. I'll check it out. It should be doable.

imanjra

LGTM

imanjra · 2024-10-22T13:12:59Z

fiftyone/operators/builtin.py

            description=description,
            color=color,
            overwrite=True,
        )

+        if curr_view:


Out of scope of this PR as it matches workspaces, but we should allow opting out of this for programmatic execution

Hmm good point, is there a way to tell if an operator is being executed programmatically vs prompting?

imanjra · 2024-10-22T13:13:19Z

fiftyone/operators/builtin.py

        ctx.dataset.update_saved_view_info(name, info)

+        if curr_name is not None and curr_name != new_name:


same as above

imanjra · 2024-10-22T13:13:32Z

fiftyone/operators/builtin.py

        ctx.dataset.delete_saved_view(name)

+        if curr_view:


same as above

brimoor · 2024-10-22T14:45:17Z

Just to clarify, we can't merge this until/unless saved views are updated per this comment. As it currently stands, one gets the error notifications pictured in the video there.

brimoor requested a review from imanjra October 22, 2024 02:56

coderabbitai bot reviewed Oct 22, 2024

View reviewed changes

fiftyone/operators/builtin.py Outdated Show resolved Hide resolved

fiftyone/operators/builtin.py Outdated Show resolved Hide resolved

imanjra approved these changes Oct 22, 2024

View reviewed changes

brimoor changed the title ~~Use set_view() in builtin view operations~~ [HOLD] Use set_view() in builtin view operations Oct 22, 2024

brimoor marked this pull request as draft October 22, 2024 14:44

brimoor force-pushed the ctx-view-builtin-updates branch from 36a1959 to a28b518 Compare December 21, 2024 01:09

brimoor changed the title ~~[HOLD] Use set_view() in builtin view operations~~ Use set_view() in builtin view operations Dec 21, 2024

use set_view() in builtin view operations

221ea6c

brimoor force-pushed the ctx-view-builtin-updates branch from a28b518 to 221ea6c Compare December 26, 2024 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use set_view() in builtin view operations #4960

Use set_view() in builtin view operations #4960

brimoor commented Oct 22, 2024 •

edited

Loading

coderabbitai bot commented Oct 22, 2024 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

brimoor commented Oct 22, 2024

coderabbitai bot left a comment

imanjra commented Oct 22, 2024

imanjra left a comment

imanjra Oct 22, 2024

brimoor Oct 22, 2024

imanjra Oct 22, 2024

imanjra Oct 22, 2024

brimoor commented Oct 22, 2024

		ctx.dataset.update_saved_view_info(name, info)

		if curr_name is not None and curr_name != new_name:

Use set_view() in builtin view operations #4960

Are you sure you want to change the base?

Use set_view() in builtin view operations #4960

Conversation

brimoor commented Oct 22, 2024 • edited Loading

Change log

TODO

coderabbitai bot commented Oct 22, 2024 • edited Loading

Review skipped

Walkthrough

Changes

Possibly related PRs

Suggested reviewers

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

brimoor commented Oct 22, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

imanjra commented Oct 22, 2024

imanjra left a comment

Choose a reason for hiding this comment

imanjra Oct 22, 2024

Choose a reason for hiding this comment

brimoor Oct 22, 2024

Choose a reason for hiding this comment

imanjra Oct 22, 2024

Choose a reason for hiding this comment

imanjra Oct 22, 2024

Choose a reason for hiding this comment

brimoor commented Oct 22, 2024

brimoor commented Oct 22, 2024 •

edited

Loading

coderabbitai bot commented Oct 22, 2024 •

edited

Loading