-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
5196: Fix location stripped from EXIF metadata #5227
Changes from all commits
62b0346
064d814
ad13fe5
1b9a668
e5945ef
e25783c
27dcc77
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -440,6 +440,9 @@ Upload your first media by tapping on the add button.</string> | |
<string name="ends_on">Ends on:</string> | ||
<string name="display_campaigns">Display campaigns</string> | ||
<string name="display_campaigns_explanation">See the ongoing campaigns</string> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This title might not be so helpful. Could we think of a better title for the description? Something like "Use document style photo picker" amd invert the values accordingly. |
||
<string name="get_content_photo_picker_title">Use GET_CONTENT photo picker</string> | ||
<string name="get_content_photo_picker_explanation">Disable if your pictures get uploaded without location</string> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clearly, it's incorrect to claim that this switch would magically fix all location loss issues. So, may be tweak this into something like (assuming we're doing the above suggested inversion):
We would need some way for users to help recognize the new photo picker. |
||
<string name="location_loss_warning">Please make sure that this new Android picker does not strip location from your pictures.</string> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This message isn't going to help much as we can't expect the users to know what the "new Android picker" means and whether / not it strips location. It would be great to tweak this in a way that helps them in some way. Some set of actionable steps to help them verify this would be better. The only way to not strip locations while using the new picker is to use the "Browse.." option in the new picker's overflow menu. That does not have any location stripping issues since it goes to the Documents UI based picker. We could convey this may be in the dialog. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally I believe this is good enough for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's fine if that we do not need to explain in much detail in the dialog given we have an accompanying help page. My concern is that , description and dialog text expect a bit more Android knowledge that seems ideal. This has been the base of my suggestions and I don't understand how they're not an improvement of the status quo.
My whole point is we can't expect users to understand what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to rephrase the warning as - "Please make sure that the current setting option does not strip location from your pictures" or something along these lines? This way the users who do not have Android knowledge can also understand the intent behind the warning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Additionally giving some hint as to how they could identify that locations are stripped (by checking the "Read More" link) would be helpful. |
||
|
||
<string name="nearby_campaign_dismiss_message">You won\'t see the campaigns anymore. However, you can re-enable this notification in Settings if you wish.</string> | ||
<string name="this_function_needs_network_connection">This function requires network connection, please check your connection settings.</string> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,6 +70,12 @@ | |
app:singleLineTitle="false" | ||
android:summary="@string/display_campaigns_explanation" | ||
android:title="@string/display_campaigns" /> | ||
|
||
<SwitchPreference | ||
android:defaultValue="false" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still prefer keeping this turned on by default (ignoring the inversion mentioned above) with a way to suggest turning it off in a particular case. Given this has merged, I'll open a new issue about it with some details. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would disagree on this one, priority number one is to avoid any data loss, this is far more important than any potential convenience. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My suggestion tries to balance both the data loss and convenience 🙂 More details in this issue: #5242 |
||
android:key="getContentPhotoPickerPref" | ||
android:summary="@string/get_content_photo_picker_explanation" | ||
android:title="@string/get_content_photo_picker_title"/> | ||
</PreferenceCategory> | ||
|
||
<PreferenceCategory | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment could be updated a bit to reflect the current state of the code where it chooses the intent conditionally based on user preference.
The reason for the same could also be conveyed briefly.