-
Notifications
You must be signed in to change notification settings - Fork 109
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
Fix/cache render details #794
Conversation
🤖 Pull request artifacts
|
@@ -64,14 +64,19 @@ var handleResponse = function (deferred, guid, data, success) { | |||
} | |||
|
|||
if (success) { | |||
// Parse portal details | |||
var dict = window.decodeArray.portal(data.result, 'detailed'); | |||
cache.store(guid, dict); |
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.
The data is cached twice, is this done on purpose?
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.
It is. Both for consistency wrt hooks.
First cache is make sure the data is there when the portal entity is created (for portalAdded
hook)
Second cache updates it (from map data) for portalDetailLoaded
hook functions. (including renderPortalDetails)
var portalData; | ||
|
||
// If the portal was cleared then exit. | ||
if (window.selectedPortal === null) return; | ||
|
||
portalData = window.portalDetail.get(window.selectedPortal); | ||
portalData = data.portalData; |
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.
Is this change about code improvement (decoupling plugins from cache) rather than fixing a bug?
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.
both, this code triggers the issue but it could have used the hook data instead
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 think I've found a solution to make it backwards compatible. I'll create a separate PR for this
Clarify
renderPortalDetails
: its selects, refresh and render portal details.Split it in a select/refresh part and render part.
Revert part of fa3dddd
Invalidate cache entry if map data is inconsistent with portal details. (from map_data_render)
Hook portal details rendering on
portalAdded
andportalDetailLoaded
eventUse hook data in unique/machina-tools plugins instead of using
portalDetail.get
.I'm not satisfied with the data flow I've made for the permanent portal markers, as data is store in two places (portal marker and cache) and
createPortalEntity
is used to parse data for map data and portal details.This fix a bit this issue by parsing portal details twice but it feels wrong to me.