From 5e271c0e089327fcbfe9c630874149a0b4ff6b29 Mon Sep 17 00:00:00 2001 From: mpl Date: Thu, 2 Aug 2018 03:35:21 +0200 Subject: [PATCH] perkeepd: remove map zoom guided by loc: or locrect: As well as the rectangle which was drawn to show the area corresponding to the requested location. The initial zoom is now only based on the location of the search results. As (the intended) side-effect, geocoding requests are not needed anymore, since the map aspect does not need to know the coordinates of a loc: request anymore. Which allowed to trim down the server/perkeepd/ui/goui/geo pkg. Updates #1115 Change-Id: I22983661fd482b652d8bf34a63fa88d7b7695d5e --- server/perkeepd/ui/goui/geo/geo.go | 157 ------------------- server/perkeepd/ui/goui/main.go | 16 +- server/perkeepd/ui/goui/mapquery/mapquery.go | 6 - server/perkeepd/ui/map.js | 92 +---------- 4 files changed, 10 insertions(+), 261 deletions(-) diff --git a/server/perkeepd/ui/goui/geo/geo.go b/server/perkeepd/ui/goui/geo/geo.go index edb1d33fe..f554c9ff3 100644 --- a/server/perkeepd/ui/goui/geo/geo.go +++ b/server/perkeepd/ui/goui/geo/geo.go @@ -19,14 +19,10 @@ limitations under the License. package geo import ( - "context" "fmt" - "log" - "math" "strconv" "strings" - "perkeep.org/internal/geocode" "perkeep.org/pkg/types/camtypes" ) @@ -36,35 +32,6 @@ const ( LocMapPredicatePrefix = "map" ) -// HandleLocAreaPredicate checks whether predicate is a location area predicate -// (locrect). If so, it runs asynchronously handleCoordinatesFound on the given -// coordinates, and returns true. Otherwise, it returns false. -func HandleLocAreaPredicate(predicate string, handleCoordinatesFound func(*camtypes.LocationBounds)) bool { - r, err := rectangleFromPredicate(predicate, LocAreaPredicatePrefix) - if err != nil { - return false - } - go handleCoordinatesFound(r) - return true -} - -// HandleLocAreaPredicate checks whether predicate contains a map location predicate -// (map:). If so, it runs asynchronously handleCoordinatesFound on the given -// coordinates, and returns true. Otherwise, it returns false. -func HandleZoomPredicate(predicate string, handleCoordinatesFound func(*camtypes.LocationBounds)) bool { - tp := strings.TrimSpace(predicate) - if tp == "" { - return false - } - fields := strings.Fields(tp) - r, err := rectangleFromPredicate(fields[len(fields)-1], LocMapPredicatePrefix) - if err != nil { - return false - } - go handleCoordinatesFound(r) - return true -} - // IsLocMapPredicate returns whether predicate is a map location predicate. func IsLocMapPredicate(predicate string) bool { if _, err := rectangleFromPredicate(predicate, LocMapPredicatePrefix); err != nil { @@ -100,127 +67,3 @@ func rectangleFromPredicate(predicate, kind string) (*camtypes.LocationBounds, e West: coord[1], }, nil } - -// IsLocPredicate returns whether the given predicate is a simple (as in, not -// composed) location predicate, such as the one supported by the Perkeep search -// handler (e.g. "loc:seattle"). -func IsLocPredicate(predicate string) bool { - if !strings.HasPrefix(predicate, LocPredicatePrefix+":") { - return false - } - loc := strings.TrimPrefix(predicate, LocPredicatePrefix+":") - if !strings.HasPrefix(loc, `"`) { - if len(strings.Fields(loc)) > 1 { - // Not a simple location query, but a logical one. Deal with it in another CL. - // TODO(mpl): accept more complex queries. - return false - } - return true - } - // we have a quoted location - if !strings.HasSuffix(loc, `"`) { - // the quoted location ends before the end of the query, or the quote never closes. either way, refuse that. - return false - } - if strings.Count(loc, `"`) != 2 { - // refuse anything that is not just one quoted location - return false - } - return true -} - -// Lookup searches for the coordinates of the given location, and passes the -// found zone (a rectangle), if any, to handleCoordinatesFound. -func Lookup(location string, handleCoordinatesFound func(*camtypes.LocationBounds)) { - go func() { - rect, err := geocode.Lookup(context.Background(), location) - if err != nil { - log.Printf("geocode lookup error: %v", err) - return - } - if len(rect) == 0 { - log.Printf("no coordinates found for %v", location) - return - } - handleCoordinatesFound(&camtypes.LocationBounds{ - North: rect[0].NorthEast.Lat, - South: rect[0].SouthWest.Lat, - East: rect[0].NorthEast.Long, - West: rect[0].SouthWest.Long, - }) - }() -} - -// Location is a geographical coordinate, specified by its latitude and its longitude. -type Location struct { - Lat float64 // -90 (south) to 90 (north) - Long float64 // -180 (west) to 180 (east) -} - -// TODO(mpl): write tests for LocationCenter, if we end up keeping it. not -// needed anymore for now, but might soon very well be. Otherwise remove. - -// LocationCenter returns the center of the rectangle defined by the given -// coordinates. -func LocationCenter(north, south, west, east float64) Location { - var lat, long float64 - if west < east { - long = west + (east-west)/2. - } else { - // rectangle spanning longitude ±180° - awest := math.Abs(west) - aeast := math.Abs(east) - if awest > aeast { - long = east - (awest-aeast)/2. - } else { - long = west + (aeast-awest)/2. - } - } - // TODO(mpl): are there tricky cases at ±90? - lat = south + (north-south)/2. - return Location{ - Lat: lat, - Long: long, - } -} - -// EastWest is returned by WrapAntimeridian. It exists only because there's no -// multi-valued returns with javascript functions, so we need WrapAntimeridian to -// return some sort of struct, that gets converted to a javascript object by -// gopherjs. -type EastWest struct { - E float64 - W float64 -} - -// WrapAntimeridian determines if the shortest geodesic between east and west -// goes over the antimeridian. If yes, it converts one of the two to the closest -// equivalent value out of the [-180, 180] range. The choice of which of the two to -// convert is such as to maximize the part of the geodesic that stays in the -// [-180, 180] range. -// The reason for that function is that leaflet.js cannot handle drawing areas that -// cross the antimeridian if both corner are in the [-180, 180] range. -// https://github.com/Leaflet/Leaflet/issues/82 -func WrapAntimeridian(east, west float64) EastWest { - if west < east { - return EastWest{ - E: east, - W: west, - } - } - lc := LocationCenter(50, -50, west, east) - if lc.Long > 0 { - // wrap around the +180 antimeridian. - newEast := 180 + (180 - math.Abs(east)) - return EastWest{ - E: newEast, - W: west, - } - } - // else wrap around the -180 antimeridian - newWest := -180 - (180 - west) - return EastWest{ - E: east, - W: newWest, - } -} diff --git a/server/perkeepd/ui/goui/main.go b/server/perkeepd/ui/goui/main.go index 08b7cb4d1..29c24d46f 100644 --- a/server/perkeepd/ui/goui/main.go +++ b/server/perkeepd/ui/goui/main.go @@ -24,7 +24,6 @@ import ( "perkeep.org/server/perkeepd/ui/goui/aboutdialog" "perkeep.org/server/perkeepd/ui/goui/dirchildren" "perkeep.org/server/perkeepd/ui/goui/downloadbutton" - "perkeep.org/server/perkeepd/ui/goui/geo" "perkeep.org/server/perkeepd/ui/goui/importshare" "perkeep.org/server/perkeepd/ui/goui/mapquery" "perkeep.org/server/perkeepd/ui/goui/selectallbutton" @@ -40,22 +39,15 @@ func main() { "ShareItemsBtn": sharebutton.New, "SelectAllBtn": selectallbutton.New, "NewDirChildren": dirchildren.New, - "Geocode": geo.Lookup, - "IsLocPredicate": geo.IsLocPredicate, // TODO: we want to investigate integrating the share importer with the other // importers. But if we instead end up keeping it tied to a dialog, we need to add // a cancel button to the dialog, that triggers the context cancellation. "ImportShare": func(cfg map[string]string, shareURL string, updateDialogFunc func(message string, importedBlobRef string)) { importshare.Import(context.TODO(), cfg, shareURL, updateDialogFunc) }, - "HandleLocAreaPredicate": geo.HandleLocAreaPredicate, - "HandleZoomPredicate": geo.HandleZoomPredicate, - "LocPredicatePrefix": geo.LocPredicatePrefix, - "LocationCenter": geo.LocationCenter, - "WrapAntimeridian": geo.WrapAntimeridian, - "NewMapQuery": mapquery.New, - "DeleteMapZoom": mapquery.DeleteZoomPredicate, - "ShiftMapZoom": mapquery.ShiftZoomPredicate, - "HasZoomParameter": mapquery.HasZoomParameter, + "NewMapQuery": mapquery.New, + "DeleteMapZoom": mapquery.DeleteZoomPredicate, + "ShiftMapZoom": mapquery.ShiftZoomPredicate, + "HasZoomParameter": mapquery.HasZoomParameter, }) } diff --git a/server/perkeepd/ui/goui/mapquery/mapquery.go b/server/perkeepd/ui/goui/mapquery/mapquery.go index b1e5e4bae..c9241de60 100644 --- a/server/perkeepd/ui/goui/mapquery/mapquery.go +++ b/server/perkeepd/ui/goui/mapquery/mapquery.go @@ -234,12 +234,6 @@ func (q *Query) SetZoom(north, west, south, east float64) { q.Expr = handleZoomPredicate(q.Expr, false, zoomExpr) } -// GetZoom returns the location area that was requested for the last successful -// query. -func (q *Query) GetZoom() *camtypes.LocationBounds { - return q.zoom -} - // HasZoomParameter returns whether queryString is the "q" parameter of a search // query, and whether that parameter contains a map zoom (map predicate). func HasZoomParameter(queryString string) bool { diff --git a/server/perkeepd/ui/map.js b/server/perkeepd/ui/map.js index 5f2aeb4b0..7c20d28bb 100644 --- a/server/perkeepd/ui/map.js +++ b/server/perkeepd/ui/map.js @@ -93,12 +93,6 @@ cam.MapAspect = React.createClass({ }, componentWillMount: function() { - this.location = { - North: 0.0, - South: 0.0, - East: 0.0, - West: 0.0, - }; this.clusteringOn = this.props.config.mapClustering; if (this.clusteringOn == false) { // Even 100 is actually too much, and https://github.com/perkeep/perkeep/issues/937 ensues @@ -117,7 +111,6 @@ cam.MapAspect = React.createClass({ this.cluster = null; this.markersGroup = null; this.mapQuery = null; - this.locationFound = false; this.locationFromMarkers = null; this.initialSearchSession = this.props.searchSession; }, @@ -189,91 +182,18 @@ cam.MapAspect = React.createClass({ ); }, - // setCoordinatesFromSearchQuery looks into the search session query for obvious - // geographic coordinates. Either a location predicate ("loc:seattle"), or a - // location area predicate ("locrect:48.63,-123.37,46.59,-121.28") are considered - // for now. - setCoordinatesFromSearchQuery: function() { + triggerInitialZoom: function() { var q = this.initialSearchSession.getQueryExprOrRef(); - if (goreact.IsLocPredicate(q)) { - // a "loc" query - goreact.Geocode(q.substring(goreact.LocPredicatePrefix.length), function(rect) { - return this.handleCoordinatesFound(rect, true); - }.bind(this)); - return; - } - if (goreact.HandleLocAreaPredicate(q, function(rect) { - return this.handleCoordinatesFound(rect, true); - }.bind(this))) { - // a "locrect" area query - return; - } q = goreact.ShiftMapZoom(q); - if (goreact.HandleZoomPredicate(q, function(rect) { - return this.handleCoordinatesFound(rect, false); - }.bind(this))) { - // we have a zoom (map:) in the query - return; - } - // Not a location type query window.dispatchEvent(new Event('resize')); }, - // handleCoordinatesFound sets this.location (a rectangle), this.latitude, and - // this.longitude (center of this.location), from the given rectangle. - handleCoordinatesFound: function(rect, draw) { - if (!rect) { - return; - } - var eastWest = goreact.WrapAntimeridian(rect.East, rect.West); - rect.West = eastWest.W; - rect.East = eastWest.E; - if (this.sameLocations(rect, this.location)) { - return; - } - this.location = rect; - if (draw) { - L.rectangle([[this.location.North, this.location.East],[this.location.South,this.location.West]], {color: "#ff7800", weight: 1}).addTo(this.map); - } - this.locationFound = true; - window.dispatchEvent(new Event('resize')); - return; - }, - - // refreshMapView pans to the relevant coordinates found for the current search - // session, if any. Otherwise, pan to englobe all the markers that were drawn. + // refreshMapView pans to englobe all the markers that were drawn. refreshMapView: function() { - var zoom = null; - if (!this.locationFound && !this.locationFromMarkers) { - if (!this.mapQuery) { - return; - } - zoom = this.mapQuery.GetZoom(); - if (!zoom) { - return; - } + if (!this.locationFromMarkers) { + return; } - if (zoom) { - // TODO(mpl): I think we want to remove that case, now that locationFound also - // takes into account when a "map:" predicate is found in the initial search - // session query. - var location = L.latLngBounds(L.latLng(zoom.North, zoom.East), L.latLng(zoom.South, zoom.West)); - } else if (this.locationFound) { - // pan to the location we found in the search query itself. - var location = L.latLngBounds(L.latLng(this.location.North, this.location.East), - L.latLng(this.location.South, this.location.West)); - } else { - // otherwise, fit the view to encompass all the markers that were drawn - var location = this.locationFromMarkers; - } - this.map.fitBounds(location); - }, - - sameLocations: function(loc1, loc2) { - return (loc1.North == loc2.North && - loc1.South == loc2.South && - loc1.West == loc2.West && - loc1.East == loc2.East) + this.map.fitBounds(this.locationFromMarkers); }, // loadMarkers sets markers on the map for all the permanodes, with a location, @@ -475,7 +395,7 @@ cam.MapAspect = React.createClass({ // needed/useless as MapSorted queries do not support continuation of any kind. if (this.firstLoad) { - this.setCoordinatesFromSearchQuery(); + this.triggerInitialZoom(); } // even if we're not here because of a zoom change (i.e. either first load, or // new search was entered), we still call updateSearchBar here to update the zoom