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 a cache for pinned statistics #4070
Conversation
Patch initially written by The-Compiler
Thanks for the pull request! Note that reviews currently can take a bit longer, as @The-Compiler is busy with exams until September. (Powered by GitMate.io) |
…nto jay/pinned-statistics-cache
03f48df
to
a00d4ad
Compare
…nto jay/pinned-statistics-cache
qutebrowser/mainwindow/tabwidget.py
Outdated
@@ -110,11 +110,17 @@ def set_tab_pinned(self, tab: QWidget, | |||
""" | |||
bar = self.tabBar() | |||
idx = self.indexOf(tab) | |||
old_pinned = tab.data.pinned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you just do:
if tab.data.pinned == pinned:
return
here? No need to call set_tab_data
and set tab.data.pinned
if it's already the same value either, right?
qutebrowser/mainwindow/tabwidget.py
Outdated
self.update_tab_title(idx) | ||
# pylint: disable=protected-access | ||
self.tabBar()._pinned_statistics.cache_clear() | ||
# pylint: enable=protected-access |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a substantiated reason to override pylint here - I can see two solutions:
- Make
_pinned_statistics
public (even if it's not called as a function from the outside). This seems fair, because we can't look atTabBar
in isolation anymore when changing something aboutpinned_statistics
. - Probably preferrable: Add a
clear_pinned_statistics
signal toTabWidget
, and hook that up toself._pinned_statistics.cache_clear
inTabBar
.
qutebrowser/mainwindow/tabwidget.py
Outdated
@@ -394,6 +416,7 @@ def _on_config_changed(self, option: str): | |||
"tabs.indicator.width", | |||
"tabs.min_width", | |||
"tabs.pinned.shrink"]: | |||
self._pinned_statistics.cache_clear() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Use the same order (before/after _minimum_tab_size_hint_helper.cache_clear
) like below - doesn't matter which one 😉
4b7ca5f
to
d8b1753
Compare
# function calls tabSizeHint to resize the tabs after removing the tab. | ||
# If we assume super().removeTab will NOT call tabSizeHint prematurely, | ||
# this should work. | ||
self.tabBar().clear_pinned_statistics.emit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like how sensitive this is to the exact order things are done in, so I would like to find a better way to invalidate the cache if possible. One way that I thought of is that we can keep a count of tabs independent from the QTabBar, and when they differ invalidate the cache (in tabSizeHint). This has the advantage of being able to more 'correctly' handle additions/removals if the tabs are resized (which they should be, by qt), but if a new tab is added before the redraw there can be problems.
In order to prevent a crash when this happens I clamped the division by zero below, not a great solution, but I'd much rather have mis-sized tabs than a crash if we mess up this logic.
I thought about this a bit and we might not actually need any of this (unless I forgot a feature with tab sizing). I'll submit a PR and hopefully we can discard this one...
Combined with config caching, I think this should raise the amount of usable tabs to about ~100.
This is a little more complicated than @The-Compiler s initial patch, but I think it should be a little better this way (as it only clears the pinned statistic cache when needed). A cleaner way of doing this is keeping track of the pinned width as the tabs change (by adding and removing sizes) rather than iterating over them every time. However, I think this is more than enough to fix it from being a bottleneck with lots of tabs.
This helps out with #4069, but only for extreme amounts of tabs (~100), wheras config caching would help everything. I think this patch is the one that helps the most with deleting the first tab (which is the described issue in #4069) as well.
This change is