Fix #5246: map icon in Upload Wizard indicating if location is included in the EXIF data #5343
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
The existing map icon may not be intuitive enough to indicate whether the location EXIF data will be included The two new XML map icons are intended to indicate the status of location sharing with the location data in the Upload Wizard
If an image is capture with the in-app camera, the location in the image metadata by default If so, the map icon in the Upload Wizard should be labelled with a green tick during initialisation of its UploadMediaDetailFragment instance
If the user selects images from the device storage to upload, the location EXIF data might originally not be included The map icon is labelled with a red question mark After pin-pointing the location manully, the map icon should be labelled with a green tick instead
|
Would you mind posting fullscreen screenshots of the whole activity, in both dark mode and light mode? |
SVG path is invalid, resulting in failure to render the icons Also imports are required for UploadMediaDetailFragment to use Drawable objects and R objects
When an image is chosen from the album to the Upload Wizard, its EXIF might contain location data. hasLocation() and fix of init() in UploadMediaDetailFragment ensures that the map icon is shown correctly
|
Here we go: 
Test performed on a physical Android 13 phone. There were some SVG rendering problems with XML in the initial three commits. This has now been fixed. |
|
Is this still a draft, or ready for merging? |
nicolas-raoul
requested changes
Oct 17, 2023
app/src/main/java/fr/free/nrw/commons/filepicker/UploadableFile.java
Outdated
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragment.java
Outdated
Show resolved
Hide resolved
Ready for review now.
Surely I can do the refinement 👍🏼 |
Fix the comment about red and green labels for the map icon
Instead of using printStackTrace(), error directed to logcat
nicolas-raoul
requested changes
Oct 17, 2023
Clean up the catch clause in hasLocation() and getDataTimeFromExif()
Resolved. |
nicolas-raoul
approved these changes
Oct 18, 2023
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)
Regarding issue #5246, the idea is to have variants of the map icon (that enables the user to manually add the location for the image EXIF) in the Upload Wizard.
Similar to the suggestion in the issue's discussion thread, my idea is to have the map icon labelled with a red question mark if the location is not to be uploaded. On the other hand, if the location is included in the EXIF, the map icon is labelled with a green tick. See the icons below.
I used Inkscape to modify the SVG path data and transformed it back to the Android XML format. Just wonder if there are any more intuitive and clear icons than the currently proposed ones. If so, we can create and test them in the app.
In terms of the change in the map icon, I reckon that the location is included by default if the image is taken by the in-app camera. That's why there is concern that users may unintentionally share their location as mentioned in the issue thread. For this, we use
inAppPictureLocationto determine which icon to display.Another situation is when the user manually pinpoints a location. The map icon is then labelled with a green tick.
I'd be happy to have insights for:
If the location removal feature in #5262 is available, then I guess this shall also be extended (i.e. changing the green tick back to a red question mark).
Tests performed (required)
To be completed after confirming details.
Screenshots (for UI changes only)