feat: query with geo bounding box #148

ruanyl · 2022-12-29T08:17:27Z

What's changed:

When the map application is opened(initial load), it will query data with the current map bounds
When user zoom in/out, move the map, resize the window, etc. New queries will be fired with the updated bounding box and then update the map accordingly.

Bug fixes:

fixed wrong maplibre instance passed to addTooltip() function which results in a runtime error
fixed empty onHover popup rendered on layer which has tooltip disabled

Signed-off-by: Yulong Ruan [email protected]

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

VijayanB · 2022-12-29T23:53:00Z

@ruanyl Looking really good. Thanks for making the changes. Two comments.

Can we add check box (instead of switch) inside Filters section like below to decide whether to apply this filter or not?
We also enable this checkbox only if selected field type is geo_point. WDYT?

I still see tooltip hover when it is disabled.

ruanyl · 2022-12-30T01:14:00Z

Hi @VijayanB, thanks for the comments!

For adding the a checkbox to let user control how the map data should be loaded seems to me that we're exposing to many implementation details to the end user.

From the end user's point of view, when they are viewing the map, they may not interested in how the data is loaded and adding this checkbox may confuse the user. But of course, this is just my two cents.

For the empty popup, nice catch! Now I'm able to reproduce it when I disabled the tooltip but not refresh the page. And I investigated the issue, I think the reason is mouseenter event listener added by addTooltip function is not unsubscribed(map.off()) when layer config changed(tooltip config). The empty popup is actually from the old mouseenter event listener.

To fix the issue, we would need to unsubscribe the previous event listener whenever adding the new event listener when layer config changed, we do this for click popup in map_container.tsx.

It seems it's not a trivial task to refactor this part, I'd suggest to fix it in a separate task.

VijayanB · 2022-12-30T01:27:34Z

Hi @VijayanB, thanks for the comments!

For adding the a checkbox to let user control how the map data should be loaded seems to me that we're exposing to many implementation details to the end user.

From the end user's point of view, when they are viewing the map, they may not interested in how the data is loaded and adding this checkbox may confuse the user. But of course, this is just my two cents.

Totally understand. However, adding filter implicitly (without an option to disable ) might confuse users too. This option was already available in Coordinate map, so i assume our users are aware of that.
One use case i could think of is, let's say if i want to view top 100 documents ( in an index irrespective of which part of map i am looking at ) i won't be able to do it unless i am at higher zoom level.

For the empty popup, nice catch! Now I'm able to reproduce it when I disabled the tooltip but not refresh the page. And I investigated the issue, I think the reason is mouseenter event listener added by addTooltip function is not unsubscribed(map.off()) when layer config changed(tooltip config). The empty popup is actually from the old mouseenter event listener.

To fix the issue, we would need to unsubscribe the previous event listener whenever adding the new event listener when layer config changed, we do this for click popup in map_container.tsx.

It seems it's not a trivial task to refactor this part, I'd suggest to fix it in a separate task.

Agree. Let's do it in separate task like your suggestion.

ruanyl · 2022-12-30T01:33:22Z

@VijayanB Aha, you're right, thanks for the context, that makes sense to me 👍

VijayanB · 2022-12-30T23:44:48Z

@ruanyl Even when i haven't enable the option to use geo filters, i see requests are triggered in network tab and these requests for some reason have old time filter ex: i updated my time filter to 15 weeks, but, i see these requests uses old time filter vale ( 15 m) .

ruanyl · 2023-01-01T05:37:54Z

@VijayanB Do you mean the global time filter is not applied? Is it a regression due to this PR?

VijayanB · 2023-01-02T05:47:40Z

lobal time filter is not

Ideally no new request should be triggered since useGeofilter is false.
When geofilter is true, time filter is not reflected.

ruanyl · 2023-01-03T02:30:40Z

Ideally no new request should be triggered since useGeofilter is false.

When geofilter is true, time filter is not reflected.

@VijayanB the above issue should have been fixed now

maps_dashboards/public/components/map_container/map_container.tsx

VijayanB · 2023-01-03T03:07:05Z

maps_dashboards/public/components/layer_config/documents_config/document_layer_source.tsx

@@ -303,6 +309,16 @@ export const DocumentLayerSource = ({
            filters={selectedLayerConfig.source.filters ?? []}
            onFiltersUpdated={onFiltersUpdated}
          />
+          <EuiSpacer />
+          <EuiFormRow>
+            <EuiSwitch


I found out that switch is preferred if an action is performed immediately. In this case, the update happens after user update layer config. Shall we change this control to checkbox?

In the screenshot you shared, it's a switch component, shall we make it consistent?

Yeah. I shared screenshot from existing component ( i thought i mentioned in text), but i received this feedback from UX designer for tooltip. We also changed to checkbox for tooltip too.

That's clear, I will update.

maps_dashboards/public/components/layer_config/documents_config/document_layer_source.tsx

VijayanB · 2023-01-03T03:57:06Z

@ruanyl can you rebase with latest changes? Thanks

VijayanB · 2023-01-03T19:05:57Z

maps_dashboards/public/model/layerRenderController.ts

+    const mapBounds = maplibreRef.current.getBounds();
+    const filterBoundingBox = {
+      bottom_right: {
+        lon: adjustLongitudeForSearch(mapBounds.getSouthEast().lng),


Will calling mapBounds.getSouthEast().wrap() solves this problem?

@VijayanB that won't work out of the box, calling wrap() will wrap the longitude to range (-180, 180]. In the screenshot, after wrapping, the longitude of left bounding is great than the right bounding, the consequence is OpenSearch won't return results from the expected bounding box. What do people usually do to tackle this issue in a map application?

i got your point, let's keep your existing solution for bounds crossing date line. I can ask around, but on top of my head i might consider break it into two polygon and add or filter :)

For this did we break the query? I hope no because geobounding box query takes care of these things in the code.

@navneet1v i raised new PR to support corner cases here: #175 . Here, i didn't break it into multiple query, instead wrapped the bounds. However, i had put ceiling to MIN/MAX if at least one complete map is visible. ( hybrid approach ) Let me know whether you see any problem with that approach.

ruanyl · 2023-01-06T01:04:17Z

@junqiu-lei @VijayanB Could you check the PR again?

VijayanB · 2023-01-06T02:04:19Z

@junqiu-lei @VijayanB Could you check the PR again?

Sure. Since there is a conflict, can you rebase? Sorry for keeping this PR unmerged

- allow user to turn on/off geo bounding box query display a switch in map filter config panel to allow user to switch on/off geo bounding box query - search requests will only be sent for layers which have geo bounding query enabled - geo bounding box query will applied global filters Bug fixes: - fixed wrong maplibre instance passed to addTooltip() function which results in a runtime error - fixed empty onHover popup rendered on layer which has tooltip disabled Signed-off-by: Yulong Ruan <[email protected]>

ruanyl · 2023-01-06T02:27:17Z

@VijayanB No worry, the PR is updated now :)

junqiu-lei

Thanks! The PR Looks good to me.

Signed-off-by: Yulong Ruan <[email protected]>

ruanyl requested a review from a team December 29, 2022 08:17

VijayanB reviewed Jan 3, 2023

View reviewed changes

maps_dashboards/public/components/map_container/map_container.tsx Outdated Show resolved Hide resolved

VijayanB reviewed Jan 3, 2023

View reviewed changes

maps_dashboards/public/components/map_container/map_container.tsx Show resolved Hide resolved

VijayanB reviewed Jan 3, 2023

View reviewed changes

maps_dashboards/public/components/layer_config/documents_config/document_layer_source.tsx Show resolved Hide resolved

ruanyl force-pushed the feat-geo-bounding-box branch 2 times, most recently from 98e942e to 53a48ee Compare January 3, 2023 09:28

VijayanB reviewed Jan 3, 2023

View reviewed changes

ruanyl force-pushed the feat-geo-bounding-box branch from 687a8bb to 17a3316 Compare January 6, 2023 02:25

junqiu-lei previously approved these changes Jan 6, 2023

View reviewed changes

fix: ts type error

6fbf779

Signed-off-by: Yulong Ruan <[email protected]>

ruanyl dismissed junqiu-lei’s stale review via 6fbf779 January 6, 2023 02:30

VijayanB previously approved these changes Jan 6, 2023

View reviewed changes

Merge branch 'feature/new-maps' into feat-geo-bounding-box

f448091

junqiu-lei dismissed VijayanB’s stale review via f448091 January 7, 2023 00:06

junqiu-lei approved these changes Jan 7, 2023

View reviewed changes

VijayanB approved these changes Jan 7, 2023

View reviewed changes

junqiu-lei merged commit 05b863d into opensearch-project:feature/new-maps Jan 7, 2023

junqiu-lei mentioned this pull request Jan 7, 2023

Merge Feature/new-maps branch head to main #170

Merged

junqiu-lei added feature v2.5.0 'Issues and PRs related to version v2.5.0' labels Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: query with geo bounding box #148

feat: query with geo bounding box #148

ruanyl commented Dec 29, 2022

VijayanB commented Dec 29, 2022 •

edited

Loading

ruanyl commented Dec 30, 2022

VijayanB commented Dec 30, 2022

ruanyl commented Dec 30, 2022

VijayanB commented Dec 30, 2022

ruanyl commented Jan 1, 2023

VijayanB commented Jan 2, 2023

ruanyl commented Jan 3, 2023 •

edited

Loading

VijayanB Jan 3, 2023

ruanyl Jan 3, 2023

VijayanB Jan 3, 2023

ruanyl Jan 3, 2023

VijayanB commented Jan 3, 2023

VijayanB Jan 3, 2023

ruanyl Jan 4, 2023

VijayanB Jan 5, 2023

navneet1v Jan 7, 2023

VijayanB Jan 7, 2023 •

edited

Loading

ruanyl commented Jan 6, 2023

VijayanB commented Jan 6, 2023

ruanyl commented Jan 6, 2023

junqiu-lei left a comment

feat: query with geo bounding box #148

feat: query with geo bounding box #148

Conversation

ruanyl commented Dec 29, 2022

What's changed:

Bug fixes:

VijayanB commented Dec 29, 2022 • edited Loading

ruanyl commented Dec 30, 2022

VijayanB commented Dec 30, 2022

ruanyl commented Dec 30, 2022

VijayanB commented Dec 30, 2022

ruanyl commented Jan 1, 2023

VijayanB commented Jan 2, 2023

ruanyl commented Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VijayanB commented Jan 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VijayanB Jan 7, 2023 • edited Loading

Choose a reason for hiding this comment

ruanyl commented Jan 6, 2023

VijayanB commented Jan 6, 2023

ruanyl commented Jan 6, 2023

junqiu-lei left a comment

Choose a reason for hiding this comment

VijayanB commented Dec 29, 2022 •

edited

Loading

ruanyl commented Jan 3, 2023 •

edited

Loading

VijayanB Jan 7, 2023 •

edited

Loading