Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

fix: layer select state not update properly #268

Merged
merged 3 commits into from Jul 9, 2022

Conversation

airslice
Copy link
Contributor

@airslice airslice commented Jul 8, 2022

Overview

This PR fix three issues:

  1. When create a new layer by drop the layer in Earth Editor, it doesn't got auto selected (No indicator shown on map).

    • This issue also lead to when create a new infobox for this layer the Infobox popup doesn't show automaticlly.
    • The bug comes from the state update: selectedLayer happens before layers data update so it got undefined.
    • Why it works before is bc the setLayers with new created object will lead to anothor selectedLayer update which can get the latest layers data.
    • In order to implement the update after layers data changed I replace the useUpdate with an internal implemtation (which refers to the source code of useUpdate but expose the state) and use the state as the dependence of selectedLayer.
  2. When unselect a layer the plugin api select doesn't emit (the plugin will not be noticed as unselect).

    • For select event undefined is also meaningful and also should emit event to notice plugin there's no selected object.
  3. Plugin API event select cameramove emitted too much (not as expected i think).

    • The value of args (used as dependency of useEffect inside useEmit) is something like [camera] or [selectedLayer.id] which would be differect in each update and make the side effect trigger every time. For example if you add cameramove event from API then when you change any state eg. change scene background color you'll find the event also emitted. Also if you add a select event and select a layer then when you move camera the select event keeps emitting.

What I've done

  • Replace useUpdate with a internal implement and use its state as selectedLayer's dependency.
  • Replace undefined to [undefined] in order to emit plugin api select event when unselect.
  • Add useMemo to cache the array like [camera] and [selectedLayer.id] to avoid unnecessary emit.

What I haven't done

How I tested

  • Create new layer from earth editor and create new infobox for it.
  • Select & unselect layer and watch the event got with plugin.
  • Add select mousemove event from plugin and watch the event emit.

Screenshot

Which point I want you to review particularly

Memo

@airslice airslice self-assigned this Jul 8, 2022
@netlify
Copy link

netlify bot commented Jul 8, 2022

Deploy Preview for reearth-web ready!

Name Link
🔨 Latest commit ae9c5ed
🔍 Latest deploy log https://app.netlify.com/sites/reearth-web/deploys/62c81b74757bc30008de0d71
😎 Deploy Preview https://deploy-preview-268--reearth-web.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@airslice airslice added the bug Something isn't working label Jul 8, 2022
@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #268 (ae9c5ed) into main (eb41daf) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #268   +/-   ##
=======================================
  Coverage   52.19%   52.19%           
=======================================
  Files          56       56           
  Lines        1205     1205           
  Branches      193      193           
=======================================
  Hits          629      629           
  Misses        503      503           
  Partials       73       73           

@airslice airslice marked this pull request as ready for review July 8, 2022 12:07
Copy link
Member

@KaWaite KaWaite left a comment

Choose a reason for hiding this comment

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

Ok! Just as an FYI, the reason you need to implement that forceUpdate might be related to react 18's changes and a problem with Jotai where renders or re-renders that rely on Jotai state don't notice a change and don't re-render.
I'll let you know as I know more, but just remember about this code as we might not need it if I can fix the bigger overlying issue.

@lavalse
Copy link
Member

lavalse commented Jul 9, 2022

And I tested it. This also fix the infobox issue

@airslice airslice merged commit 5f7c69e into main Jul 9, 2022
@airslice airslice deleted the fix/layer-not-auto-selected-after-create branch July 9, 2022 04:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
4 participants