EXIF removal set to off by default #3043

VitalyVPinchuk · 2019-06-28T09:04:24Z

Description (required)
Settings -> Privacy -> Manage EXIF TAGS -> all tags checked

Based upon discussion #2863 (comment)

What changes did you make and why?
Set default values to ON for manageExifTags preference.

Tested betaDebug on emulator with API level 28.

* Fix bugs in peer review flow * Fix tests * Bug fixes * Fix remaining issues with peer review

pullrequest

✅ A review job has been created and sent to the PullRequest network.

@VitalyVPinchuk you can click here to see the review status or cancel the code review job.

codecov-io · 2019-06-28T09:15:48Z

Codecov Report

Merging #3043 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #3043      +/-   ##
=========================================
- Coverage    4.41%   4.41%   -0.01%     
=========================================
  Files         259     259              
  Lines       12309   12310       +1     
  Branches     1056    1056              
=========================================
  Hits          544     544              
- Misses      11726   11727       +1     
  Partials       39      39

Impacted Files	Coverage Δ
...java/fr/free/nrw/commons/upload/FileProcessor.java	`3.96% <0%> (ø)`	⬆️
...fr/free/nrw/commons/settings/SettingsFragment.java	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fb3331...2170020. Read the comment docs.

ashishkumar468 · 2019-06-28T13:10:55Z

Hi @VitalyVPinchuk , I was looking into the code, In FileProcessor#getExifTagsToRedact. As far as I could understand, there is no where we check which tags need to be redacted

Set<String> redactTags = new HashSet<>(Arrays.asList(
                context.getResources().getStringArray(R.array.pref_exifTag_values)));

This is code which fetches the list of attributes to be redacted, but how does it verify the on/off state ?

VitalyVPinchuk · 2019-06-28T13:26:17Z

Hi @VitalyVPinchuk , I was looking into the code, In FileProcessor#getExifTagsToRedact. As far as I could understand, there is no where we check which tags need to be redacted
Set<String> redactTags = new HashSet<>(Arrays.asList(
                context.getResources().getStringArray(R.array.pref_exifTag_values)));
This is code which fetches the list of attributes to be redacted, but how does it verify the on/off state ?

Hi @ashishkumar468
This line gets only the ticked tags
Set<String> prefManageEXIFTags = defaultKvStore.getJson(Prefs.MANAGED_EXIF_TAGS, setType);
and then
if (prefManageEXIFTags != null) redactTags.removeAll(prefManageEXIFTags);
excludes them from the whole set of tags.

ashishkumar468 · 2019-06-28T13:32:31Z

@VitalyVPinchuk As far as I understand, the ticked one's should not be redacted, and those which are not ticked, those should be redacted. Would be better if one of @misaochan @neslihanturan @maskaravivek @nicolas-raoul could confirm this ?

VitalyVPinchuk · 2019-06-28T13:43:21Z

e ticked one's should not be redacted,

Yes. This is exactly what it does. What seems to be wrong?

ashishkumar468 · 2019-06-28T14:07:56Z

`Set prefManageEXIFTags = defaultKvStore.getJson(Prefs.MANAGED_EXIF_TAGS, setType);```

@VitalyVPinchuk This gives null when all of them are ticked, because of which, these attributes do not get removed from the attributes which need to be redacted, as a result of which, even the ticket attributes get redacted

VitalyVPinchuk · 2019-06-28T14:18:31Z

Strange.
This should return set of strings with 7 elements when all the tags are selected.
And the method should return hashset of zero elements, which means there is nothing to redact.
It works for me this way, but I'll check once again.

ashishkumar468 · 2019-06-28T15:33:40Z

Hi @VitalyVPinchuk , I rechecked, the value which is coming from preference is null

Attaching relevant screenshots

ashishkumar468 · 2019-06-28T15:39:10Z

A probable cause is that although these values are checked by default, they have no entries in the preferences and would be added only when we check/uncheck them and that is why null is being returned when we ask for it.

VitalyVPinchuk · 2019-06-28T19:44:08Z

A probable cause is that although these values are checked by default, they have no entries in the preferences and would be added only when we check/uncheck them and that is why null is being returned when we ask for it.

Yes, you're absolutely right. Now I see. The values are added to preferences only after checking/unchecking them. I didn't know about that. Thanks!

pullrequest

lgtm

Reviewed with ❤️ by PullRequest

VitalyVPinchuk · 2019-06-29T06:56:43Z

Hello!
Default values from preferences.xml (android:defaultValue) are ignored.
So I've tried to setDefaultValues(this, R.xml.preferences, false). But Set<String> prefManageEXIFTags = defaultKvStore.getJson(Prefs.MANAGED_EXIF_TAGS, setType) still returns null, unlike
Set<String> prefManageEXIFTags = sharedPreferences.getStringSet("manageExifTags", new HashSet<String>()), which returns hashset of selected tags.

What is the best way to set defaults for preferences from xml file?

* Removed hardcoded strings in DeleteHelper * Revert "Removed hardcoded strings in DeleteHelper" This reverts commit 4bc55fc.

ashishkumar468 · 2019-07-03T07:37:46Z

Hi @VitalyVPinchuk , Apologies for the delayed response, you can use something like this
PreferenceManager.setDefaultValues(this, R.xml.preference, true);

See this

VitalyVPinchuk · 2019-07-03T08:27:15Z

I wonder what I should do to set defaults from xml into defaultKvStore. I've already set defaults for shared preferences and they return right values.

neslihanturan · 2019-07-03T09:03:19Z

Hi @VitalyVPinchuk , please do not use shared preferences at all. Instead use defaultKvStore. You can simply search in settings fragment and see it's usages. And then please remove shared preferences added by you. An example:
defaultKvStore.putBoolean(Prefs.IS_CONTRIBUTION_COUNT_CHANGED, true);

VitalyVPinchuk · 2019-07-03T09:15:01Z

Hi @neslihanturan , sure I will remove shared preferences. Just added the code for test reasons. But is there a way to use defaults from XML with defaultKvStore just like PreferenceManager.setDefaultValues(this, R.xml.preference, true);?

neslihanturan · 2019-07-03T09:38:38Z

Hi @VitalyVPinchuk , I think this line "addPreferencesFromResource(R.xml.preferences);" already gets all default preferences values. So multiSelectListPref should have the default values. Can you try seeing the default content of:

multiSelectListPref.getValues()

VitalyVPinchuk · 2019-07-03T13:13:40Z

Thanks! It works.

But we also need to use defaults even without opening settings activity (where we load them). What could be done about that?

ashishkumar468 · 2019-07-03T13:15:04Z

@VitalyVPinchuk Do you mean that the values are initialised only when SettingsActivity is opened ?

VitalyVPinchuk · 2019-07-03T13:16:28Z

@VitalyVPinchuk Do you mean that the values are initialised only when SettingsActivity is opened ?

I guess so.

ashishkumar468 · 2019-07-03T13:29:51Z

@VitalyVPinchuk Do you mean that the values are initialised only when SettingsActivity is opened ?

I guess so.

Then can we not initialise them in the App class , CommonsApplication ?

app/src/main/java/fr/free/nrw/commons/CommonsApplication.java

maskaravivek

@VitalyVPinchuk Can you please make the suggested changes. Also, IMO we should simply check if shared pref has the exif values set or not. If its not set at all then we can assume that the user doesn't want us to remove EXIF information.

app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.java

…t been initialized

VitalyVPinchuk · 2019-07-04T09:56:32Z

@VitalyVPinchuk Can you please make the suggested changes. Also, IMO we should simply check if shared pref has the exif values set or not. If its not set at all then we can assume that the user doesn't want us to remove EXIF information.

Hi @maskaravivek
I got your point and made those changes.

But still, I think we should read android:defaultValue from xml into JsonKvStore prior to opening SettingsFragment.

For example, if I change default value for android:key="uploads" from 100 to 5 and install app anew, it still loads 100 images.

misaochan · 2019-07-10T13:55:52Z

Is this PR ready for testing? :)

VitalyVPinchuk · 2019-07-10T14:13:38Z

Yes, it is.
Or do you mean that tests should be added?

pullrequest

✖️ This code review was cancelled. See Details

misaochan · 2019-07-11T12:10:51Z

Don't worry about the unit tests - this is a bugfix anyway so no need for that. :)

Yes, I was planning on testing your PR as all PRs need to be manually tested before we merge them. But I just noticed that you have submitted your PR to master, whereas this bugfix is needed in 2.11-release. Could you rebase your PR on that branch and resubmit to it, please? Sorry for the inconvenience, we should have mentioned that earlier.

VitalyVPinchuk · 2019-07-11T13:45:32Z

Don't worry about the unit tests - this is a bugfix anyway so no need for that. :)

Yes, I was planning on testing your PR as all PRs need to be manually tested before we merge them. But I just noticed that you have submitted your PR to master, whereas this bugfix is needed in 2.11-release. Could you rebase your PR on that branch and resubmit to it, please? Sorry for the inconvenience, we should have mentioned that earlier.

Hello @misaochan
Sure, I'll try to rebase though I've never done this before :)
Any help appreciated!

maskaravivek · 2019-07-11T14:00:18Z

@misaochan @VitalyVPinchuk I just rebased the PR against 2.11-release. You can click on the Edit button on the right side of the PR title and change the base branch. In most cases, there won't be a merge conflict if the change is small.

misaochan · 2019-07-12T09:44:21Z

@maskaravivek Changing the base alone is insufficient, though - as you can see, now there are 72 files changed. ;) The person submitting the PR still needs to rebase on 2.11-release via git.

@VitalyVPinchuk You can use git rebase upstream/2.11-release

maskaravivek

Approving the code changes. Wasn't able to test the changes.

VitalyVPinchuk · 2019-07-12T11:00:37Z

@maskaravivek Changing the base alone is insufficient, though - as you can see, now there are 72 files changed. ;) The person submitting the PR still needs to rebase on 2.11-release via git.

@VitalyVPinchuk You can use git rebase upstream/2.11-release

Hello @misaochan @maskaravivek
Is it still necessary to do git rebase upstream/2.11-release?

ashishkumar468 · 2019-07-12T12:22:56Z

Yes, @VitalyVPinchuk . Please do it. As you can see the number of file changes are 72, and relevant to yours would be 3-4 I guess :-)

…t been initialized

* Removed hardcoded strings in DeleteHelper * Revert "Removed hardcoded strings in DeleteHelper" This reverts commit 4bc55fc.

…s-android-commons into fix_defaults_for_exif

misaochan · 2019-07-13T08:39:23Z

Umm, there are still 66 changes, haha...

@VitalyVPinchuk , would it help if you created a new PR from scratch? You can create a local branch based on 2.11-release, copy your changes to that branch, and then submit your PR to 2.11-release, if that would be easier for you than trying to figure out the rebase.

Alternatively we can create the PR for you, but then it wouldn't be attributed to you, so we will try to avoid this if possible.

VitalyVPinchuk · 2019-07-13T11:28:42Z

Ok, I'll make a local branch from 2.11-release and put the changes there.
I used to play with rebase command but never used it in real life so clearly I did something wrong.

maskaravivek and others added 3 commits June 26, 2019 21:31

Fix bugs in peer review flow (commons-app#3039) (commons-app#3042)

d762895

* Fix bugs in peer review flow * Fix tests * Bug fixes * Fix remaining issues with peer review

Localisation updates from https://translatewiki.net.

0c3b002

EXIF removal set to off by default

9adb3e6

pullrequest bot reviewed Jun 28, 2019

View reviewed changes

pullrequest bot reviewed Jun 29, 2019

View reviewed changes

[WIP] Set defaults for SharedPreferences

db1f5ac

translatewiki and others added 2 commits July 1, 2019 09:22

Localisation updates from https://translatewiki.net.

cc92773

Removed hardcoded strings in DeleteHelper (commons-app#3047)

6fb3331

* Removed hardcoded strings in DeleteHelper * Revert "Removed hardcoded strings in DeleteHelper" This reverts commit 4bc55fc.

maskaravivek reviewed Jul 4, 2019

View reviewed changes

app/src/main/java/fr/free/nrw/commons/CommonsApplication.java Outdated Show resolved Hide resolved

maskaravivek requested changes Jul 4, 2019

View reviewed changes

app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.java Outdated Show resolved Hide resolved

app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.java Outdated Show resolved Hide resolved

EXIF removal minor fixes: no tags are removed if shared prefs have no…

06e14be

…t been initialized

Merge branch 'master' into fix_defaults_for_exif

2170020

pullrequest bot reviewed Jul 10, 2019

View reviewed changes

maskaravivek changed the base branch from master to 2.11-release July 11, 2019 13:58

maskaravivek approved these changes Jul 12, 2019

View reviewed changes

translatewiki and others added 7 commits July 12, 2019 15:27

Localisation updates from https://translatewiki.net.

9a5880c

EXIF removal set to off by default

1fc9291

[WIP] Set defaults for SharedPreferences

67c6ae1

EXIF removal minor fixes: no tags are removed if shared prefs have no…

fe27638

…t been initialized

Localisation updates from https://translatewiki.net.

fe5982e

Removed hardcoded strings in DeleteHelper (commons-app#3047)

cc82a21

* Removed hardcoded strings in DeleteHelper * Revert "Removed hardcoded strings in DeleteHelper" This reverts commit 4bc55fc.

Merge branch 'fix_defaults_for_exif' of github.com:VitalyVPinchuk/app…

5722c6c

…s-android-commons into fix_defaults_for_exif

VitalyVPinchuk closed this Jul 13, 2019

VitalyVPinchuk mentioned this pull request Jul 13, 2019

EXIF removal set to off by default (base 2.11-release) #3068

Merged

EXIF removal set to off by default #3043

EXIF removal set to off by default #3043

Conversation

VitalyVPinchuk commented Jun 28, 2019

pullrequest bot left a comment

Choose a reason for hiding this comment

codecov-io commented Jun 28, 2019 • edited Loading

Codecov Report

ashishkumar468 commented Jun 28, 2019 • edited Loading

VitalyVPinchuk commented Jun 28, 2019

ashishkumar468 commented Jun 28, 2019

VitalyVPinchuk commented Jun 28, 2019

ashishkumar468 commented Jun 28, 2019 • edited Loading

VitalyVPinchuk commented Jun 28, 2019

ashishkumar468 commented Jun 28, 2019 • edited Loading

ashishkumar468 commented Jun 28, 2019

VitalyVPinchuk commented Jun 28, 2019

pullrequest bot left a comment

Choose a reason for hiding this comment

VitalyVPinchuk commented Jun 29, 2019

ashishkumar468 commented Jul 3, 2019 • edited Loading

VitalyVPinchuk commented Jul 3, 2019 • edited Loading

neslihanturan commented Jul 3, 2019

VitalyVPinchuk commented Jul 3, 2019

neslihanturan commented Jul 3, 2019

VitalyVPinchuk commented Jul 3, 2019

ashishkumar468 commented Jul 3, 2019

VitalyVPinchuk commented Jul 3, 2019

ashishkumar468 commented Jul 3, 2019

maskaravivek left a comment

Choose a reason for hiding this comment

VitalyVPinchuk commented Jul 4, 2019

misaochan commented Jul 10, 2019

VitalyVPinchuk commented Jul 10, 2019

pullrequest bot left a comment

Choose a reason for hiding this comment

misaochan commented Jul 11, 2019 • edited Loading

VitalyVPinchuk commented Jul 11, 2019

maskaravivek commented Jul 11, 2019

misaochan commented Jul 12, 2019 • edited Loading

maskaravivek left a comment

Choose a reason for hiding this comment

VitalyVPinchuk commented Jul 12, 2019

ashishkumar468 commented Jul 12, 2019

misaochan commented Jul 13, 2019

VitalyVPinchuk commented Jul 13, 2019

codecov-io commented Jun 28, 2019 •

edited

Loading

ashishkumar468 commented Jun 28, 2019 •

edited

Loading

ashishkumar468 commented Jun 28, 2019 •

edited

Loading

ashishkumar468 commented Jun 28, 2019 •

edited

Loading

ashishkumar468 commented Jul 3, 2019 •

edited

Loading

VitalyVPinchuk commented Jul 3, 2019 •

edited

Loading

misaochan commented Jul 11, 2019 •

edited

Loading

misaochan commented Jul 12, 2019 •

edited

Loading