LF-3828 fix selecting vertical line on map

[navDhammu]

Description

Fixes the issue where user can't select a fence on the map when its a vertical line or clicking on the vertical portion of a fence.

This issue may be caused by a bug in google maps api where the method isLocationOnEdge (inside useSelectionHandler.js line 127) is not able to detect clicks for vertical lines - see related stackoverflow question: https://stackoverflow.com/questions/19113399/google-maps-api-v3-islocationonedge-doesnt-work

Changes:

• Bypassed the google maps api by removing handleSelection() inside polyline click event listener and replaced it with history.push (the handleSelection method calls history.push internally).

Jira link: https://lite-farm.atlassian.net/browse/LF-3828

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Passes test case
• UI components visually reviewed on desktop view
• UI components visually reviewed on mobile view
• Other (please explain)

Checklist:

• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• The precommit and linting ran successfully
• I have added or updated language tags for text that's part of the UI
• I have added "MISSING" for all new language tags to languages I don't speak
• I have added the GNU General Public License to all new files

[antsgar]

@navDhammu thank you for working on this one! Do you think replacing isLocationOnEdge with containsLocations within the hook as suggested in the SO thread would work? Although the hook could definitely use some refactoring, it's nice to have all the logic and the handling of all of the different cases encapsulated in there instead of in the component, and I think replacing the call to the hook's method with a history.push in the component directly would go against that premise.

[navDhammu]

@antsgar Thank you for reviewing this. containsLocation only works for polygons, so in this case if a user just draws a straight line then containsLocations won't work.

In the handleSelection method:

else if (
     overlappedLocations.area.length === 0 &&
     overlappedLocations.line.length === 1 &&
     overlappedLocations.point.length === 0
   ) {
     history.push(
       `/${overlappedLocations.line[0].type}/${overlappedLocations.line[0].id}/details`,
     );

This is the piece of code that does history.push and takes you to the selected view when a line click is detected, however with isLocationOnEdge it isn't detected and so its never executed. I'm not sure how to make this work other than calling the history.push directly, if we want to encapsulate it in handleSelection maybe pass some additional info to handleSelection and then call it inside?

[Duncan-Brain]

Okay this is a tough one to investigate! Just subbing in for @antsgar while she is out. I definitely didn't realize for a while that since handleSelection is stored as a callback I had to logout to see any local changes I made to the function take place.

I agree that some of the checks seem somewhat not important -- like if it is clicked than surely it is visible haha. I think also because it is maybe called for both areaLine and line that this fix would still run the other useful parts of the function -- but I have to admit all this maps code is confusing for me. And the handleSelectionFile has a big Do not modify, copy or reuse disclaimer haha

That being said I think the overlappedLocationsCopy is probably something we want to function correctly even though I dont fully understand it. So what I am seeing is that isLocationEdge() will work with reduced precision to 10e-4 -- and I think this is more ideal since it doesn't break the existing flow as much. The down side it doesn't fix the zooming issue where you may have to zoom in on a really big fence or double click the fence multiple times in order for it to work.

I agree if its clicked its probably visible, and therefore the usefulness of the google check is lost a bit on me.

[navDhammu]

@Duncan-Brain Thanks for doing the review.

Good find on the 10e-4 tolerance value! When I first investigated this I tried some values for the precision like 10e-5 and it was working 50-50. And 10e-2, 10e-3 was giving me a weird dialog showing the names of all my fences (see below).

So I was doubtful on how reliable changing the precision would be, but out of all values 10e-4 doesn't seem to be causing any issues.

And yeah I don't understand what overlappedLocations is doing or why the isLocationOnEdge check is even needed if you can simply attach a click listener to the line. That's why I just put history.push directly in the handler and tested it by creating, editing, and deleting all types of lines and there wasn't a problem. But there might be some edge cases I'm not aware of so if changing the precision is working then might be good to go with that.

Although I'm not sure what you mean by the zooming issues?

[Duncan-Brain]

So that weird dialog is what I think the overlapped function is supposed to be doing -- its most evident with multiple close together points like sensors. By summing up all the overlapping point that are too close together to display you can select from the list of points that in that dialog.

Makes me wonder if we have it only working for sensors but I think ideally that dialog should allow you to click on the fence name and would take you too the details page too (another I think loophole fix for this issue if that works)

I think I found the 10-4 as some magic number in a thread somewhere. But I think that dialog behaviour is acceptable too.

Zooming behaviour for horizontal (not vertical -- but also vertical with the fix) - I am double clicking to zoom in the video:

Screen.Recording.2024-02-13.at.1.46.08.PM.mov

The nice part about your fix was it wasn't affected by zoom level -- another mystery maps behaviour for me.

[antsgar]

@navDhammu sorry if this doesn't make sense, trying to understand the map code -- what would happen if we just removed the isLocationOnEdge check on line 127 and only checked if the line is visible?

[navDhammu]

@antsgar From what I can understand - if we remove isLocationOnEdge then I think then all the visible lines on the map would show up in that dialog view. The isLocationOnEdge is trying to select only the lines that are within the precision of a point (the third parameter).

[navDhammu]

@Duncan-Brain Ok that makes sense, the dailog with the fences allows me to click on the name and it takes me to the correct view. But its not that obvious that's what its supposed to do. Like if I'm zoomed in and clearly trying to select a specific fence, I don't need to see another dialog before I can view the fence.

I updated it to use the 10e-4 value and it's working for me and I'm not getting any zoom issues, I can single click when zoomed in or out and it's taking me to the correct page.