Fixes Issue #5713: "Edit location" is hard to use (and confusing) #5767
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Two minor things:
When tapping Edit, the pin "jumps" a bit:
https://github.com/user-attachments/assets/5702fa4e-4306-48bf-a5a9-28a4dfa682a7
After reinstalling the app (or wiping its storage), the first time GPS location is shown rather than EXIF location:
https://github.com/user-attachments/assets/52cd7b82-4973-494d-91b0-e1173fa10ce7
Probably related to granting permission.
I am fine with merging as-is and creating separate issues, but if you know how to fix them before merging that would be even better. 🙂
|
I'll fix those two problems and re-request a review when I'm done. Thank you for providing those videos. |
Once the "Show in Map App" button is pressed, the Map app will center on the current photo's EXIF location, if that data is available. If not, the map app will center on where the location picker's map is centered. Javadoc updated to reflect this small change.
Before this change, any time the map center had to move, several lines of code had to be written. This change creates several methods which move the map center to a specified location. By using these new methods, moving the map center only takes one simple method call.
The original method did not include a null check for the input GeoPoint. This change includes a null check. If the input Geopoint is null, the method will simply return.
Before this change, there were two methods with the same behavior. This change removes the newer method and renames the old method to the descriptive name the newer one had. Additionally, code which calls this method has been changed to reflect the new name.
Before this change, a method call to move the map to the device's GPS location was called at the very end of the map setup method. This would move the map away from the media's EXIF location data (if it was available). This change places the method call to move the map to the device's GPS location before the method call to move the map to the media's EXIF location (if the data is available). In short, if no exif data is available, the map attempts to move to the device's GPS location. If the exif location data does exist, the map will move to that location.
…ation Before this commit, the method name was unclear and the documentation did not fully explain the method's behavior. Additionally, it was a public method. This commit renames the method (also changing calls to it), adds more documentation, and changes the method scope from public to private.
…p to device GPS location Before this method was created, several lines of code were required to move the location picker map to the device's GPS location. After this method was created, the location picker map can be moved to the GPS location in a single method call.
…evice GPS location Before this change, there was no explicit if check on whether there was EXIF data available before moving the location picker map. After this change, an explicit if check (which checks the activity name string) will move the location picker map to either the EXIF location (if that data is available) or to the device's GPS location (if EXIF data is not available).
Before this change, the showInMapApp method had correct behavior, but could be rewritten to become more clear. After this change, the showInMapApp code has been rewritten. Specifically, common code in an if statement has been factored out.
Before this change, there was no null checks when accessing data from an object, which could create null pointer exceptions at runtime. After this change, null checks are introduced to make sure null pointer exceptions will not occur.
Before this change, a comment in showInMapApp did not properly describe an if statement's intended behavior. After this change, the old comment was removed and 2 new comments were added in each part of the if statement to properly describe the intended behavior.
Before this change, there was several lines of code which moved the map center to the EXIF location. After this change, the several lines of code have been replaced with a simpler method call which produces the same result.
Before this change, there was two sections of code which moved the map center to the same location. Both of these code sections occur very close to each other (one in onCreate(), the other in setupMapView()) After this change, the code section in onCreate() has been removed. With the map centering code only in the setupMapView() method. This change eliminates redundancy, reduces the amount of code in onCreate(), and makes this java file easier to understand.
Before this commit, the location picker pin and shadow graphics were incorrectly located on the screen. Specifically, the bottom of the pin was not located at the center of the view. Since pressing the checkmark button saves the location at the center of the view, users would most likely save the wrong location. After this commit, the location picker pin and shadow graphics have been moved upward. The shadow graphic is now located at the exact center of the view, and the bottom of the location picker pin is directly over the center as well. Users may now use the bottom of the pin as an accurate location picker.
Prior to this change, if the menu asking for permissions to access the device's GPS was accepted, the map would automatically move to the most recent or current GPS location, rather than staying at the uploaded image's available EXIF location. After this change, the map will stay centered at the image's available EXIF location, as intended. The relevant method name and javadoc have been changed to more accurately describe the method's behavior.
Jason-Whitmore
force-pushed
the
location_edit_fix
branch
from
bdf3d17
to
102fbfb
Compare
nicolas-raoul
requested changes
Sep 11, 2024
There was a problem hiding this comment.
I noticed a small issue:
- Open the app
- In the main activity (my uploads), tap on an already uploaded picture. (it might work with other people's pictures in Explore too)
- In the "Coordinates" section, tap the pen icon
- Main branch: The pin is initially centered on the picture's current location. This pull request: The pin is initially centered on my current GPS position. I believe the former makes more sense.
Would you mind checking?
…ion on already uploaded media Before this commit, when the user pressed the edit location icon on media which was already uploaded, the map would center on the device's current GPS location. After this commit, pressing the same edit location icon will now center the map on the location metadata. This is done by checking the activity string to see if the media is already uploaded. If so, a method is called to center the map on the media's location metadata.
Before this commit, there were several mentions of EXIF location data in the javadocs. It is not clear if this is correct, since many images have location data that is chosen by the user rather than derived from EXIF. After this commit, the more generic term "location metadata" is used in place of EXIF location data. Hopefully this change will help avoid confusion in the future.
|
I saw the issue and fixed it. The first new commit fixes the issue. The second commit contains some minor changes to the javadocs. Please let me know if there are any other issues. Thank you! |
nicolas-raoul
approved these changes
Sep 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description (required)
Fixes #5713
What changes did you make and why?
Changes were made for both functionality and clarity.
Functionality:
Changes for functionality were made to solve the first 2 subissues that were described in issue #5713. The third subissue was discussed further in the comments and does not seem to be a bug at this point.
To fix subissue 1, an if check was added to see if the selected image had EXIF location data. If it did, the map center would move to the EXIF location. If the image did not have EXIF location data, the map center would move to the device's GPS location (if available).
To fix subissue 2, the correct location variable was used in the (renamed) showInMapApp() method so that the map app would center on the location EXIF data.
Clarity:
Changes for clarity included moving around, rewriting, or adding code (such as helper methods). Also, redundant code was removed. These changes make the code easier to understand and should help with long term maintenance.
Tests performed (required)
Tested betaDebug on Android Studio Emulator with API level 34.
Screenshots (for UI changes only)
In this first video, the device's GPS location is set to NYC. The first image selected for upload contains EXIF location data of Seattle. When the "Edit Location" button is pressed, the map will center on the EXIF location. The second image selected for upload does not contain EXIF location data. When the "Edit Location" button is pressed, the map will center on the device's GPS location in NYC.
issue5713_part1_compressed.webm
In this second video, the image with EXIF data is selected for upload. In the "Edit Location" menu, the map center is moved out into the water and then the "Show in Map App" button is pressed. The external map app correctly centers on the image's EXIF location, rather than centering on the water.
issue5713_part2_compressed.webm