Skip to content
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

Fixed issue #812 #842

Closed
wants to merge 3 commits into from
Closed

Fixed issue #812 #842

wants to merge 3 commits into from

Conversation

ftsalamp
Copy link
Contributor

Fixed issue #812 using Approach A.

Conflicts:
	app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.java
	app/src/main/java/fr/free/nrw/commons/upload/ShareActivity.java
	app/src/main/res/values/strings.xml
@codecov-io
Copy link

Codecov Report

Merging #842 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #842      +/-   ##
=========================================
- Coverage     6.9%   6.85%   -0.05%     
=========================================
  Files          95      95              
  Lines        5071    5105      +34     
  Branches      472     477       +5     
=========================================
  Hits          350     350              
- Misses       4694    4728      +34     
  Partials       27      27
Impacted Files Coverage Δ
...fr/free/nrw/commons/settings/SettingsFragment.java 45.94% <ø> (ø) ⬆️
...java/fr/free/nrw/commons/upload/ShareActivity.java 0% <0%> (ø) ⬆️
...mmons/contributions/ContributionsListFragment.java 0% <0%> (ø) ⬆️
...ee/nrw/commons/media/MediaDetailPagerFragment.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 43d3f05...22ff940. Read the comment docs.

@neslihanturan
Copy link
Collaborator

Thanks for this PR @ftsalamp .
According to my tests on android 5.0.2:

  • It works as intended, saves the photo to gallery on cancellation of upload.
    However, tests on android 4.2.2:
  • Failed with security exception, gallery is stopped. With such error:
    java.lang.SecurityException: Permission Denial: opening provider android.support.v4.content.FileProvider from ProcessRecord{42992670 2928:com.android.gallery3d/u0a52} (pid=2928, uid=10052) that is not exported from uid 10151

@neslihanturan
Copy link
Collaborator

neslihanturan commented Sep 7, 2017

This is a quite important fix, however need some small changes to pass tests. I want to edit this PR to merge it as soon as possible, however there is several approaches to edit someone else's PR. Does anyone can tell me the best approach, to make our commit history clean?

@nicolas-raoul
Copy link
Member

nicolas-raoul commented Sep 7, 2017

@neslihanturan

  1. Have the person git rebase to master, if possible
  2. Create a feature branch with the same name from the same point
  3. Pull from their branch
  4. Do as if it were yours

I would say.
Here ftsalamp worked directly in master, but I suggest you create a feature branch for it.

@neslihanturan
Copy link
Collaborator

Thanks @nicolas-raoul , but I have a small uncertainty. Since this changes on master branch of ftsalamp's fork, and you said create a feature branch from same point, with same name, what should be the name of the new feature branch?

@nicolas-raoul
Copy link
Member

nicolas-raoul commented Sep 9, 2017 via email

@neslihanturan
Copy link
Collaborator

neslihanturan commented Sep 12, 2017

It seems like my tests failed because of a problem that exists on master branch too. So I decided to resolve that issue on another PR. For now, I only resolved conflicts to be able to merge this changes. Thanks for this changes @ftsalamp .

@misaochan
Copy link
Member

@neslihanturan Shall we close this so we can focus on #876 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants