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

Pictures can be uploaded directly clicking them from the device's camera #2075

Closed
wants to merge 17 commits into from

Conversation

shashvat-kedia
Copy link
Contributor

Issue #2027 Fixed

@jesmrec
Copy link
Collaborator

jesmrec commented Dec 18, 2017

thanks for your contribution. As usual, it will pass a QA process and code review before merging. If we detect bugs, improvements or issues related with the code, we will report here.

@shashvat-kedia
Copy link
Contributor Author

@jesmrec How much time does it take for the QA process and code review? As I want to work on Issue #2036 and thus any further changes I make will get updated in this PR itself.

@jesmrec
Copy link
Collaborator

jesmrec commented Dec 19, 2017

first step is designing a test plan and execute it (i will take care). Please keep tuned here. In any case, i encourage you not to wait and start developing other stuff if you really wish to do it. Branches finally merges (or rebase) to have all stuff together and perform then appropiate checks.

@shashvat-kedia
Copy link
Contributor Author

@jesmrec There should be a feature that allows the user to reset their Passcode in case they forget it right?

@jesmrec
Copy link
Collaborator

jesmrec commented Dec 19, 2017

@SD1998 there shouldn't. Passcode is the way you can protect the data in your app if you lose your device (for example), in case any other protection in the device is set (not probably). The only way to remove the passcode is inputing the passcode. If not, app has to be removed and reinstalled.

@jesmrec
Copy link
Collaborator

jesmrec commented Dec 20, 2017

@SD1998 this is available only for pics (not videos), isn't it?

@shashvat-kedia
Copy link
Contributor Author

@jesmrec Yes comment

@jesmrec jesmrec added this to the 2.5.1 milestone Dec 20, 2017
@jesmrec
Copy link
Collaborator

jesmrec commented Dec 21, 2017

In landscape orientation, mobile phones does not show the upload option of the FAB (on the top) because there is not enough room for 4 options with this orientation. Any possibility to fix this? or is a collateral effect (feature)? @SD1998 @davigonz

@shashvat-kedia
Copy link
Contributor Author

@jesmrec Should I implement a horizontally opening FAB for landscape orientation?

android:theme="@style/Theme.ownCloud.Fullscreen">
</activity>
android:theme="@style/Theme.ownCloud.Fullscreen" />
<activity android:name=".ui.activity.CapturedFileChecker" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include the suffix "Activity" for all the activities you add here, it will be helpful to distinguish them from services and other components. In this case, CapturedFileCheckerActivity would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the activity tags as there is no such activity present in the project(I created the activity before but then changed my approach and deleted them as they were of no use but the activity tag for them was still present in the Manifest file)

</activity>
android:theme="@style/Theme.ownCloud.Fullscreen" />
<activity android:name=".ui.activity.CapturedFileChecker" />
<activity android:name=".ui.activity.Camera" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, CameraActivity

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment the same applies for the Camera Activity.

build.gradle Outdated

// fix conflict in dependencies; see http://g.co/androidstudio/app-test-app-conflict for details
androidTestCompile "com.android.support:support-annotations:${supportLibraryVersion}"
androidTestCompile "com.android.support:support-annotations:" +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you moved all these dependencies and kept the comments above? The different dependencies were ordered with different comments to know why they have been included. Can you reorder them as before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed this

@davigonz
Copy link
Contributor

davigonz commented Dec 29, 2017

Hi @SD1998 , I've tested your feature and the upload from camera is working, congrats!! 👏

Apart from that, I've just started with the code review and requested you some changes to be applied. I will continue with the review in the coming days, meanwhile you can start to apply those changes, go for it! 😉

Should I implement a horizontally opening FAB for landscape orientation?

We will discuss about it also, maybe some UI changes are needed.

@shashvat-kedia
Copy link
Contributor Author

@davigonz Yes I will start working on the changes.

@davigonz
Copy link
Contributor

davigonz commented Jan 5, 2018

@SD1998 @jesmrec

In landscape orientation, mobile phones does not show the upload option of the FAB (on the top) because there is not enough room for 4 options with this orientation. Any possibility to fix this? or is a collateral effect (feature)?

Should I implement a horizontally opening FAB for landscape orientation?

Well, since the new "Upload from camera" floating button doesn't fit in landscape mode, I've thought in a new design to fix the problem by using bottom sheets:

filelist

upload bottom sheets

So, we have two options here:

  1. Delete the floating button to upload from camera only in landscape mode and include bottom sheets as you can see in the design above.
  2. Delete the floating button to upload from camera in both portrait and landscape modes, and include bottom sheets in both cases, which would be clearer and more consistent for the user.

Besides, using bottom sheets would enable us to add more options in the future (even with a scroll if needed), without depending on the space occupied by new floating buttons.

@SD1998 I would go for the second option, here you have some articles explaining how to implement bottom sheets:

Ping me if you have any question and I will help you

CC / @michaelstingl

@shashvat-kedia
Copy link
Contributor Author

@davigonz Yes even I think the second option is the better option. So I will remove the FAB and implement bottom sheets instead.

@shashvat-kedia
Copy link
Contributor Author

@davigonz Can I use fragments instead of bottom sheets?

@davigonz
Copy link
Contributor

davigonz commented Jan 5, 2018

Can I use fragments instead of bottom sheets?

@SD1998 You can use BottomSheetDialogFragment. In this link you will find an example.

@shashvat-kedia
Copy link
Contributor Author

@davigonz Thanks

@jesmrec
Copy link
Collaborator

jesmrec commented Jan 9, 2018

Please, take in account about the label Upload to ownCloud:

  • ownCloudinstead of OwnCloud
  • ownCloud word has to be brandeable

In any case, this label is not very important so it is implicit that the user wants to upload.

CC @davigonz

@jesmrec
Copy link
Collaborator

jesmrec commented Jan 9, 2018

I have detected the following behaviour, in landscape orientation in Android 8.

jan-09-2018 10-18-22

@shashvat-kedia
Copy link
Contributor Author

@jesmrec Yes I just noticed the behaviour. Actually the bottom sheet in not opening completely only one option is being shown but you can drag the bottom sheet upwards this will show the second option. I am trying to change it so that both the options are shown when the bottom sheets is displayed.

@shashvat-kedia
Copy link
Contributor Author

@jesmrec Regarding this comment I will change OwnCloud to ownCloud.

@shashvat-kedia
Copy link
Contributor Author

@davigonz Are there any more changes required?

@davigonz
Copy link
Contributor

davigonz commented Jan 22, 2018

I have added the feature again and based on the number of files changed I think there have been no unnecessary deletions. Sorry for the inconvenience.

@SD1998 Awesome, the PR changes look better now 👏

Are there any more changes required?

Let me have a final look and if everything is OK, we can start with some QA.

@davigonz
Copy link
Contributor

davigonz commented Jan 22, 2018

Hi again @SD1998 , you've been merging the owncloud master branch into your master branch when it was out-of-date with owncloud master. I'm sorry if I have not commented this to you before but we always use rebase so that all the commits appear not mixed in the history (I will update the CONTRIBUTING.md with this stuff).

Let's see an example:

  1. Clone your repo and checkout to your master branch.
  2. After executing git log and scrolling down a bit, I've noticed this:

screen shot 2018-01-22 at 13 11 38

Here you can see that a master commit appears between two of your commits. To avoid that we always follow the next steps when one of our feature branches is out-of-date with master:

  • git checkout master
  • git fetch
  • git rebase
  • git checkout feature
  • git fetch
  • git rebase
  • git rebase master
  • Solve conflicts, if any
  • git push -f origin feature

When we execute git rebase master, all the commits of the feature branch are placed after the last commit of the master branch.

screen shot 2018-01-22 at 13 25 35

If you want to know more info about it, check this

To have your commits history properly, you will need to perform a rebase with ownCloud master branch, by executing git rebase master (of ownCloud). At this point, you will probably have some conflicts to fix, in several steps, have you ever used rebase with several steps?

Once all the conflicts are fixed, execute git push -f origin/master (be sure before executing this command, it will overwrite your commits history in your master branch) , and all your commits of your feature should appear after master ones, not mixed.

Please, if you have any question, ask me, I hope you are learning a lot about git 😉

@davigonz davigonz mentioned this pull request Jan 22, 2018
3 tasks
@shashvat-kedia
Copy link
Contributor Author

@davigonz When I am using rebase all the files that got deleted before Code Review are getting deleted again (i.e After this all the files were added again but when I rebase all the files that were added are getting removed).

@davigonz
Copy link
Contributor

davigonz commented Jan 23, 2018

When I am using rebase all the files that got deleted before Code Review are getting deleted again (i.e After this all the files were added again but when I rebase all the files that were added are getting removed).

@SD1998 Are you using rebase in several steps? With rebase --continue? This should pass through all the commits, applying them, including the one to add the deleted files.

@shashvat-kedia
Copy link
Contributor Author

@davigonz Yes I am using rebase with several steps.

@shashvat-kedia
Copy link
Contributor Author

@davigonz should I just rebase from the following commit? As this and the commits that follow this particular commit as these are the ones that have all the files.

@davigonz
Copy link
Contributor

should I just rebase from the following commit? As this and the commits that follow this particular commit as these are the ones that have all the files.

@SD1998 and what would happen with the rest of commits? I'm not sure if that will fix it, are you sure you are not missing some conflicts in any step? I will try it to do the rebase locally and see what happens.

@shashvat-kedia
Copy link
Contributor Author

shashvat-kedia commented Jan 24, 2018

@davigonz Will this project be there for GSOC 2018?

@michaelstingl
Copy link
Contributor

@SD1998 please discuss GSOC in https://central.owncloud.org/c/gsoc

@davigonz
Copy link
Contributor

davigonz commented Jan 24, 2018

Will this project be there for GSOC 2018?

@SD1998 We have several proposals in the link @michaelstingl pasted just above, including the one I added yesterday about the integration of a document scanner in the Android app but the accepted organizations will be announced on February 12rd (program timeline). After that, if ownCloud is accepted as project in GSOC 2018, students can begin discussing project ideas.

@davigonz
Copy link
Contributor

@SD1998 during the rebase, all your commits are applied one by one, some of them can trigger conflicts while others not. After this there isn't any commit that can delete the files again since the next commits are:

  1. Feature added.
  2. Deleted String added.
  3. Merge branch 'master' of https://github.com/owncloud/android

Are you following these steps during the rebase?

  1. git rebase master (of ownCloud)
  2. Some conflicts appear
  3. Solve conflicts
  4. git add file_with_conflict_solved (for each file with conflicts solved)
  5. git rebase --continue

Repeat till there are no more conflicts.

@shashvat-kedia
Copy link
Contributor Author

@davigonz Yes I am following those steps. So should I just rebase from the commit that you have mentioned or should I start from the first commit?

@davigonz
Copy link
Contributor

Yes I am following those steps. So should I just rebase from the commit that you have mentioned or should I start from the first commit?

@SD1998 You mean this?

git checkout 434cd8c481fcc1a88c21920d49472655534c43f9
git rebase master (of ownCloud)

This will apply all the previous commits to that specific commit but not the last three, which include the new feature itself.

@shashvat-kedia
Copy link
Contributor Author

@davigonz Yes

@davigonz
Copy link
Contributor

davigonz commented Jan 24, 2018

Yes

@SD1998 if you perform the rebase from that commit, you will need to apply after that the last three commits, you can do it by using git cherry-pick commit command, selecting the commits to apply in the same order you had before.

But the last commit is a merge from master of ownCloud when it should be a rebase

@shashvat-kedia
Copy link
Contributor Author

@davigonz Thanks I will try it now.

@shashvat-kedia
Copy link
Contributor Author

shashvat-kedia commented Jan 24, 2018

@davigonz I used rebase but still the branch is out of date.
screen shot 2018-01-24 at 6 19 55 pm

But I have the changes made in the recent commits in my local repository.

@shashvat-kedia
Copy link
Contributor Author

@davigonz I have used rebase and created a new branch with the feature should I send a PR from that branch and close this one?

@davigonz
Copy link
Contributor

I have used rebase and created a new branch with the feature should I send a PR from that branch and close this one?

@SD1998 Yes please, let's close this and open a new PR.

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.

4 participants