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

Simple signup #2881

Merged
merged 9 commits into from
Aug 28, 2018
Merged

Simple signup #2881

merged 9 commits into from
Aug 28, 2018

Conversation

tobiasKaminsky
Copy link
Member

2018-08-13-144407

  • splits up in What's new screen (for upgrades) and first run
  • allows to create account on provider

Signed-off-by: tobiasKaminsky tobias@kaminsky.me

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

cleanup

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
tobiasKaminsky added a commit that referenced this pull request Aug 13, 2018
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
tobiasKaminsky added a commit that referenced this pull request Aug 13, 2018
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@codecov
Copy link

codecov bot commented Aug 14, 2018

Codecov Report

Merging #2881 into master will decrease coverage by 0.01%.
The diff coverage is 0.82%.

@@            Coverage Diff            @@
##           master   #2881      +/-   ##
=========================================
- Coverage    6.45%   6.44%   -0.02%     
=========================================
  Files         296     300       +4     
  Lines       29624   29663      +39     
  Branches     4282    4285       +3     
=========================================
- Hits         1913    1912       -1     
- Misses      27426   27466      +40     
  Partials      285     285
Impacted Files Coverage Δ
.../android/authentication/AuthenticatorActivity.java 1.67% <ø> (-0.02%) ⬇️
...com/owncloud/android/ui/activity/BaseActivity.java 1.33% <ø> (ø) ⬆️
.../java/com/owncloud/android/utils/DisplayUtils.java 5.66% <0%> (-0.07%) ⬇️
...m/owncloud/android/ui/activity/DrawerActivity.java 0% <0%> (ø) ⬆️
...cloud/android/ui/activity/FileDisplayActivity.java 0% <0%> (ø) ⬆️
...wncloud/android/ui/whatsnew/ProgressIndicator.java 0% <0%> (ø) ⬆️
...ud/android/ui/activity/ManageAccountsActivity.java 0% <0%> (ø) ⬆️
...ncloud/android/ui/fragment/FeatureWebFragment.java 0% <0%> (ø)
...owncloud/android/ui/activity/WhatsNewActivity.java 0% <0%> (ø) ⬆️
...oud/android/ui/adapter/FeaturesWebViewAdapter.java 0% <0%> (ø)
... and 23 more

@nextcloud nextcloud deleted a comment Aug 14, 2018
@nextcloud nextcloud deleted a comment Aug 14, 2018
@nextcloud nextcloud deleted a comment Aug 14, 2018
@nextcloud nextcloud deleted a comment Aug 14, 2018
@nextcloud nextcloud deleted a comment Aug 14, 2018
@nextcloud nextcloud deleted a comment Aug 14, 2018
@nextcloud nextcloud deleted a comment Aug 14, 2018
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@nextcloud nextcloud deleted a comment Aug 14, 2018
@nextcloud nextcloud deleted a comment Aug 14, 2018
@nextcloud nextcloud deleted a comment Aug 14, 2018
@nextcloud nextcloud deleted a comment Aug 14, 2018
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@nextcloud nextcloud deleted a comment Aug 14, 2018
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@nextcloud nextcloud deleted a comment Aug 14, 2018
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
createAccount(false);
Intent firstRunIntent = new Intent(getApplicationContext(), FirstRunActivity.class);
firstRunIntent.putExtra(FirstRunActivity.EXTRA_ALLOW_CLOSE, true);
startActivity(firstRunIntent);
Copy link
Member

Choose a reason for hiding this comment

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

Why do you want to run the "FirstRunActivity" every time a user wants to add another account? I am fine with having this as a very first run whenever the app is installed (not updated) but even during testing this annoyed my since I always have to click login to get to the real screen since all of my accounts are on the same server. And even if not, why would I want to see the wizard again and not just type in the URL?

Copy link
Member Author

Choose a reason for hiding this comment

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

So you propose to show this only when creating the very first account?
But what would be if you want to create a second account on a different provider?
That is why I chose to show it every time, but I am up for better ideas :-)

cc @jancborchardt

Copy link
Member

Choose a reason for hiding this comment

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

Also interested in @jancborchardt's opinion UX wise since it leads to one additional click every time someone wants to add another account :)

Copy link
Member

Choose a reason for hiding this comment

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

Good point @AndyScherzinger, but I would say we should indeed always show this first run activity. There are lots of cases which lead you to adding another account:

  • One of them is having another account already set up
  • Another is wanting to create a completely new one
  • Another is wanting to create one and then being made aware of the ability to set up your own server.
  • Another is just to see what happens when you tap it, to find out about what the feature means.

Especially as adding another account will not happen that often, the additional click in one of the cases is not that bad. Does that sound ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am also fine with this. We release 3.2.2 this week and can see what the users feedback is.
And then, maybe change it for 3.3.0.
Hope you are fine with this approach, @AndyScherzinger?

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Aug 14, 2018

👍 @tobiasKaminsky all good and working except for one comment, see #2881 (comment) - I strongly vote against showing this screen every time a user adds an account. 🙏

Approved with PullApprove

@AndyScherzinger
Copy link
Member

@tobiasKaminsky I did a bit of code cleanup: c6b2574 😃 Hope you are fine with that

@nextcloud nextcloud deleted a comment Aug 15, 2018
tobiasKaminsky added a commit that referenced this pull request Aug 20, 2018
@AndyScherzinger
Copy link
Member

@jancborchardt all fine for me 👍

@mario
Copy link
Contributor

mario commented Aug 24, 2018

I'd also vote for adding a different way to sign up for a provider if it's your n-th account (where n > 1), but definitely NOT showing this whole thing.

public static final String EXTRA_ALLOW_CLOSE = "ALLOW_CLOSE";
public static final int FIRST_RUN_RESULT_CODE = 199;

private ProgressIndicator mProgress;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather progressIndicator please.

Copy link
Member

Choose a reason for hiding this comment

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

fixed via 0c5b020


return textView;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

New line.

Copy link
Member

Choose a reason for hiding this comment

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

fixed via 0c5b020

@nextcloud-android-bot
Copy link
Collaborator

Lint

TypeMasterPR
Warnings9494
Errors

FindBugs (new)

Warning TypeNumber
Bad practice Warnings35
Correctness Warnings166
Experimental Warnings4
Internationalization Warnings12
Malicious code vulnerability Warnings9
Multithreaded correctness Warnings9
Performance Warnings156
Security Warnings163
Dodgy code Warnings135
Total689

FindBugs (master)

Warning TypeNumber
Bad practice Warnings35
Correctness Warnings165
Experimental Warnings4
Internationalization Warnings12
Malicious code vulnerability Warnings9
Multithreaded correctness Warnings9
Performance Warnings160
Security Warnings163
Dodgy code Warnings135
Total692

@nextcloud nextcloud deleted a comment Aug 24, 2018
@AndyScherzinger
Copy link
Member

@tobiasKaminsky @mario I fixed the latest code review comments. So the only issue remaining is if this should be kept the way it is regarding addition of new accounts when there are already some existing also looking at possible user feedback since this has been backported and shipped with 3.2.2, see #2881 (comment)

@AndyScherzinger
Copy link
Member

also cc @ardevd for feedback if this is fine behavior-wise

@AndyScherzinger
Copy link
Member

To get 3.3.0 out the door I am fine with merging this as-is while I'd still hope we can find a better solution post-3.3.0 like showing the login screen directly (after the initial account has been created) and having the provider selection as an option at the bottom of the login screen.

What do you think @mario? I am not really happy with the actual state but consider it a trade-off to get 3.3.0 ready for an RC even though there are still some more PRs I'd like to see in 3.3.0 :/

@AndyScherzinger AndyScherzinger mentioned this pull request Aug 28, 2018
58 tasks
@AndyScherzinger
Copy link
Member

also fine as-is hoping for us to find a better solution later

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.

5 participants