Add support for tooltip overwrite #5351
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fiftyone/operators/types.py (1)
1909-1919: LGTM! Consider adding docstring for the new parameter.The implementation of the
overwriteparameter is clean and logical. The default value ofTruemaintains backward compatibility while providing flexibility to prevent accidental overwrites when needed.Add parameter documentation:
def add_tooltip(self, row, column, value, overwrite=True, **kwargs): + """ + Args: + overwrite (True): whether to overwrite an existing tooltip at the same position + """tests/unittests/operators/tableview_tests.py (1)
46-48: Add more test cases for comprehensive coverage.While the basic functionality is tested, consider adding the following test cases:
- Verify tooltip attributes are properly updated when overwriting
- Verify the error message when
overwrite=False- Verify the tooltip map is updated correctly after overwrite
table.add_tooltip(1, 1, "Tooltip 1") table.add_tooltip(1, 2, "Tooltip 2") with self.assertRaises(ValueError): table.add_tooltip(1, 1, "Tooltip 3", overwrite=False) table.add_tooltip(1, 1, "Tooltip 3") + +# Test tooltip attributes are updated +table.add_tooltip(1, 1, "Tooltip 4", color="red") +self.assertEqual(table._tooltip_map[(1, 1)].value, "Tooltip 4") +self.assertEqual(table._tooltip_map[(1, 1)].color, "red") + +# Test error message +with self.assertRaisesRegex(ValueError, "Tooltip for row '1' and column '1' already exists"): + table.add_tooltip(1, 1, "Tooltip 5", overwrite=False) + +# Test tooltip map consistency +table.add_tooltip(1, 1, "Tooltip 6") +self.assertEqual(len(table.tooltips), 2) # Should still have only 2 tooltips +self.assertEqual(table._tooltip_map[(1, 1)].value, "Tooltip 6")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fiftyone/operators/types.py(1 hunks)tests/unittests/operators/tableview_tests.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build
minhtuev
force-pushed
the
feat/support-tooltip-overwrite
branch
from
962464a to
92d05f2
Compare
There was a problem hiding this comment.
Someone else with more understanding of panels can correct me, but I'm not sure if this is necessary under the current implementation as it seems like the panel instance/table doesn't persist over multiple renders so every time you get new data, the tooltip is new.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fiftyone/operators/types.py (1)
1915-1925: Enhance the error message for better clarity.Consider providing more context in the error message by including the existing tooltip's value.
- raise ValueError( - f"Tooltip for row '{row}' and column '{column}' already exists" - ) + raise ValueError( + f"Tooltip for row '{row}' and column '{column}' already exists with value '{self._tooltip_map[(row, column)].value}'. Set overwrite=True to replace it." + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fiftyone/operators/types.py(2 hunks)tests/unittests/operators/tableview_tests.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (2)
tests/unittests/operators/tableview_tests.py (1)
47-60: LGTM! Comprehensive test coverage for tooltip overwrite functionality.The test case thoroughly validates:
- Overwriting existing tooltip
- Tooltip value update
- Additional arguments storage
- JSON representation correctness
fiftyone/operators/types.py (1)
1851-1867: LGTM! Well-structured changes to the Tooltip class.The changes properly integrate the required
valueparameter throughout the class implementation.
What changes are proposed in this pull request?
Allowed users to easily overwrite tooltips in TableView and updated tests
How is this patch tested? If it is not, please explain why.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
valueparameter for tooltips, enhancing tooltip functionality.add_tooltipmethod in TableView to include anoverwriteparameter for managing existing tooltips.Bug Fixes
overwriteparameter.