DataTable new rows can have auto height. #3213
Conversation
Good call. See 65aaf39. |
When adding a row with automatic height, I need to render the cells to compute their height. Instead of wasting that rendering, I want to do it well and then cache it, which means I need to reuse some of the logic of the other rendering methods. By extracting some logic, I'll be able to reuse it.
|
Have you tested this with sorting and checked what happens as rows move around - does it redraw correctly with the correct heights etc? |
We extract this logic into a method for two reasons. For one, having this as a method with an lru cache enables caching these auxiliary styles, which don't depend directly on the location of the cell, but instead depend on the values of 9 Boolean flags (making for a total of 512 possible combinations, versus the infinite number of different positions/states a cell can be in. Secondly, having this as a method allows me to compute these styles more easily from within _update_dimensions when trying to salvage the renderings of the cells of a new row that may have been pre-rendered with the wrong height. (See the following commits for more context.)
| is_header_cell: Is this a cell from a header? | ||
| is_row_label_cell: Is this the label of any given row? | ||
| is_fixed_style_cell: Should this cell be styled like a fixed cell? | ||
| hover: Does this cell have the hover pseudo class? | ||
| cursor: Is this cell covered by the cursor? | ||
| show_cursor: Do we want to show the cursor in the data table? | ||
| show_hover_cursor: Do we want to show the mouse hover when using the keyboard | ||
| to move the cursor? | ||
| has_css_foreground_priority: `self.cursor_foreground_priority == "css"`? | ||
| has_css_background_priority: `self.cursor_background_priority == "css"`? |
There was a problem hiding this comment.
Yesterday we talked about rhetorical questions as docstrings, but given that these are just a bunch of Boolean values, this actually looks reasonable to me.
There was a problem hiding this comment.
Sorry, I'm going to veto that. Docstrings should be in pose.
Suggest "True if the cursor foreground priority is 'css'" or similar.
|
@darrenburns @willmcgugan after much sweat and tears, I'm ready for your reviews. 🙃 |
| is_header_cell: Is this a cell from a header? | ||
| is_row_label_cell: Is this the label of any given row? | ||
| is_fixed_style_cell: Should this cell be styled like a fixed cell? | ||
| hover: Does this cell have the hover pseudo class? | ||
| cursor: Is this cell covered by the cursor? | ||
| show_cursor: Do we want to show the cursor in the data table? | ||
| show_hover_cursor: Do we want to show the mouse hover when using the keyboard | ||
| to move the cursor? | ||
| has_css_foreground_priority: `self.cursor_foreground_priority == "css"`? | ||
| has_css_background_priority: `self.cursor_background_priority == "css"`? |
There was a problem hiding this comment.
Sorry, I'm going to veto that. Docstrings should be in pose.
Suggest "True if the cursor foreground priority is 'css'" or similar.
src/textual/widgets/_data_table.py
Outdated
| ) | ||
|
|
||
| self._cell_render_cache[cell_cache_key] = lines | ||
|
|
||
| return self._cell_render_cache[cell_cache_key] | ||
|
|
||
| @functools.lru_cache( | ||
| maxsize=512 |
There was a problem hiding this comment.
This is broken I'm afraid. @lru_cache on a method will give you a single cache for the process. You are using get_component which may return different results depending on the instance. Consequently if you have 2 or more DataTables, the styles are going to get mixed.
If you need caching (suspect it is a win), have a look at cache.py which has some home grown cache objects you can use per-instance.
Also consider reducing the maxsize. I'm guessing some combos are used more than others.
There was a problem hiding this comment.
While we concluded that this is not broken in the sense that you described because lru_cache also caches the instance, there are issues waiting to happen with hot-reloading, dark-mode toggling, etc.
Those are mostly addressed in #3313 and 2524af4.
I'd like for #3313 to be merged into main so I can merge it here to make sure this was all I needed and to add tests for it.
The first five parameters (is_header_cell, is_row_label_cell, is_fixed_style_cell, hover, and cursor) are the ones that change more frequently, so it is reasonable to fix the size of the cache at 32. Related comment: #3213 (comment)
Related comment: #3213 (comment).
Fixes #3122.
Adds
auto_heighttoRowto try and keepRowandColumnas similar as possible, as per a conversation with @darrenburns.Temporarily sets the row height to 0 to signal the row height hasn't been computed yet (we can't set to
Nonebecause we really need anintthere for computations that run in the interim).