From aec1e513394badccf7de823cfd099d5a2140cc4d Mon Sep 17 00:00:00 2001 From: Josephine Lim Date: Thu, 20 Dec 2018 23:19:27 +1000 Subject: [PATCH] Javadocs and minor refactoring for Nearby package (#2188) * Add Traceur for getting meaningful RxJava stack traces (#1832) * Hotfix for overwrite issue in 2.8.0 (#1838) * This solution is an hotfix for overrite issue came back on 2.8.0 version. What I did is checking the extension, and if it is null, adding .jpg suffix. Because commons files always have suffixes, and we should compare file names after adding suffixes. Othervise overrides are possible. * Check if file title includes an extension already, by checking if is there any dot in it. * Fix logic error * Add uncovered tests * Remove unecessary line breaks * Make Javadocs more explicit * Versioning and changelog for v2.8.2 (#1842) * Versioning for v2.8.2 * Changelog for v2.8.2 * Delete unused MaterialShowcase class * Add Javadocs and fix lint errors for DirectUpload.java * Fix whitespace and add docs * Replace fragment.getActivity() with the parentActivity var * Rename unnecessarily-overloaded method getFromWikidataQuery(), add Javadocs * Javadocs and whitespaces for NearbyPlaces.java * Use local vars where possible instead of class fields. Non-constants should not be in all caps * Missed one unnecessary class field * Remove unnecessary whitespaces that don't improve readability * Add class summary * Optimize imports * Fix access modifiers in Place.java * Clearer Javadocs * Add Javadocs to Place.java * Remove residual conflict * Fix lint issues in Sitelinks * Javadocs for Sitelinks.java * DirectUpload: Replace nested conditionals with guard clauses --- .../free/nrw/commons/nearby/DirectUpload.java | 122 +++++++++++------- .../nrw/commons/nearby/NearbyController.java | 2 +- .../free/nrw/commons/nearby/NearbyPlaces.java | 80 +++++------- .../fr/free/nrw/commons/nearby/Place.java | 54 +++++++- .../fr/free/nrw/commons/nearby/Sitelinks.java | 28 +++- 5 files changed, 186 insertions(+), 100 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/DirectUpload.java b/app/src/main/java/fr/free/nrw/commons/nearby/DirectUpload.java index 5d82b5492b..85439145e9 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/DirectUpload.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/DirectUpload.java @@ -1,20 +1,22 @@ package fr.free.nrw.commons.nearby; +import android.app.Activity; import android.os.Build; import android.support.v4.app.Fragment; import android.support.v4.content.ContextCompat; import android.support.v7.app.AlertDialog; -import android.util.Log; import fr.free.nrw.commons.R; import fr.free.nrw.commons.contributions.ContributionController; import fr.free.nrw.commons.utils.PermissionUtils; -import timber.log.Timber; import static android.Manifest.permission.READ_EXTERNAL_STORAGE; import static android.Manifest.permission.WRITE_EXTERNAL_STORAGE; import static android.content.pm.PackageManager.PERMISSION_GRANTED; +/** + * Initiates the uploads made from a Nearby Place, in both the list and map views. + */ class DirectUpload { private ContributionController controller; @@ -25,59 +27,85 @@ class DirectUpload { this.controller = controller; } - // These permission requests will be handled by the Fragments. - // Do not use requestCode 1 as it will conflict with NearbyFragment's requestCodes + /** + * Initiates the upload if user selects the Gallery FAB. + * The permission requests will be handled by the Fragments. + * Do not use requestCode 1 as it will conflict with NearbyFragment's requestCodes. + */ void initiateGalleryUpload() { - Log.d("deneme7","initiateGalleryUpload"); - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { - if (ContextCompat.checkSelfPermission(fragment.getActivity(), READ_EXTERNAL_STORAGE) != PERMISSION_GRANTED) { - if (fragment.shouldShowRequestPermissionRationale(READ_EXTERNAL_STORAGE)) { - new AlertDialog.Builder(fragment.getActivity()) - .setMessage(fragment.getActivity().getString(R.string.read_storage_permission_rationale)) - .setPositiveButton(android.R.string.ok, (dialog, which) -> { - Timber.d("Requesting permissions for read external storage"); - fragment.getActivity().requestPermissions - (new String[]{READ_EXTERNAL_STORAGE}, PermissionUtils.GALLERY_PERMISSION_FROM_NEARBY_MAP); - dialog.dismiss(); - }) - .setNegativeButton(android.R.string.cancel, null) - .create() - .show(); - } else { - fragment.getActivity().requestPermissions(new String[]{READ_EXTERNAL_STORAGE}, PermissionUtils.GALLERY_PERMISSION_FROM_NEARBY_MAP); - } - } else { - controller.startSingleGalleryPick(); - } + // Only need to handle permissions for Marshmallow and above + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) { + return; } - else { + + Activity parentActivity = fragment.getActivity(); + if (parentActivity == null) { + controller.startSingleGalleryPick(); + return; + } + + // If we have permission, go ahead + if (ContextCompat.checkSelfPermission(parentActivity, READ_EXTERNAL_STORAGE) == PERMISSION_GRANTED) { controller.startSingleGalleryPick(); + return; } + + // If we don't have permission, and we need to show the rationale, show the rationale + if (fragment.shouldShowRequestPermissionRationale(READ_EXTERNAL_STORAGE)) { + new AlertDialog.Builder(parentActivity) + .setMessage(parentActivity.getString(R.string.read_storage_permission_rationale)) + .setPositiveButton(android.R.string.ok, (dialog, which) -> { + parentActivity.requestPermissions(new String[]{READ_EXTERNAL_STORAGE}, PermissionUtils.GALLERY_PERMISSION_FROM_NEARBY_MAP); + dialog.dismiss(); + }) + .setNegativeButton(android.R.string.cancel, null) + .create() + .show(); + return; + } + + // If we don't have permission, and we don't need to show rationale just request permission + parentActivity.requestPermissions(new String[]{READ_EXTERNAL_STORAGE}, PermissionUtils.GALLERY_PERMISSION_FROM_NEARBY_MAP); } + /** + * Initiates the upload if user selects the Camera FAB. + * The permission requests will be handled by the Fragments. + * Do not use requestCode 1 as it will conflict with NearbyFragment's requestCodes. + */ void initiateCameraUpload() { - Log.d("deneme7","initiateCameraUpload"); - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { - if (ContextCompat.checkSelfPermission(fragment.getActivity(), WRITE_EXTERNAL_STORAGE) != PERMISSION_GRANTED) { - if (fragment.shouldShowRequestPermissionRationale(WRITE_EXTERNAL_STORAGE)) { - new AlertDialog.Builder(fragment.getActivity()) - .setMessage(fragment.getActivity().getString(R.string.write_storage_permission_rationale)) - .setPositiveButton(android.R.string.ok, (dialog, which) -> { - fragment.getActivity().requestPermissions - (new String[]{WRITE_EXTERNAL_STORAGE}, PermissionUtils.CAMERA_PERMISSION_FROM_NEARBY_MAP); - dialog.dismiss(); - }) - .setNegativeButton(android.R.string.cancel, null) - .create() - .show(); - } else { - fragment.getActivity().requestPermissions(new String[]{WRITE_EXTERNAL_STORAGE}, PermissionUtils.CAMERA_PERMISSION_FROM_NEARBY_MAP); - } - } else { - controller.startCameraCapture(); - } - } else { + // Only need to handle permissions for Marshmallow and above + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) { + return; + } + + Activity parentActivity = fragment.getActivity(); + if (parentActivity == null) { + controller.startCameraCapture(); + return; + } + + // If we have permission, go ahead + if (ContextCompat.checkSelfPermission(parentActivity, WRITE_EXTERNAL_STORAGE) == PERMISSION_GRANTED) { controller.startCameraCapture(); + return; } + + // If we don't have permission, and we need to show the rationale, show the rationale + if (fragment.shouldShowRequestPermissionRationale(WRITE_EXTERNAL_STORAGE)) { + new AlertDialog.Builder(parentActivity) + .setMessage(parentActivity.getString(R.string.write_storage_permission_rationale)) + .setPositiveButton(android.R.string.ok, (dialog, which) -> { + parentActivity.requestPermissions(new String[]{WRITE_EXTERNAL_STORAGE}, PermissionUtils.CAMERA_PERMISSION_FROM_NEARBY_MAP); + dialog.dismiss(); + }) + .setNegativeButton(android.R.string.cancel, null) + .create() + .show(); + return; + } + + // If we don't have permission, and we don't need to show rationale just request permission + parentActivity.requestPermissions(new String[]{WRITE_EXTERNAL_STORAGE}, PermissionUtils.CAMERA_PERMISSION_FROM_NEARBY_MAP); } } diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyController.java b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyController.java index e6ac27c904..7faebb70c9 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyController.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyController.java @@ -60,7 +60,7 @@ public NearbyPlacesInfo loadAttractionsFromLocation(LatLng curLatLng, LatLng lat Timber.d("Loading attractions neari, but curLatLng is null"); return null; } - List places = nearbyPlaces.getFromWikidataQuery(latLangToSearchAround, Locale.getDefault().getLanguage(), returnClosestResult); + List places = nearbyPlaces.radiusExpander(latLangToSearchAround, Locale.getDefault().getLanguage(), returnClosestResult); if (null != places && places.size() > 0) { LatLng[] boundaryCoordinates = {places.get(0).location, // south diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyPlaces.java b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyPlaces.java index 903a938390..9251490936 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyPlaces.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyPlaces.java @@ -21,11 +21,12 @@ import fr.free.nrw.commons.upload.FileUtils; import timber.log.Timber; +/** + * Handles the Wikidata query to obtain Places around search location + */ public class NearbyPlaces { - private static int MIN_RESULTS = 40; private static final double INITIAL_RADIUS = 1.0; // in kilometers - private static double MAX_RADIUS = 300.0; // in kilometers private static final double RADIUS_MULTIPLIER = 1.618; private static final Uri WIKIDATA_QUERY_URL = Uri.parse("https://query.wikidata.org/sparql"); private static final Uri WIKIDATA_QUERY_UI_URL = Uri.parse("https://query.wikidata.org/"); @@ -46,38 +47,34 @@ public NearbyPlaces() { } /** - * Returns list of nearby places around curLatLng or closest result near curLatLng. This decision - * is made according to returnClosestResult variable. If returnClosestResult true, then our - * number of min results set to 1, and max radius to search is set to 5. If there is no place - * in 5 km radius, empty list will be returned. This method sets radius variable according to - * search type (returnClosestResult or not), but search operation will be handled in overloaded - * method below, which is called from this method. - * @param curLatLng Our reference location - * @param lang Language we will display results in - * @param returnClosestResult If true, return only first result found, else found satisfactory - * number of results - * @return List of nearby places around, or closest nearby place - * @throws IOException + * Expands the radius as needed for the Wikidata query + * @param curLatLng coordinates of search location + * @param lang user's language + * @param returnClosestResult true if only the nearest point is desired + * @return list of places obtained + * @throws IOException if query fails */ - List getFromWikidataQuery(LatLng curLatLng, String lang, boolean returnClosestResult) throws IOException { + List radiusExpander(LatLng curLatLng, String lang, boolean returnClosestResult) throws IOException { + + int minResults; + double maxRadius; + List places = Collections.emptyList(); - /** - * If returnClosestResult is true, then this means that we are trying to get closest point - * to use in cardView above contributions list - */ + // If returnClosestResult is true, then this means that we are trying to get closest point + // to use in cardView in Contributions fragment if (returnClosestResult) { - MIN_RESULTS = 1; // Return closest nearby place - MAX_RADIUS = 5; // Return places only in 5 km area + minResults = 1; // Return closest nearby place + maxRadius = 5; // Return places only in 5 km area radius = INITIAL_RADIUS; // refresh radius again, otherwise increased radius is grater than MAX_RADIUS, thus returns null } else { - MIN_RESULTS = 40; - MAX_RADIUS = 300.0; // in kilometers + minResults = 40; + maxRadius = 300.0; // in kilometers radius = INITIAL_RADIUS; } - // increase the radius gradually to find a satisfactory number of nearby places - while (radius <= MAX_RADIUS) { + // Increase the radius gradually to find a satisfactory number of nearby places + while (radius <= maxRadius) { try { places = getFromWikidataQuery(curLatLng, lang, radius); } catch (InterruptedIOException e) { @@ -85,34 +82,28 @@ List getFromWikidataQuery(LatLng curLatLng, String lang, boolean returnCl return places; } Timber.d("%d results at radius: %f", places.size(), radius); - if (places.size() >= MIN_RESULTS) { + if (places.size() >= minResults) { break; } else { radius *= RADIUS_MULTIPLIER; } } // make sure we will be able to send at least one request next time - if (radius > MAX_RADIUS) { - radius = MAX_RADIUS; + if (radius > maxRadius) { + radius = maxRadius; } - return places; } /** - * Creates and sends query for nearby wikidata items needs picture. - * Reads results from query and turns them into meaningful place variables. - * Adds them to the list of places. - * @param cur Our reference location to check around - * @param lang Language we will display results in - * @param radius Our query area is a circle, where cur is center and radius is radius - * @return - * @throws IOException + * Runs the Wikidata query to populate the Places around search location + * @param cur coordinates of search location + * @param lang user's language + * @param radius radius for search, as determined by radiusExpander() + * @return list of places obtained + * @throws IOException if query fails */ - private List getFromWikidataQuery(LatLng cur, - String lang, - double radius) - throws IOException { + private List getFromWikidataQuery(LatLng cur, String lang, double radius) throws IOException { List places = new ArrayList<>(); String query = wikidataQuery @@ -125,8 +116,7 @@ private List getFromWikidataQuery(LatLng cur, // format as a URL Timber.d(WIKIDATA_QUERY_UI_URL.buildUpon().fragment(query).build().toString()); - String url = WIKIDATA_QUERY_URL.buildUpon() - .appendQueryParameter("query", query).build().toString(); + String url = WIKIDATA_QUERY_URL.buildUpon().appendQueryParameter("query", query).build().toString(); URLConnection conn = new URL(url).openConnection(); conn.setRequestProperty("Accept", "text/tab-separated-values"); BufferedReader in = new BufferedReader(new InputStreamReader(conn.getInputStream())); @@ -162,8 +152,7 @@ private List getFromWikidataQuery(LatLng cur, double latitude; double longitude; - Matcher matcher = - Pattern.compile("Point\\(([^ ]+) ([^ ]+)\\)").matcher(point); + Matcher matcher = Pattern.compile("Point\\(([^ ]+) ([^ ]+)\\)").matcher(point); if (!matcher.find()) { continue; } @@ -192,5 +181,4 @@ private List getFromWikidataQuery(LatLng cur, return places; } - } diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/Place.java b/app/src/main/java/fr/free/nrw/commons/nearby/Place.java index 7635ac06f1..d3eca04650 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/Place.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/Place.java @@ -12,6 +12,9 @@ import fr.free.nrw.commons.location.LatLng; import timber.log.Timber; +/** + * A single geolocated Wikidata item + */ public class Place { public final String name; @@ -22,7 +25,7 @@ public class Place { private final String category; public Bitmap image; - public Bitmap secondaryImage; + private Bitmap secondaryImage; public String distance; public final Sitelinks siteLinks; @@ -38,20 +41,44 @@ public Place(String name, Label label, String longDescription, this.siteLinks = siteLinks; } + /** + * Gets the name of the place + * @return name + */ public String getName() { return name; } + /** Gets the label of the place + * e.g. "building", "city", etc + * @return label + */ public Label getLabel() { return label; } + /** + * Gets the long description of the place + * @return long description + */ public String getLongDescription() { return longDescription; } + /** + * Gets the Commons category of the place + * @return Commons category + */ public String getCategory() {return category; } + /** + * Sets the distance of the place from the user's location + * @param distance distance of place from user's location + */ public void setDistance(String distance) { this.distance = distance; } + /** + * Gets the secondary image url for bookmarks + * @return secondary image url + */ public Uri getSecondaryImageUrl() { return this.secondaryImageUrl; } /** @@ -59,7 +86,7 @@ public void setDistance(String distance) { * @return returns the entity id if wikidata link exists */ @Nullable - public String getWikiDataEntityId() { + String getWikiDataEntityId() { if (!hasWikidataLink()) { Timber.d("Wikidata entity ID is null for place with sitelink %s", siteLinks.toString()); return null; @@ -70,18 +97,35 @@ public String getWikiDataEntityId() { return wikiDataLink.replace("http://www.wikidata.org/entity/", ""); } - public boolean hasWikipediaLink() { + /** + * Checks if the Wikidata item has a Wikipedia page associated with it + * @return true if there is a Wikipedia link + */ + boolean hasWikipediaLink() { return !(siteLinks == null || Uri.EMPTY.equals(siteLinks.getWikipediaLink())); } - public boolean hasWikidataLink() { + /** + * Checks if the Wikidata item has a Wikidata page associated with it + * @return true if there is a Wikidata link + */ + boolean hasWikidataLink() { return !(siteLinks == null || Uri.EMPTY.equals(siteLinks.getWikidataLink())); } - public boolean hasCommonsLink() { + /** + * Checks if the Wikidata item has a Commons page associated with it + * @return true if there is a Commons link + */ + boolean hasCommonsLink() { return !(siteLinks == null || Uri.EMPTY.equals(siteLinks.getCommonsLink())); } + /** + * Check if we already have the exact same Place + * @param o Place being tested + * @return true if name and location of Place is exactly the same + */ @Override public boolean equals(Object o) { if (o instanceof Place) { diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/Sitelinks.java b/app/src/main/java/fr/free/nrw/commons/nearby/Sitelinks.java index f4925af0e5..e67136286c 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/Sitelinks.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/Sitelinks.java @@ -5,13 +5,16 @@ import android.os.Parcelable; import android.support.annotation.Nullable; +/** + * Handles the links to Wikipedia, Commons, and Wikidata that are displayed for a Place + */ public class Sitelinks implements Parcelable { private final String wikipediaLink; private final String commonsLink; private final String wikidataLink; - protected Sitelinks(Parcel in) { + private Sitelinks(Parcel in) { wikipediaLink = in.readString(); commonsLink = in.readString(); wikidataLink = in.readString(); @@ -29,6 +32,9 @@ public int describeContents() { return 0; } + /** + * Creates sitelinks from the parcel + */ public static final Creator CREATOR = new Creator() { @Override public Sitelinks createFromParcel(Parcel in) { @@ -41,18 +47,35 @@ public Sitelinks[] newArray(int size) { } }; + /** + * Gets the Wikipedia link for a Place + * @return Wikipedia link + */ public Uri getWikipediaLink() { return sanitiseString(wikipediaLink); } + /** + * Gets the Commons link for a Place + * @return Commons link + */ public Uri getCommonsLink() { return sanitiseString(commonsLink); } + /** + * Gets the Wikidata link for a Place + * @return Wikidata link + */ public Uri getWikidataLink() { return sanitiseString(wikidataLink); } + /** + * Sanitises and parses the links before using them + * @param stringUrl unsanitised link + * @return sanitised and parsed link + */ private static Uri sanitiseString(String stringUrl) { String sanitisedStringUrl = stringUrl.replaceAll("[<>\n\r]", "").trim(); return Uri.parse(sanitisedStringUrl); @@ -73,6 +96,9 @@ private Sitelinks(Sitelinks.Builder builder) { this.commonsLink = builder.commonsLink; } + /** + * Builds a list of Sitelinks for a Place + */ public static class Builder { private String wikidataLink; private String commonsLink;