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

Better run conditions UI #1189

Merged
merged 30 commits into from Aug 2, 2018

Conversation

Projects
None yet
3 participants
@Catfriend1
Copy link
Contributor

Catfriend1 commented Jul 25, 2018

Purpose
Make the run conditions clearer to understand for the user.
Provide the ability to have ticked run condition options effective when the app runs in foreground or background. Previously, run conditions only applied to the "run in background" option which lead to an accidentially burnt data plan for some users (see issue #549).

Screenshots
image
image

Fixed issues:

  • "WiFi only" should not depend on "background/foreground", Wifi only should be default" (fixes #549)
  • "Disable Syncthing in Flightmode" (fixes #1173) ... and enable it on phones that have problems detecting wifi in flight mode the user can manually connect.
  • "Add run condition: Avoid metered networks" (fixes #937)
  • "Option to link Syncthing to Global Android "Auto-sync Data"" (fixes #588)

Related PR:
This is a follow-up to PR #1187.

Status
Most parts have been tested, no problems discovered until now. ( device lg-h815, Android 7.1.2, commit 095a98e )
I offer doing some more intense testing when the review is done.

Forum discussion
https://forum.syncthing.net/t/new-and-improved-run-conditions-for-syncthing-android/11882

@Catfriend1 Catfriend1 added this to the syncthing_v0.14.50 milestone Jul 25, 2018

@Catfriend1 Catfriend1 self-assigned this Jul 25, 2018

Catfriend1 added some commits Jul 25, 2018

WIP - Directly go to SettingsActivity#Run_Conditions screen
after the "change run condition" button has been clicked in
StateDialogActivity during STATE_DISABLED

Catfriend1 added some commits Jul 25, 2018

Disable Syncthing in flight mode (fixes #1173)
... and allow to enable it on phone that have problems
detecting wifi connection if the user manually enables
one during flight mode.

@Catfriend1 Catfriend1 changed the title WIP - Better run conditions UI (fixes #549) WIP - Better run conditions UI Jul 25, 2018

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 25, 2018

@AudriusButkevicius @capi @Nutomic
Hi together,
I've made a proposal of a new run conditions UI and verified everything working correctly at commit 50ea1e4 . May I have your honest feedback on this. Please tell me if you like/don't like it. I would love to see the long outstanding issues fixed by this.

Although this PR's changes completely work built on top of the current master, some parts of it should get a follow-up by introducing the most important three options like "mobile data, wifi, metered wifi" via the first start welcome wizard (see PR #1176). It's technically okay but not very nice to greet a new user that has just installed syncthing app while on mobile data connection with "syncthing is disabled" (as "Run on wifi" is the default). This should be explained by the new welcome wizard slides.

Debug build: https://build.syncthing.net/repository/download/SyncthingAndroid_Build/24402:id/outputs/apk/debug/app-debug.apk

Catfriend1 added some commits Jul 26, 2018

Implement mPendingRunConditions in SettingsActivity
to queue run condition changes until the user leaves
the preferences screen after making changes. (fixes #1196)
@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 26, 2018

So the wording can be improved, and I haven't looked at the code, but I guess it's a sensible improvement.

What happens if you uncheck run on wifi and run on mobile data?
Does it even make sense not to run on wifi ever?

One of the options is in german so I have no idea what it means.

Catfriend1 added some commits Jul 26, 2018

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 26, 2018

@AudriusButkevicius I think you meant this part:
image
This option wasn't modified it works like it has been before. It is the "Respect android battery saving" run condition. If checked, it ensures syncthing gets disabled as soon as the phone gets in the "save energy" state (during the last percent when discharging or when the quick toggle is hit).

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 26, 2018

@AudriusButkevicius ( Does it even make sense not to run on wifi ever? )
Yes, looking through the issues I recall some users told that they use syncthing in "default configured manner" over mobile data connection syncing little amounts of data. I wouldn't cut down this possibility by the run conditions but syncing on wifi being the default (which is proposed here already) definitely makes sense. ( That's why we also need the welcome wizard to explain this and put a toggle on it. )

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 26, 2018

@AudriusButkevicius ( What happens if you uncheck run on wifi and run on mobile data? )
Syncthing gets into the "disabled" state as expected by logical means. We would have the later ability to add WiMax and so on as separate check boxes when users request it with a valid use case.
If you uncheck wifi+mobile data, you can for example add "Run when device in flight mode" and syncthing gets started when totally offline. On older phones, the flight mode introduces wifi detection problems (already described) and the box can help to combine with "run on wifi".

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 26, 2018

If anything I think we should invert the Wifi condition and have it "Disabled on Wifi" or something like that, as it does not make sense not to want to have that box checked in most cases.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 26, 2018

@AudriusButkevicius You're right about the wifi check box. We always want it. Except on tethering. That's why the wifi checkbox enables the "Run when wifi tethered" checkbox subsequently. The reason why I used the "positive naming" (instead of the old "blacklist" naming with the "only" wording) was it's easier to understand for newbies as they go round ticking "what they want to happen" and not what they don't want. From my usage perspective, it's no problem to invert this as I know what's meant. What I still consider a little bit unlucky are the "blacklist" conditions like "charging", "power saving" or "auto-sync data". Those act as blockers disregarding all other circumstances like mobile data, wifi, etc. when one of them is ticked and the condition is not met. I hope we get more opinion on this as I feel like I should not decide this alone. I think that was the reason for Nutomic originally wording the conditions "stop on mobile data" and "stop on wifi". Btw the code is easier to read (and handle) if we do "positive" cases where we can. If you like, I'll put a commit so you can see the "Disable Wifi" variant and test it how the UI feels like then?!

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 29, 2018

@imsodin ( Do the apply regardless of how the app is started? )
Before PR: No. / With PR: Yes.

I think we should do this as you suggested: ( then you could just have an option to apply run conditions, where all these options are offered. ) Is it okay making the option default?

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 29, 2018

Why would you not want the run conditions to apply always?
How did the run conditions get applied before, if not always?

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 29, 2018

@AudriusButkevicius Before the PR, run conditions only applied if the user decided to always run syncthing android in the background as a service when the phone booted. Accidents syncing on mobile data connection happened because most users manually started the app and forgot to close it manually through the drawer as switching over to another app didn't suffice.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 29, 2018

A right, this is a great improvement in that case.

But as far as I recall it used to say syncthing is disabled and force you to fiddle with run conditions, even if you launched the app manually?

@AudriusButkevicius
Copy link
Member

AudriusButkevicius left a comment

In principle this looks good to me, but I'll leave for you two guys to decide once this is ready.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 29, 2018

Still ToDo:

  • @imsodin ( I also have some comments on individual strings, but I will write them up once the general structure is agreed upon. )
  • Improve the welcome wizard and "synching is disabled" UI to avoid greeting users like @AudriusButkevicius commented above. I would do them in follow-up PR's.
@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 29, 2018

Found a minor bug, will fix it shortly.
We should run "evaluateRunConditions" onResume() of the MainActivity. Otherwise, if the user changes the metered flag for a wifi in Android 7, the app will not notice it as there is no broadcast.

@imsodin
Copy link
Member

imsodin left a comment

Now that I actually understand what this PR does (as in apply run conditions unconditionally, which is great!), I have other questions. I'll try to check in more often, I am just pretty busy away from the computer atm.

Before there was the always run in background option - that seems to be gone. Is that correct? If so, will Syncthing now always start automatically or never (I believe neither possibility would be good). I consider the old name confusion anyway, but an option like "Start Syncthing automatically" should still be there, right? Looking at the current settings structure, that option could be part of Behaviour?

The flight mode option isn't in the screenshot? What is its purpose anyway? I mean afaik that's just a quick toggle to disable both cellular and wifi, i.e. the wifi and mobile data run conditions should be enough?
Same for Service setting.

The rest are just string nit-picks.

@@ -306,9 +316,29 @@ Please report any problems you encounter via Github.</string>
<string name="sync_only_wifi_ssids_location_permission_rejected_dialog_title">Permission required</string>
<string name="sync_only_wifi_ssids_location_permission_rejected_dialog_content">Starting with Android 8.1, location access is required to be able to read the WiFi\'s name. You can use this feature only if you grant this permission.</string>

<string name="power_source_title">Run when device powered by</string>

This comment has been minimized.

@imsodin

imsodin Jul 30, 2018

Member

Run when device is powered by

<string-array name="power_source_entries">
<item>AC and battery power</item>
<item>AC power</item>
<item>Battery power</item>

This comment has been minimized.

@imsodin

imsodin Jul 30, 2018

Member

Consequently you need to add a final dot here as well, if you do it on line 328 below.


<string name="sync_only_wifi_ssids">Restrict to certain wifi networks</string>
<string name="run_on_whitelisted_wifi_title">Run on specified wifi networks</string>
<string name="run_on_whitelisted_wifi_networks">Run on selected wifi networks: %1$s</string>

This comment has been minimized.

@imsodin

imsodin Jul 30, 2018

Member

Maybe "Run on selected wifi networks only" or "Run only on selected wifi networks".
(Bold is just for purposes of review, not to be done in the actual UI).

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 31, 2018

@imsodin The "start syncthing automatically option" equals to the previous UI option "always run in background" and is here:
image
The flight mode option is on the screenshot:
image
You are right that logically flight mode is a combination out of wifi and mobile data connection, but this option, as the summary below it says, is there for a workaround as some phones have problems detecting the flight mode correctly, plus, the user can make exceptions from it by enabling wifi manually when in flight mode (Android shows indeed two icons then, flight mode and wifi connection). That's why I made this option as there are tickets reported about it. I also have this problem on my phone. To better understand the flight mode issue it needs testing on different phones running different android versions. If you look at other apps like e.g. Folder Sync or CSipSimple that also have network-related run conditions you'll also find this "workaround option" for flight mode.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 31, 2018

@imsodin The "Run as background service" option could be moved to another sub pref screen (which has to be available instead of greyed out when syncthing is not running). From code perspective, it is a run condition (see BootReceiver, NotificationManager and AppConfigReceiver) which applies to running the android app automatically (not syncthing core). From user perspective, it's indeed behaviour.
Hmm... you're right. If this will never lead to the screen "Syncthing is disabled" we can move it. I'll check if behaviour is always available and not greyed out, then move it.

Review - Minor string improvements
Removed unused variable from RunConditionMonitor
Move always_run_in_background pref to pref category
"Behaviour".
@imsodin

This comment has been minimized.

Copy link
Member

imsodin commented Jul 31, 2018

Must have been blind regarding those options.
I think it's a bit hidden at the bottom of run conditions for a pretty important setting, but thematically I don't think it's far off there. Also the small text below Service does explain what it means, but people are lazy. "Run as background service" is probably the technically correct term, but I believe something like "Start service automatically on boot" would be easier to understand.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Aug 1, 2018

@imsodin Are we ready now or can I still improve it?

@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

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Aug 1, 2018

@imsodin Would you please be so kind and review the "resolve merge conflicts" commit a022ea5 ? I think its okay as those are two constants taken over from the welcome wizard PR manually as github couldn't merge automatically.

@Catfriend1 Catfriend1 requested a review from imsodin Aug 1, 2018

@imsodin

imsodin approved these changes Aug 2, 2018

@Catfriend1 Catfriend1 merged commit 6a04d64 into syncthing:master Aug 2, 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:newRunCondUI branch Aug 2, 2018

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