Area selection by ctrl key with mouse for SAM #897
Conversation
| useEffect(() => { | ||
| if (!markersData.error && !markersData.isFetching) { | ||
| // convert marker data into markers | ||
| const markers = markersData.data?.markersData?.map((markerProps) => ( |
There was a problem hiding this comment.
Had a quick look (not tested yet!) and markersData seems missing from the dependencies and there's no need to convert markersData into markers. You can do the bounds check below with markerData instead of markers (so it's data.position.lat instead of marker.props.position.lat.
There was a problem hiding this comment.
@bobular Thank you for your feedbacks 👍 As for markersData-related one, yes it can be simplified so I made a new commit to address them. For the dependency, I checked that markersData-related one causes infinite loop so I didn't add it to the dependency array.
|
Hi @moontrip - it looks like Mac command key is going to be a problem for us |
There was a problem hiding this comment.
This is a great start.
A few things are of concern though.
- The installation issues of
leaflet-area-select- I haven't investigated but combined with - The lack of Apple
Cmdkey support I think that - We should consider better maintained alternatives such as https://github.com/Leaflet/Leaflet.draw or https://www.npmjs.com/package/@geoman-io/leaflet-geoman-free
- Leaving out essential hook dependencies to avoid loops
- I think we can avoid tracking state of
boxCoordat MapAnalysis.tsx and repetition of code within the map types and instead deploy a custom hook in each MapType that returns a handler for the selected bounds, and do something like this in DonutMarkerMapType.MapLayerComponent:
const handleSelectedArea = useAreaSelectedMarkers({
selectedMarkers,
setSelectedMarkers,
markerDataResponse,
});
return (
<>
{markerDataResponse.isFetching && <Spinner />}
{markers && (
<AreaSelect onSelectedArea={handleSelectedArea} />
<SemanticMarkers
markers={markers}
// no changes to any props here
/>
)}
</>
);
We probably want to check markerDataResponse.pending inside the hook and have the handler do nothing if the markers are pending.
|
FYI: Stories are fixed with #901 (merged) |
|
I think for now, all the story needs to do is call the callback which prints the bounds to the console. No need to actually do any marker-selection logic. |
|
@bobular Thank you for your comments. I didn't have much time to work for VEuPathDB this week as I needed to do other project. I will check your feedbacks carefully, but I could manage to enable Mac's Command key for current area selection. Although I am using Windows/Ubuntu, I could test it works as Window key is essentially the same with Mac's Command key in the key mapping: |
|
Hi @moontrip - I wasn't able to use the windows key on Ubuntu Linux unfortunately. Not without remapping keys in X11 anyway, which I'm not going to do at this point. Assuming your fix is good, then let's continue with the "hacked (not) import" - I had a closer look and realise it's not interfering with any other Leaflet internals. No need to look at the other libraries (though we might need them one day for polygon/lasso support - though I think I remember you already started looking at that some time ago?) If you could take the logic out of MapAnalysis and make a hook/handler as suggested, that would be great. I also wonder if ctrl-rectangle select should add to the marker selection? |
|
Hi @bobular Thank you for your comments 👍 You can find my reply (if necessary) in the following starting from DK: with bold style.
DK: Actually, I am also using Ubuntu in the VM. Unfortunately, Ubuntu assigned the windows key as a Super key for their purpose, so certainly it cannot be tested under Ubuntu. What I did for test was that I used
DK: Sounds great. Yes IIRC I did look at react-leaflet-draw a couple of years ago. Not quite sure how much it was upgraded since then.
DK: I will test it next Monday when I will work for VEuPathDB
DK: I'm sorry that I cannot fully understand your question. Can you please elaborate it more? |
At the moment, the rectangle selection is replacing the current selected markers. I think it should add the markers in the rectangle to whatever previous selected markers existed (if any). |
|
@bobular Ah I got your point 👍 I am not quite sure if it is doable due to a couple of reasons, e.g., mouse click event, etc., but I will check
|
|
@bobular couple of updates are made: a) Area selection can add markers with pre-existing selected markers |
|
Hi @moontrip Looks like these changes got merged when I merged the child PR. Nice. |
Yes because you branched out from this PR. Thank you for enhancing this work 👍 👍 |
This will address #863. A couple of notes are provided in the following and a video capture is attached in the end:
The main area-select is taken from 'leaflet-area-select' library. I couldn't figure out why the lib did not work with our repo, even if it just worked fine with simple react env (after running create-react-app). Thus, I modified the lib and directly loaded it instead of installing (i.e., yarn add).
The 'leaflet-area-select' is quite old library and does not have a typescript definition. Since it is not trivial task to type a leaflet-plugin, I just used
// @ts-nocheckto ignore typescript (AreaSelect.tsx)After thinking over and trying several cases, the best place to use marker selection logic I found is each marker's MapType (e.g., DonutMarkerMapType, etc.) and a useEffect is utilized to update selectedMarkers. However, since MapType involves conditions (checking errors for marker response, etc.), thus some duplicate codes were utilized inside the useEffect, (markers & selectedMarkers).
Overall, it seems to work fine: a video shot is attached in the next
map.area.selection.by.ctrl.key.with.mouse.mp4