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

Implement dynamic scrollbar for SpreadSheet #130

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Implement dynamic scrollbar for SpreadSheet #130

merged 2 commits into from
Sep 26, 2023

Conversation

hanjinliu
Copy link
Owner

@hanjinliu hanjinliu commented Sep 25, 2023

Now the max value of the scrollbars in SpreadSheet are dynamic.

Summary by CodeRabbit

  • New Feature: Introduced dynamic adjustment of table size based on the current index, enhancing user navigation experience.
  • Refactor: Centralized logic for determining table shape into a new tableShape() method, improving code maintainability.
  • New Feature: Added vertical and horizontal scrolling handlers to improve user interaction with large datasets.
  • Refactor: Encapsulated row and column count setting in dedicated methods, enhancing code readability and maintainability.

@coderabbitai
Copy link

coderabbitai bot commented Sep 25, 2023

Walkthrough

This pull request introduces significant enhancements to the SpreadSheetModel and SpreadSheetTableView classes, improving table shape determination, scrolling handling, and dynamic adjustment of row and column counts. It also adds type hinting for better code readability and maintainability.

Changes

File Path Summary
.../_table_base.py Modified moveToItem method to use tableShape() for determining table shape.
.../_spreadsheet.py Introduced _nrows, _ncols, and _out_of_bound_color_cache variables with corresponding setter methods. Enhanced rowCount(), columnCount(), and moveToItem() methods. Added scroll event handlers _on_v_scroll() and _on_h_scroll().

🐇💻

"In the land of code where the shadows lie,
A rabbit hopped and tweaked on the fly.
Rows and columns dynamically set,
Scrolling handled, no need to fret.
With a hop, skip, and a jump so spry,
Celebrate these changes reaching for the sky!" 🎉🚀


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between 15cba80 and f936aff.
Files selected for processing (2)
  • tabulous/_qt/_table/_base/_table_base.py (1 hunks)
  • tabulous/_qt/_table/_spreadsheet.py (5 hunks)
Additional comments (Suppressed): 6
tabulous/_qt/_table/_base/_table_base.py (1)
  • 815-818: The tableShape() method is now being used to determine the shape of the table instead of directly calling rowCount() and columnCount() methods on the model. Ensure that the new tableShape() method returns a tuple of row count and column count in the same order as before to maintain consistency.
tabulous/_qt/_table/_spreadsheet.py (5)
  • 39-43: The _out_of_bound_color_cache variable now has a type hint, which improves code readability and maintainability. The _nrows and _ncols variables are initialized with default values of 100 and 30 respectively.

  • 68-84: The rowCount() and columnCount() methods have been modified to return the values of _nrows and _ncols respectively, instead of returning the maximum row and column counts from the table configuration. This allows for dynamic management of the number of rows and columns in the spreadsheet.

Two new methods, _set_row_count() and _set_column_count(), have been introduced to update the values of _nrows and _ncols. These methods also ensure that the number of rows and columns does not exceed the maximum allowed by the table configuration.

However, there is a potential issue with these methods. They call self.setShape() before updating _nrows and _ncols, but it's unclear whether setShape() depends on the current values of _nrows and _ncols. If it does, then the order of operations should be reversed.

    def _set_row_count(self, nrows: int):
        _nrows = min(nrows, self._table_config.max_row_count)
-        78:         self.setShape(_nrows, self._ncols)
-        79:         self._nrows = _nrows
+        78:         self._nrows = _nrows
+        79:         self.setShape(self._nrows, self._ncols)

    def _set_column_count(self, ncols: int):
        _ncols = min(ncols, self._table_config.max_column_count)
-        83:         self.setShape(self._nrows, _ncols)
-        84:         self._ncols = _ncols
+        83:         self._ncols = _ncols
+        84:         self.setShape(self._nrows, self._ncols)
  • 209-213: The valueChanged signals of the vertical and horizontal scroll bars are now connected to the _on_v_scroll() and _on_h_scroll() methods respectively. This allows for handling of scrolling events.

  • 313-333: The moveToItem() method has been modified to dynamically adjust the number of rows and columns in the spreadsheet based on the target row and column. If the target row or column is less than zero, the maximum allowed number of rows or columns is set. If the target row or column exceeds the current number of rows or columns, the number of rows or columns is increased by 20.

This change allows for dynamic resizing of the spreadsheet as the user navigates to different cells, but it may have performance implications if the number of rows or columns becomes very large. It would be good to test this feature with large datasets to ensure that it performs well.

  • 418-438: The _on_v_scroll() and _on_h_scroll() methods handle vertical and horizontal scrolling events respectively. They adjust the number of rows and columns in the spreadsheet based on the position of the scroll bar. If the difference between the total number of rows or columns and the index of the last visible row or column is less than 20 or greater than 40, the number of rows or columns is increased by 20.

These methods allow for dynamic resizing of the spreadsheet as the user scrolls, but similar to the changes in moveToItem(), they may have performance implications if the number of rows or columns becomes very large. It would be good to test these methods with large datasets to ensure that they perform well.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between f936aff and 0eb8e3f.
Files selected for processing (1)
  • tabulous/_qt/_table/_spreadsheet.py (6 hunks)
Additional comments (Suppressed): 7
tabulous/_qt/_table/_spreadsheet.py (7)
  • 31-31: The _FETCH_SIZE constant is introduced to control the number of rows and columns fetched at a time. Ensure that this value is optimal for performance, as too small a value may result in frequent fetches and too large a value may consume unnecessary memory.

  • 43-44: Two new instance variables _nrows and _ncols are introduced to store the number of rows and columns in the table. These are initially set to multiples of _FETCH_SIZE. Make sure these values are updated correctly throughout the codebase.

  • 72-75: The rowCount() and columnCount() methods have been updated to return _nrows and _ncols respectively. This change implies that the number of rows and columns is now dynamic and can be changed during runtime. Verify that this change does not break any existing functionality that relies on these methods.

  • 77-85: New setter methods _set_row_count() and _set_column_count() are introduced to update _nrows and _ncols respectively. These methods also call setShape() to update the shape of the table. Ensure that these methods are called appropriately when the number of rows or columns changes.

  • 213-214: The vertical and horizontal scrollbars' valueChanged signals are connected to the _on_v_scroll() and _on_h_scroll() methods respectively. This suggests that scrolling will now trigger these methods to handle the event. Ensure that these connections are established correctly and that the methods handle the events as expected.

  • 314-344: The moveToItem() method has been added to move the current index. It also adjusts the row and column counts if necessary. Ensure that this method is called correctly and that it behaves as expected.

  • 429-451: The _on_v_scroll() and _on_h_scroll() methods are introduced to handle vertical and horizontal scrolling events respectively. They adjust the row and column counts based on the scrollbar's position. Ensure that these methods work as expected and do not cause any unexpected behavior when scrolling.

@hanjinliu hanjinliu merged commit 399f0c2 into main Sep 26, 2023
@hanjinliu hanjinliu deleted the dynamic-scroll branch September 26, 2023 05:18
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.

1 participant