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

Add welcome wizard slides on first start requesting permissions #1176

Merged
merged 17 commits into from Aug 1, 2018

Conversation

Projects
None yet
3 participants
@Catfriend1
Copy link
Contributor

Catfriend1 commented Jul 3, 2018

Purpose
Improve the app by adding a welcome wizard explaining required and optional permissions.

Screenshots
image
image
image
image

Testing
Verified working on device lg-h815 running Android 7.1.2 at commit fd490d8 .

EDIT: Removed the relation to issue #1129 . I'll do that fix based on this PR in a follow-up so this doesn't get too big.

@Catfriend1 Catfriend1 self-assigned this Jul 3, 2018

Catfriend1 added some commits Jul 16, 2018

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 20, 2018

Uploaded new screenshots and inserted icon assets and texts.
EDIT: There is also a back button now.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 20, 2018

@AudriusButkevicius @capi @Nutomic I've uploaded new screenshots for my suggestion of a welcome wizard for syncthing. Do you agree if I finish the first implementation as shown above? Would you like to work with me on that together as I'm not a designer?
I intent to make this PR completely ready for the three welcome steps shown above and then - with your okay - we could merge it so another one can improve design and texts. I've also planned to move the one-time init step "Generating keys and config ..." (spinner on first app launch) to a fourth slide after the location permission question as this is one time stuff which is "ugly" implemented into MainActivity.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 20, 2018

Its all in german so I have no idea what the guide says.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 20, 2018

@AudriusButkevicius see strings.xml for the English texts. The german parts got loaded because I reused the texts that syncthing already had for german since many releases.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 22, 2018

So the wording can be improved but in general, it looks good. I would move all the logic for this into it's own class/view/activity/whatever it's called, so we don't clutter the already large existing class.

Add welcome wizard code
Ask for storage and location permission
Location permission is required to run the app
Recheck storage permission on each app start, if revoked the
welcome slides show again.

@Catfriend1 Catfriend1 requested review from capi and AudriusButkevicius Jul 24, 2018

@Catfriend1 Catfriend1 changed the title WIP - Add welcome wizard on first start Add welcome wizard slides on first start requesting permissions (fixes #1129) Jul 24, 2018

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 24, 2018

@AudriusButkevicius The code for the slides requesting permissions from the user is not huge, so I decided to add it to FirstStartActivity (which was nearly "empty" before). It's all in one place now :).

* Storage permission has been granted.
*/
private void startApp() {
Boolean doInitialKeyGeneration = !Constants.getConfigFile(this).exists();

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 24, 2018

Author Contributor

Sidenote: I've replaced the "isFirstStart" check (previously done via preference check) because it was not very reliable detecting if syncthing needs to generate new keys and config. We now use the same indicator we already have in other classes (e.g. ConfigXml). If no config is present (or it got corrupted in a way crucial files are missing) we'll trigger the "key and config generation" process on app start. This gracefully auto-fixes an unrecoverable config break so we don't have to advise users to clear app's data and cache.

Catfriend1 added some commits Jul 24, 2018

Catfriend1 added some commits Jul 26, 2018

@Catfriend1 Catfriend1 added this to the 0.10.14 milestone Jul 27, 2018

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 27, 2018

@AudriusButkevicius I'm really missing feedback from others on those UI enhancement PR's ... so thank you for giving feedback on them. The question is, can we assume people might be happy with this if we merge and I'll further extend it in follow-up PR's by new slides?
I recall you'd liked to improve the wording, so no first priority, but if you'd like to suggest a better wording in strings.xml, I'll put them in.

@Catfriend1 Catfriend1 changed the title Add welcome wizard slides on first start requesting permissions (fixes #1129) Add welcome wizard slides on first start requesting permissions Jul 27, 2018

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 27, 2018

I'll look at this at some point this weekend given the WIP part is gone.

@imsodin
Copy link
Member

imsodin left a comment

If you have high-level stuff that you think would benefit from another set of eyes, you can ping me. I am not the ideal candidate for it, as I am not very invested in advance functionalities of the app (I am happy as long as it starts and stops when I manually tell it to). I don't follow the syncthing-android activities closely and have no plans to dig into it, but as far as I have time and something to say I am happy to provide feedback. Just to have said it explicitly: Consider all of my feedback as suggestions - I don't have any stakes in the android wrapper.

I believe this is a good improvement, it's always nice to be up front about required permissions. Are these pages conditional, i.e. only show up if the permissions weren't already granted?

<string name="cont">Continue</string>
<!-- Slide 2 -->
<string name="storage_permission_title">Storage permission</string>
<string name="storage_permission_desc">Syncthing requires the storage permission to do file synchronization. We ask you to grant the permission.</string>

This comment has been minimized.

@imsodin

imsodin Jul 27, 2018

Member

As this is a required permission, I wouldn't ask and have a button - just explain and then request the permission on pressing "next".

Alternative wording that doesn't just repeat the title and seems "clearer" and easier to me:
Syncthing needs to access your storage to do file synchronization.

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

I think this summons a separate system dialog, hence has to be "initiated" somehow.

Agree with the better wording.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

Pressing "next" would add little block to our btnNext click handler, no problem to do this, in my opinion the UI feels more polite if the user has something to click like the button "hey I grant you what you asked for".
I put the text "Syncthing needs to access your storage to do file synchronization."

<string name="welcome_text">Syncthing is an open-source file synchronization application.\n\
To share data with other devices, you need to add their unique device IDs to the device list. Afterwards you can select which folders to share with which devices.\n\
Please report any problems you encounter via Github.</string>

<string name="cont">Continue</string>
<!-- Slide 2 -->
<string name="storage_permission_title">Storage permission</string>

This comment has been minimized.

@imsodin

imsodin Jul 27, 2018

Member

Potentially capitalize fully, as it's a title.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

Ok, let's see how it looks.

This comment has been minimized.

@imsodin

imsodin Jul 30, 2018

Member

"Fully" was misleading, sorry. I didn't mean all caps, I meant capitalization as in "Location Permission" (like in how you do it in titles, i.e. capitalize every first letter unless it's words like of, and, or, ...).

<string name="storage_permission_desc">Syncthing requires the storage permission to do file synchronization. We ask you to grant the permission.</string>

<!-- Slide 3 -->
<string name="location_permission_title">Location permission</string>

This comment has been minimized.

@imsodin

imsodin Jul 27, 2018

Member

Also maybe capitalize.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

Ok, let's see how it looks.


<!-- Slide 3 -->
<string name="location_permission_title">Location permission</string>
<string name="location_permission_desc">Syncthing can be configured to run only on selected wifi networks. Due to android requirements, you need to grant the permission if you\'d like to use this feature. Otherwise, feel free to deny it.</string>

This comment has been minimized.

@imsodin

imsodin Jul 27, 2018

Member

Here the button makes sense, as it's voluntary. I'd turn the latter part around to focus on that choice instead of the android requirement. Something like If you want to use this feature, press the button above to give the required location access to Syncthing. Otherwise you can skip this step.

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

I think this needs an explanation why...

Syncthing can be configured to run only on selected wifi networks. Sadly, Android requires applications to have location permissions to be able look up active WiFi network name, as you can sometimes infer users location from the name of the network they are connected to. If you want to use this feature, press the button above to give the required location permissions to Syncthing. Otherwise you can skip this step.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

Hmm.. both suggestions are good. imsodin's is comforting the user but the user doesn't really get why a location permission should be granted for a wifi feature. Audrius' suggestion tells the truth (what for example complies with fdroid policy for app approval) but also looks a tiny bit scary as we say what nasty things we could do (but won't do :). For the first try on "later" public feedback I'll take Audrius' suggestion.

This comment has been minimized.

@imsodin

imsodin Jul 30, 2018

Member

Fine with me, but please drop the sadly. We want the user to be cheerful about using Syncthing ;)

private static final int REQUEST_WRITE_STORAGE = 142;
private static final int SLIDE_POS_LOCATION_PERMISSION = 1;

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

What are these magic numbers?

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

This is an exception in the wizard as we have to make sure the app quits if the user didn't grant storage permissions during the second slide and tell him when exiting why we do it (that was already in the original welcome screen). So it is a slide index to make it easier if we need to rearrange slides later. I chose this way as the ViewPager doesn't really help here returning something unique and easily available to get the xml_layout_slide_name.

/**
* Recheck storage permission. If it has been revoked after the user
* completed the welcome slides, displays the slides again.
*/

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

Are we going to show all slides if the permission is revoked?

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

Yes, I decided to do so in the first proposal as I consider it rare that users revoke the permission if they actively use the app. (see rare one bug report on the tracker where one user did). Reason is it will never harm anything going through the slides again - also being able to grant the location permission if this was also revoked "later" after completing the wizard for the first time. Cluttering the code to handle rare cases is a maybe can do but currently don't see necessary.

private TextView[] dots;
private int[] layouts;
private Button btnBack;
private Button btnNext;

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

Why no m prefix for these?

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

I've had them temporarily in another context, added the "m" and gave the buttons a new var name.

if (!haveStoragePermission()) {
Toast.makeText(this, R.string.toast_write_storage_permission_required,
Toast.LENGTH_LONG).show();
this.finish();

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

What does finish do in this case?

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

I exits the app by ending the FirstStartActivity like it was the case before the PR if the user did not grant the required storage permission.

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 28, 2018

Member

Shouldn't we just reprompt the user to grant the permission? It would be very strange if "next" exits the app.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 28, 2018

Author Contributor

Android development guidelines for permission request say we're not allowed to ask this multiple times on our own. That's why the button makes sense.

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 28, 2018

Member

We should just disable the next button if the user has not granted permissions.
Or atleast print a message or something because now it will feel like a crash.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 28, 2018

Author Contributor

A message is already printed on the screen when exiting, so it's no crash.
Disabling the next button would leave no way to exit the foreground app window. User would then have to press home or the panic button.

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 28, 2018

Member

Why can't he press the back button?

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 28, 2018

Member

Or we could just make clicking next print the toast and not progress to the new window. Then the user can back out, or grant the permission.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 28, 2018

Author Contributor

I'll think about that. Good for follow-up.

return mViewPager.getCurrentItem() + i;
}

// viewpager change listener

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

capitalisation

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

ok

@Override
public boolean isViewFromObject(View view, Object obj) {
return view == obj;
}

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

Why do we need to override this?

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

The method instantiateItem(ViewGroup, int) returns an Object for a particular view. PagerAdapter implementation is considering this Object as a key value when viewpager changes a page. If we return the view itself from instantiateItem(ViewGroup, int), then our key for that page becomes the view itself. We can check return view == obj; from isViewFromObject which will always return true and our pages will display.

@Override
public void destroyItem(ViewGroup container, int position, Object object) {
View view = (View) object;
container.removeView(view);

This comment has been minimized.

@AudriusButkevicius

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

Proper uninitialization and the above mentioned reason.

<color name="bg_screen1">#3395ff</color>
<color name="bg_screen2">#20d2bb</color>
<color name="bg_screen3">#c873f4</color>
<!-- <color name="bg_screen4">#f64c73</color> -->

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

Is there harm in having these uncommented if there are only 3 layouts?

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

Uncommenting will throw a lint warning but not be a problem. I'll reserved the color as I already know what the fourth slide will be.

<string name="cont">Continue</string>
<!-- Slide 2 -->
<string name="storage_permission_title">Storage permission</string>
<string name="storage_permission_desc">Syncthing requires the storage permission to do file synchronization. We ask you to grant the permission.</string>

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

I think this summons a separate system dialog, hence has to be "initiated" somehow.

Agree with the better wording.


<!-- Slide 3 -->
<string name="location_permission_title">Location permission</string>
<string name="location_permission_desc">Syncthing can be configured to run only on selected wifi networks. Due to android requirements, you need to grant the permission if you\'d like to use this feature. Otherwise, feel free to deny it.</string>

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

I think this needs an explanation why...

Syncthing can be configured to run only on selected wifi networks. Sadly, Android requires applications to have location permissions to be able look up active WiFi network name, as you can sometimes infer users location from the name of the network they are connected to. If you want to use this feature, press the button above to give the required location permissions to Syncthing. Otherwise you can skip this step.

Catfriend1 added some commits Jul 27, 2018

@Catfriend1 Catfriend1 modified the milestones: 0.10.14, 0.10.15 Jul 28, 2018

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 28, 2018

Other than the next button exiting if storage permission is not granted, looks good to me.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 28, 2018

@AudriusButkevicius Okay, then let's go. As we currently have many discussion I'd suggest a follow-up PR would be great to meet again making the "app exits when storage permission not granted" better.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 29, 2018

Anything left todo, or are we good to merge this?

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 29, 2018

Ok from my side. (I'll already mentioned I'd like to take care of more welcome slides, especially those required to make the user experience smooter on first app start relating to key generation and run conditions).

@imsodin: Is is okay from your side? Some of your feedback already got in.

@imsodin
Copy link
Member

imsodin left a comment

I left my notes on an outdated review, meaning they are hidden. Thus to repeat here:

The location permission explanation is fine with me, but please drop the sadly. We want the user to be cheerful about using Syncthing ;)

And I didn't mean to put the strings in all caps, but to capitalize them, i.e. make the first letter uppercase: Storage permissions -> Storage Permissions (again, that's just how I am used to do things for title-like properties, maybe mixed capitalization looks better. I don't care much about it, except that it is not all caps :)

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 30, 2018

Thanks for reviewing, @imsodin I got all comments via mail that were hidden and will take care later.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 31, 2018

@AudriusButkevicius Would you mind leaving the merges up to me after a PR has got the required approvements? Then I would plan to do a final testing after the reviews and before hitting the merge.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 31, 2018

Sure, but everytime you merge something in the branch the approval gets dismissed.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Aug 1, 2018

Ok, testing completed, no crashes or problems so far found.

@Catfriend1 Catfriend1 merged commit cdaf8e6 into syncthing:master Aug 1, 2018

2 checks passed

Build (Syncthing Android) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Catfriend1 Catfriend1 deleted the Catfriend1:addWelcomeWizard branch Aug 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment