Don't notify pages change for MPAv2 #8698
Conversation
kmcgrady
force-pushed
the
fix/initial-navbar
branch
2 times, most recently
from
175116a
to
4a1bc96
Compare
kmcgrady
force-pushed
the
fix/initial-navbar
branch
from
04f298f
to
e0d0cd4
Compare
| self._intent_page_script_hash: PageHash | None = None | ||
| self._intent_page_name: PageName | None = None |
There was a problem hiding this comment.
nit: Should these names be intended* rather than intent* throughout? It feels like that form of the word is likely slightly more correct here
There was a problem hiding this comment.
I think both convey the intent (pun intended....x2 😆), but I am happy to put the more correct form here.
| @@ -291,7 +292,7 @@ def set_pages(self, pages: dict[PageHash, PageInfo]) -> None: | |||
|
|
|||
| self.pages_strategy.set_pages(pages) | |||
| self._cached_pages = pages | |||
| self._on_pages_changed.send() | |||
| # We deliberately don't notify on page change as V2 does not require of this. | |||
There was a problem hiding this comment.
I would just get rid of this comment if we're getting rid of the call to self._on_pages_changed.send() in get_pages as well
| @@ -276,6 +276,7 @@ def get_pages(self) -> dict[PageHash, PageInfo]: | |||
|
|
|||
| pages = self.pages_strategy.get_pages() | |||
| self._cached_pages = pages | |||
| self._on_pages_changed.send() | |||
There was a problem hiding this comment.
I don't think this is the right place to call this given that moving the call here doesn't remove it from being called in V2, it just moves it to happen in a place where the weird behavior due to it is less noticeable.
I'm wondering if we really need this call at all. There's also a call to self._on_pages_changed.send() in invalidate_pages_cache, so I don't think we need another one here.
Also, I think the whole notion of page cache invalidation + on changed callbacks + etc. only really makes sense in V1, so the natural place to move all of the pages changed callback related things would be within the V1 strategy class
There was a problem hiding this comment.
I agree. I was thinking of following up with this change, but I can address this here.
kmcgrady
force-pushed
the
fix/initial-navbar
branch
from
e0d0cd4
to
4c33810
Compare
| def invalidate_pages_cache(self) -> None: | ||
| source_util.invalidate_pages_cache() |
There was a problem hiding this comment.
Is this method still useful? I don't think this will ever be called externally since we expect _handle_page_changed to call source_util.invalidate_pages_cache()
| def invalidate_pages_cache(self) -> None: | ||
| pass |
There was a problem hiding this comment.
Similar comment to above about being able to get rid of this
|
|
||
| class PagesManager: | ||
| DefaultStrategy: Type[PagesStrategyV1 | PagesStrategyV2] = PagesStrategyV1 | ||
|
|
||
| def __init__(self, main_script_path, script_cache=None, **kwargs): | ||
| self._cached_pages: dict[PageHash, PageInfo] | None = None | ||
| self._pages_cache_lock = threading.RLock() |
There was a problem hiding this comment.
I think(?) it ends up being the case that we don't need a lock here anymore, but it's not entirely obvious because it's still the case that in the V2 world we have multiple threads that may end up accessing pages simultaneously: both the user script thread and event loop thread may simultaneously access self.pages_strategy._pages.
I think this ends up being fine in the V2 world since only the user script thread can write to self.pages_strategy._pages and the event loop thread only reads, but we'll want to add some comments to clarify that the absence of locks in the V2 world is intentional + probably some additional comments in the PagesManager class clarifying that thread safety concerns around a strategy's cached pages are delegated to the strategy implementation
There was a problem hiding this comment.
Yea, I think pages need to be set before they are useful. We definitely reduce the risk because it's only tied to a particular session (and not shared across all sessions). Will add a comment.
Describe your changes
Page Changes in MPAv2 are done via the
st.navigationcall. Informing of Pages Change is not helpful for the UI display. We also came to the conclusion that the concept of cache pages did not matter for PagesManager as well. So I moved everything back to source_util to produce a cleaner system.In testing this, I noticed some inconsistencies due to the fact that the default strategy of the PagesManager is not resetting. So I did the following:
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.