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

Appconfig receiver #599

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@sqrt-1764
Copy link

sqrt-1764 commented Mar 27, 2016

Reopened Pull-Request after renaming my branch.

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Mar 27, 2016

So this is good to test now? What app did you use to test the intents?

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Mar 27, 2016

Grrr - commented on the wrong thread ;-)

I just committed my "final" version, if you want to take a look. Now I have to create the tests ...

The AppConfigReceiver-Service can be easily extended to support more commands.
Currently I implemented the action "sycthingservice" with the commands "start" and "stop".

In short words what I did:

"start" just (re)starts the Syncthingservice via context.startService().
"stop" stops the Syncthingservice by a call to startService with the new action "shutdown". This because a stopService command when the Syncthingservice wasn't active resulted in a runtime-error of the syncthing-binary.
Like this, the service can handle the shutdown internally and delay the stopService call if needed.

For the delay, I found the mStopScheduled member-variable which seems to be there for exact this job.
If mCurrentStatus is not INIT or STARTING, I stop the service immediately.
Otherwise I set mStopScheduled to true - which then does the rest.

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Mar 27, 2016

I tested this with automate
Created a flow for these two actions and hooked them to the connect and disconnect of my WIFI.

I then toggled my WIFI and watched Syncthing starting and stopping.

SyncThing.flo.zip

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Mar 27, 2016

I think it is good for a first test now.

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Mar 27, 2016

Added tests for the AppConfigReceiver class.
Did not test the changed SyncThingService class.

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Mar 29, 2016

What error did you get with stopService? The documentation says it does nothing if the service is not running.

I tried to test it with Automate (imported your flow from the zip), but I think I'm doing something wrong. After pressing the start button, checking both items and pressing ok, it either starts or stops the service, but doesn't follow the wifi state. Also, Automate says "Running Fibers: Not Running".

When Syncthing is shut down, the "Syncthing is disabled" dialog is coming up (with a button leading to the settings). There should probably be a different dialog in this case, saying that Syncthing was disabled by an external application.

Also, it's "Syncthing", without capital T ;)

Edit: Another thing, we're using Transifex for translations. That means if you put a translation directly in git, it will be overwritten when I next pull the translations from transifex. So just remove the translation here, and add it on Transifex once this is merged.

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Mar 30, 2016

Regarding the error:
You mean the error I got when I tried to stop the service while it was indexing?
That was a runtime-error from the syncthing binary I could see in the Android-Log.
I could reproduce that error when I tried to stop the service with a call to stopService() before all my three folders were indexed.
When I waited long enough, I could stop the service without an error.
So that method was not failsafe and I looked for an alternative.

Regarding Automate:
Sorry - documentation was/is missing.
My flow only wraps the creation of the intents. This could be a flow that could be presented to the Automate-Users to simplify the interface. They can call it using one of the 2 entry-points and don't have to know about the exact values of the parameters of the intent. Then the rest is up to the phantasy of the user (so i omitted the part that checks the WIFI. Could as well be the battery state or time of day to control the service ...).
The flow has 2 entry-points - one for activation one for deactivation of the service. Checking both items at the same time makes no sense. I could have created 2 separate flows, but like this the list of flows becomes not so messy. When you select a flow as a subroutine, you can only chose one entry-point.
(I consider the option to chose 2 or more starting-points as a bug - will report that there ;-) )

I don't get what you mean with "Syncthing is disabled" dialog.
When I stop the Syncthing-Service, the ongoing notification in the statusbar disappears and the device gets inactive on the device-lists of the other devices. No dialog is popping up for my devices.

Regarding the translations:
I am new to Transifex - will take a closer look now. ;-)

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Mar 30, 2016

I have merged the latest commits into my version and am testing it on my devices (Galaxy Nexus - Android 4.4, Nexus 7 (2012) - Android 5.1.1 (Cyanogen), Nexus 7 (2013) - Android 6, Nexus 5x - Android N Developer Preview.
The test of the first three started this noon, on the 5x I developed. So far it looks good. Only on the 5x I had had sporadic crashes. (One per day or so) but they did not seem to be related to starting or stopping the service. Syncthing crashed while the device was pausing or doing other things.
How to investigate this? Is there an option to switch on permanent logging? At the moment it seems that I have to manually collect the logs after a crrash? (Maybe because it is a debug-version?)

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Mar 30, 2016

If you get an error on stopService(), that seems to be a bug. Do you still have the log for that by any chance? If not, I'll look into it later. I'd rather fix the root issue than use this ugly hack of using startService to stop the service ;)

For Automate, that means I can just fire the start/stop intent by checking the correct item and pressing ok? That's totally fine for my testing then.

I'm not sure if devices keep a history of their logs. Best leave them connected to a PC with adb logcat running, or use a logcat app on the phone.

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Mar 30, 2016

For your convenience I added my first attempt to stop via stopService. Now there are two methods to stop the Syncthing-Service.
The "stop" value ot the "sycnthingservice" parameter still points to the method to shutdown via startService()
The "stop2" value points directly to stopService()

I also updated (and renamed ;-) ) the Syncthing.flo for automate. Now you have one option for start and 2 options to stop the service. Simply chose one and press OK to issue that command.

Currently strange things seem to happen if I call stopService too early. I had the case that I could not start the service after that. Apparently the service wasn't completely shut down and I got messages like
"Failed to call Rest API at https://127.0.0.1:8384/rest/events?since=0&limit=1"
in the log. Could only be cured by starting the Syncthing-App.
The other method to stop the service via startService() seems to work OK.

Syncthing.flo.zip

Regarding the logs:
Difficult if the crash is rare. I saw them when I switched on my phone after a while or suddenly popping up when I was doing something else. But at the moment no further ones since yesterday.

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 1, 2016

OK I just tried it and it seems to work perfectly. I like how Syncthing is started when you open it, and then automatically stopped when you close it, if the stop intent was last sent :)

Using stopService worked fine, I'd say we go with that and fix the crash if we get a log of that.

public class AppConfigReceiver extends BroadcastReceiver {
private static final int ID_NOTIFICATION_BACKGROUND_ACTIVE = 2;

public static final String ACTION_SYNCTHINGSERVIVE = "syncthingservice";

This comment has been minimized.

@Nutomic

Nutomic Apr 1, 2016

Member

You're using the action/extras kinda weird imo. Why not just use ACTION_START and ACTION_STOP? There's no need for extras here.

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 1, 2016

Author

My Idea behind this is to have a single entry-point to configure the app. If sometimes later other options are added, you can configure more than one property with one intent.

This comment has been minimized.

@Nutomic

Nutomic Apr 1, 2016

Member

I generally don't like the idea of writing code for potential later changes (in the end, it never gets used that way and is just needlessly complicated). And I don't see any need to start/stop Syncthing and change settings in one intent.

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 1, 2016

Author

A thing that is in the back of my head would be the following:
I use Syncthing also for backup of large folders. So it would be nice to be able to disable synchronisation of some shares if I am "on the road" only enabling them when I am at home and charging - or something like this.
Those settings for the folders would fit perfectly into my current pattern. You could then pass a collection of folders to be enabled/disabled in the extras as well as a command to start or stop the service. Or maybe a restartService command might be handy in some cases?

IMHO this would be a perfect place to interface the app ... all external configuration-changes would then come through this receiver ...

If we limit the function to only starting and stopping the service then the Receiver also should be renamed to reflect this "specialized" behaviour. Something like "conrolSyncthingService" ...

This comment has been minimized.

@Nutomic

Nutomic Apr 1, 2016

Member

But the current intents start/stop Syncthing, so you can't pause/unpause folders.

I'd rather use a seperate intent action for that, maybe PAUSE_FOLDER, with parameters folder IDs and pause state.

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 1, 2016

Author

Yes, it could be done that way.
But if the actions are too small, more than one intent might be needed to perform a certain task. Perform a configuration and then restart the service would need 2 intents - resulting in starting and stopping the service 2 times.
For performance-reasons one intent would be better - but then more than one action would have to be configurable ... which is why i came to that solution to deliver my commands in the extras of the intent.

This comment has been minimized.

@Nutomic

Nutomic Apr 2, 2016

Member

But you don't really want to start or stop Syncthing after a settings change, you want to restart it. So we could just always restart after receiving a settings change, or use a boolean extra DO_RESTART.

This comment has been minimized.

@veniosg

veniosg Apr 2, 2016

Contributor

I'd argue that "SyncthingService" is not really an action. It should at the very least contain a verb, especially if it's a public API.
It also has a typo in the name. Furthermore, it'd be wise to follow common conventions such as keeping the value namespaced, e.g. "android.intent.action.VIEW", so syncthing's actions should be "com.nutomic.syncthingandroid.action.(ACTION_NAME)"

.setContentTitle(context.getText(context.getApplicationInfo().labelRes))
.setSmallIcon(context.getApplicationInfo().icon)
.setAutoCancel(true)
.setContentIntent(PendingIntent.getActivity(context, 0, new Intent(context, MainActivity.class), PendingIntent.FLAG_UPDATE_CURRENT));

This comment has been minimized.

@Nutomic

Nutomic Apr 1, 2016

Member

The icon should be R.drawable.ic_stat_notify. setWhen() shouldn't be necessary (I think it's automatically set to the current time). setAutoCancel() is true by default. Do you see any effect on your device for setTicker()/setStyle()?

Basically, just set the things that are necessary, to keep it simple ;)

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 1, 2016

Author

I just copied and pasted a piece of code I used for my other notifications.

SetTicker used to show a text when the notification first was displayed. This behaviour was changed later. But it still seems to be useful:
http://stackoverflow.com/questions/29725102/is-notification-builder-setticker-still-useful-in-android-5-and-above

Regarding setWhen and setAutocancel:
The documentation does not mention the default behaviour, so I explicitly did set those values to be on the safe side.
But I have no problem to remove those calls.

This comment has been minimized.

@Nutomic

Nutomic Apr 1, 2016

Member

If never used setWhen/setAutoCancel, and I'd rather keep it simple. There's probably no harm in setTicker though.

@@ -467,4 +467,5 @@ Please report any problems you encounter via Github.</string>
<!-- Format string for folder size, eg "500 MiB / 1 GiB" -->
<string name="folder_size_format">%1$s / %2$s</string>

<string name="appconfig_receiver_background_enabled">Stopping SyncThingService is not supported when running in background enabled.</string>

This comment has been minimized.

@Nutomic

Nutomic Apr 1, 2016

Member

As I said, it's Syncthing without capital T :p

And I would just write "Stopping Syncthing is not supported", no need to confuse people what a "service" is.

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 1, 2016

Author

OK, you are the boss ;-)

Changed to: "Stopping Syncthing is not supported when running in background enabled."

@@ -286,4 +286,5 @@ Bitte auftretenden Probleme via Github melden.</string>
<string name="state_unknown">Unbekannt</string>
<!--Format string for folder size, eg "500 MiB / 1 GiB"-->
<string name="folder_size_format">%1$s / %2$s</string>
<string name="appconfig_receiver_background_enabled">Das Anhalten des SyncThingServices ist bei aktiviertem Hintergrunddienst nicht möglich.</string>

This comment has been minimized.

@Nutomic

Nutomic Apr 1, 2016

Member

Please remove this and submit the translation via Transifex later ;)

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 1, 2016

Author

Removed

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 1, 2016

I got this exception from testStartStopSyncthingServiceForeground. Is that the one you meant?

java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String java.lang.String.intern()' on a null object reference
at android.os.Parcel.readException(Parcel.java:1552)
at android.os.Parcel.readException(Parcel.java:1499)
at android.app.ActivityManagerProxy.stopService(ActivityManagerNative.java:3462)
at android.app.ContextImpl.stopServiceCommon(ContextImpl.java:1740)
at android.app.ContextImpl.stopService(ContextImpl.java:1699)
at android.content.ContextWrapper.stopService(ContextWrapper.java:521)
at com.nutomic.syncthingandroid.syncthing.AppConfigReceiver.onReceive(AppConfigReceiver.java:63)
at com.nutomic.syncthingandroid.test.syncthing.AppConfigReceiverTest.testStartStopSyncthingServiceForeground(AppConfigReceiverTest.java:98)
at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191)
at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176)
at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555)
at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1853)

mReceiver.onReceive(mContext, intent);
assertEquals("Start SyncthingService Background", 1, mContext.getReceivedIntents().size());
assertEquals("Start SyncthingService Background", "com.nutomic.syncthingandroid.syncthing.SyncthingService", mContext.getReceivedIntents().get(0).getComponent().getClassName());

This comment has been minimized.

@Nutomic

Nutomic Apr 1, 2016

Member

You can just use SyncthingService.class.getName() instead of writing down the ful class name.

Also, I didn't even know you could add a message to the assert like that. Prett neat!

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 1, 2016

Author

Changed that.

We always learn new things. ;-)

// so an exception is an indicator of success in this case.
try {
mReceiver.onReceive(mContext, intent);
throw new Exception("Stop SyncthingService Background - didn't fail to create the notification");

This comment has been minimized.

@Nutomic

Nutomic Apr 1, 2016

Member

Assert.fail() woud be clearer here.

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 1, 2016

Author

Agreed

try {
mReceiver.onReceive(mContext, intent);
throw new Exception("Stop SyncthingService Background - didn't fail to create the notification");
} catch (Exception e) {

This comment has been minimized.

@Nutomic

Nutomic Apr 1, 2016

Member

Can you catch the exact exception that is thrown? This would actually catch the exception you were throwing above, or also any assert.

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 1, 2016

Author

You are perfectly right! My fault!

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 1, 2016

Your tests seem kind of long. Can you split them up so each one has around 10 lines?

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 1, 2016

As the tests do test only a mocked service, I could split testing start and stop to separate tests. (Otherwise I would have to start a service in order to stop it - which was the pattern I used before)

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 1, 2016

Regarding the crashes:
On my current test device I have 3 shared with 3.x GB of data.
So in the log I can see 3 times a message like "Ready to synchronize "

I I issue the stopService before I got those three ready-messages I get the following crash:

04-02 01:03:53.542 8637-8637/com.nutomic.syncthingandroid.debug D/EventProcessor: Shutdown event processor.
04-02 01:03:53.544 8637-8637/com.nutomic.syncthingandroid.debug I/SyncthingService: Starting syncthing according to current state and preferences
04-02 01:03:53.549 8637-8637/com.nutomic.syncthingandroid.debug I/ConfigXml: Checking for needed config updates
04-02 01:03:53.740 8637-10455/com.nutomic.syncthingandroid.debug I/SyncthingNativeCode: [OKYFF] 23:03:53 INFO: syncthing v0.12.21 "Beryllium Bedbug" (go1.6 linux-arm) unknown-user@Colossus 2016-03-23 08:26:04 UTC
04-02 01:03:53.742 8637-10455/com.nutomic.syncthingandroid.debug I/SyncthingNativeCode: [OKYFF] 23:03:53 INFO: My ID: OKYFFXC-EPZ6WT6-MMWMTKJ-5EDUF47-2C7SE2I-UO22YQW-5DK36GM-45LIFQY
04-02 01:03:54.113 8637-10455/com.nutomic.syncthingandroid.debug I/SyncthingNativeCode: [OKYFF] 23:03:54 INFO: Single thread hash performance is ~13 MB/s
04-02 01:03:54.778 8637-10481/com.nutomic.syncthingandroid.debug W/SyncthingNativeCode: ionice: exec 10454: Permission denied
04-02 01:03:54.780 8637-10457/com.nutomic.syncthingandroid.debug I/SyncthingRunnableIoNice: ionice performed on libsyncthing.so
04-02 01:03:54.781 8637-10457/com.nutomic.syncthingandroid.debug E/SyncthingRunnableIoNice: Failed to set ionice 127
04-02 01:03:54.784 8637-10481/com.nutomic.syncthingandroid.debug W/SyncthingRunnable: Failed to read Syncthing's command line output
                                                                                      java.io.InterruptedIOException: read interrupted
                                                                                          at libcore.io.Posix.readBytes(Native Method)
                                                                                          at libcore.io.Posix.read(Posix.java:169)
                                                                                          at libcore.io.BlockGuardOs.read(BlockGuardOs.java:231)
                                                                                          at libcore.io.IoBridge.read(IoBridge.java:471)
                                                                                          at java.io.FileInputStream.read(FileInputStream.java:252)
                                                                                          at java.io.BufferedInputStream.read1(BufferedInputStream.java:273)
                                                                                          at java.io.BufferedInputStream.read(BufferedInputStream.java:334)
                                                                                          at sun.nio.cs.StreamDecoder.readBytes(StreamDecoder.java:287)
                                                                                          at sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:350)
                                                                                          at sun.nio.cs.StreamDecoder.read(StreamDecoder.java:179)
                                                                                          at java.io.InputStreamReader.read(InputStreamReader.java:184)
                                                                                          at java.io.BufferedReader.fill(BufferedReader.java:165)
                                                                                          at java.io.BufferedReader.readLine(BufferedReader.java:328)
                                                                                          at java.io.BufferedReader.readLine(BufferedReader.java:393)
                                                                                          at com.nutomic.syncthingandroid.syncthing.SyncthingRunnable$2.run(SyncthingRunnable.java:307)
                                                                                          at java.lang.Thread.run(Thread.java:803)
04-02 01:03:57.688 8637-8637/com.nutomic.syncthingandroid.debug I/SyncthingService: Shutting down service
04-02 01:03:57.690 8637-8637/com.nutomic.syncthingandroid.debug D/EventProcessor: Shutdown event processor.
04-02 01:03:57.784 8637-10455/com.nutomic.syncthingandroid.debug I/SyncthingNativeCode: [OKYFF] 23:03:57 OK: Ready to synchronize Leo-DCIM (read-write)
04-02 01:03:58.873 8637-8637/com.nutomic.syncthingandroid.debug I/SyncthingRunnableKill: Killed Syncthing process 10454
04-02 01:04:02.053 8637-8637/com.nutomic.syncthingandroid.debug I/SyncthingRunnableKill: Killed Syncthing process 10454
04-02 01:04:02.088 8637-10450/com.nutomic.syncthingandroid.debug E/AndroidRuntime: FATAL EXCEPTION: Thread-9
                                                                                   Process: com.nutomic.syncthingandroid.debug, PID: 8637
                                                                                   java.lang.RuntimeException: Syncthing binary crashed with error code 137, output:
                                                                                   [OKYFF] 23:03:53 INFO: syncthing v0.12.21 "Beryllium Bedbug" (go1.6 linux-arm) unknown-user@Colossus 2016-03-23 08:26:04 UTC
                                                                                   [OKYFF] 23:03:53 INFO: My ID: OKYFFXC-EPZ6WT6-MMWMTKJ-5EDUF47-2C7SE2I-UO22YQW-5DK36GM-45LIFQY
                                                                                   [OKYFF] 23:03:54 INFO: Single thread hash performance is ~13 MB/s
                                                                                   [OKYFF] 23:03:57 OK: Ready to synchronize Leo-DCIM (read-write)
.setAutoCancel(true)
.setContentIntent(PendingIntent.getActivity(context, 0, new Intent(context, MainActivity.class), PendingIntent.FLAG_UPDATE_CURRENT));

if (Build.VERSION.SDK_INT >= 21) {

This comment has been minimized.

@veniosg

veniosg Apr 2, 2016

Contributor

This should be using a constant from Build.VERSION_CODES.

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 2, 2016

Author

Well, even Android Studio itself only gives the numerical value. But agreed, it looks nicer.
Changed

/**
* Intent action to shut down the SyncthingService
*/
public static final String ACTION_SHUTDOWN = "shutdown";

This comment has been minimized.

@veniosg

veniosg Apr 2, 2016

Contributor

Same point about the values of actions as the one above. Naming seems fine for these ones though.

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 3, 2016

Just pushed the new version
After I made friends with the action-parameter (we always can introduce a "bulk"-action later ;-) ) I now changed the AppConfigReceiver to use the action-field.

The changes:

  • Changed the AppConfigReceiver-Intent from actions in intent-extras to actions in the action-property.
  • Renamed the actions according to the naming conventions of Android.
  • Moved the code that handles the service to a private inner static class to isolate that functionality.
  • Adjusted the Instrumentation-Tests
@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 3, 2016

... and the updated version of the Syncthing-Flow for those who test with Automate:

Syncthing.flo.zip

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 4, 2016

The error you had during shutdown was caused by SIGKILL, which we use to stop Syncthing. I've pushed a commit to ignore this exit code, so this should work fine now. That means you can really get rid of the "shutdown via start" code :D

I haven't tried this version again, because I assume it still works. The code looks also fine at a glance. I just noticed you have a test commented out, why is that?

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 4, 2016

There is a TODO entry in the comment ;-)
The current mocked context that I reused for my tests doesn't support the stopService command and crashes. So I delayed the test until it is really needed.

Will test if the crash went away and then switch to the stopService-Variant.

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 4, 2016

Sorry I was kinda lost in all the comments there :p

You can probably just forward the call to the actual context in MockContext (depending on what you need, haven't looked closer yet).

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 19, 2016

Yeah, best squash and force push.

I'm not quite sure how you want to make this "fake" method. We can just handle that in a new PR later.

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 19, 2016

Well, just a method that returns a constant value with a javadoc describing the intended purpose.

So the hook is implemented an can be used, but may be implemented later.
But we also can postpone that.

Am 19. April 2016 17:27:41 MESZ, schrieb Felix Ableitner notifications@github.com:

Yeah, best squash and force push.

I'm not quite sure how you want to make this "fake" method. We can just
handle that in a new PR later.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#599 (comment)

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 19, 2016

Back at keyboard and implemented your requests.
Only 2 items left:

  • Move ID_NOTIFICATION_BACKGROUND_ACTIVE to a place where all application-wide constants can be collected? Would make it easier to introduce new constants if needed in the future.
    Personally I would move such constants to the application-object which has the longest liifetime. Every other class would require to load unnecessary code in some cases. (If I understand the lifetime of the java-objects right.) But an application-object doesn't exist yet.
  • Implement a dummy isSyncthingEnabled() function and query that for the start-service logic?
    If yes - where to put it? I think it should be a static method that can be called from everywhere to any time. But DeviceStateHolder is a BroadcastService. So if it is put there, that class would always have to stay in memory.
    Again the application-object would be a candidate. Or maybe an extra singleton?
    ... but then that might be a candidate for an extra change.
@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 19, 2016

Ah, and before you ask: I reintroduced setAutoCancel at my notification in AppConfigReceiver. Without it, the notification is not cancelled when the user clicks it - which is the way I would like to see it.

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 19, 2016

Moving the notification IDs to a common place should be part of a seperate PR. Where that is doesn't matter, because the IDs are static final, so the containing object doesn't need to be instantiated.

And I don't see the point of having a dummy method in the code. Having such a method definitely makes sense, but we should also do it in a seperate PR.

@@ -1 +1 @@
Subproject commit 668eb7c398c2e8d0b9f7beb319b7789681d7d89f
Subproject commit 6d280e7b6461d407c90f8706e348393c657e995f

This comment has been minimized.

@Nutomic

Nutomic Apr 19, 2016

Member

Ah I missed this, you shouldn't change anything in the submodule.

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 19, 2016

Author

I did not intend to change something there. Besides my changes in the java-classes, I only rebased on your master - could the change come from there?

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 19, 2016

Author

Regarding the static finals: I am not 100% shure if the corresponding class is loaded if you use a static final field of that class. Technically it would not be necessary (if the initial value is a constant), but I did not find a clear reference for that in the web. So I thought defensive and would put the variable to the object that is living the longest fime ;-)

Regarding the dummy method: My thought was, that the later implementation of that dummy method would not change the other code. So a later change would be more focused.
But looking at the whole change it would make no difference - you have to make a change at both places.

So I forget about the dummy method for now.

This comment has been minimized.

@Nutomic

Nutomic Apr 19, 2016

Member

This seems to be because of this accidental change, your rebase didn't take that change for same reason. I reverted that already, so you won't have to do anything (6d280e is correct).

Static fields can be accessed without an object, just with a class. So this isn't a problem. But as I said, that's something for a seperate PR.

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 19, 2016

Did you forget to push your changes?

@sqrt-1764 sqrt-1764 force-pushed the sqrt-1764:appconfig-receiver branch from 7877530 to 06435f6 Apr 19, 2016

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 19, 2016

Just pushed my change.
Not shure what to to about the change in the submodule as I don't know what has caused that change.

The comment in the javadoc to onDestroy is quite long. You may shorten it if you wish ;-)

@@ -75,7 +73,7 @@ public void onReceiveSystemInfo(RestApi.SystemInfo info) {
public void testGetFolders() {
assertNotNull(mApi.getFolders());
}

public void testConvertNotCrashing() {

This comment has been minimized.

@Nutomic

Nutomic Apr 19, 2016

Member

Please revert your changes to this file. Of course the whitespace and extra imports shouldn't be here, but removing that should go into a seperate cleanup commit/PR.

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 20, 2016

Author

Hmm, as expected, I had had conflicts upon rebase and I then accepted the version that came from you without any further editing.

Edit: At least that was what I intended to do.
Apparently I had made a mistake there. I can see it in the Git-History - so I will revert it.

Edit 2:
I think I know what happened: I checked the file within Android-Studio and apparently AS did those changes on its own without me noticing it ...

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 20, 2016

Author

Edit 3:
I just confirmed: Android Studio (at least in Version 2.0) automatically made those changes without notifying me. Simply viewing the file in the editor is enough

case ACTION_STOP:
// Stop the Syncthing-Service
// If alwaysRunInBackground is enabled the service must not be stopped. Instead a
// notification is presented to the user.

This comment has been minimized.

@Nutomic

Nutomic Apr 20, 2016

Member

I'd put this documentation on the ACTION_START/STOP definitions (as Javadoc).

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 20, 2016

Author

Better, agreed

* Either onDestroy is first. Then PollWebGuiAvailableTaskImpl.oonPostExecute waits for onDestroy
* to finish -> mStopScheduled is set and the binary is shut down then.
* Or PollWebGuiAvailableTaskImpl.onPostExecute is first. Then onDestroy waits and then kills
* the now active sycnthing binary on the "old" execution path.

This comment has been minimized.

@Nutomic

Nutomic Apr 20, 2016

Member

Uh this is really long. Can you try to limit yourself to 2 - 3 sentences? Just describe what it does in which case, without describing so many concrete scenarios.

And there's no need to write when this was added, that's what git blame is for. Just describe what the code does now. Also, there are some typos, maybe paste this into LibreOffice first?

This comment has been minimized.

@sqrt-1764

sqrt-1764 Apr 20, 2016

Author

Too short?

The native binary crashes if stopped before it is fully active. In that case signal the stop request to PollWebGuiAvailableTaskImpl that is active in that situation and terminate the service there.

This comment has been minimized.

@Nutomic

Nutomic Apr 20, 2016

Member

Nope that sounds good to me.

Matthias Leonhardt
Added a IntentService to receive Broadcast-Intents to remotely contro…
…l / configure the app.

MainActivity: Moved binding-functions to onPause() and onResume() so that the SyncThingService is only bound to the activity if the activity is active.

New class AppConfigReceiver:
Support start and stop of the SyncThingService
- restarting a running service again should not be an issue
- stop service only if "always run in background"-mode is disabled. Otherwise show a notification indicating this.

Instrumentation-tests:
- Added tests for AppConfigReceiver
- Extended MockContext to also consume stopService commands.
- testGetReadableTransferRate: Apparently the return-values have changed a bit. Adjusted the asserts to the current return-values.

SycthingService:
Added code for thread-safety in case the service still starting when it should be stopped. Then PollWebGuiAvailableTaskImpl is active and waits for the Synthing-API to become active. So that and onDestroy have to be synchronized.
Added a stopSelf() in PollWebGuiAvailableTaskImpl.onPostExecute() in case mStopScheduled was active.

Commented my change in the javadoc at onDestroy. Put a reference to that comment to .onPostExecution()

@sqrt-1764 sqrt-1764 force-pushed the sqrt-1764:appconfig-receiver branch from 06435f6 to a812837 Apr 20, 2016

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 20, 2016

Pushed the updated version right now

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 24, 2016

Thanks, I finally merged it!

There was still the conflit with the submodule which I had to fix. That's why the commit might look weird on Github, and why Github doesn't recognize this PR as merged :/

@Nutomic Nutomic closed this Apr 24, 2016

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 26, 2016

OK - as long as my code is merged ;-)

I just joined Transifex to add the translation for my string. But the server has errors and displas no data for me. But apparently someone already added the translation as the untranslated count went from 2 to 0 this afternoon ...

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 26, 2016

Do you want to write some documentation on this? A wiki article would probably be good.

Also, ./gradlew lint is complaining because the "Receiver does not require permission". Documentation for receiver and permissions. Can we add that, or will it break Tasker etc?

@capi

This comment has been minimized.

Copy link
Member

capi commented Apr 26, 2016

I suppose it'd break Tasker, since Tasker won't be holding the permission in question.

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 26, 2016

Well, I made the code, so I also should provide the documentation. ;-)
I think a few sentences and maybe an example would be enough. People who want to use this feature should already know what an intent is ...
I also will publish my flow for automate in the automatie community once the feature is published ... so there will be a working example for the rest of the users.

Regarding the permission:
If we define a permission for the receiver, every app that wants to send those intents the receiver has to hold that permission. AFAIK (correct me if I'm wrong) that means that this permission has to be listed in the manifest of that app under "uses-permission" which will make the feature unusable for Tasker etc.
It is intended as a public service - and therefore must not be protected by a permission, so I think that lint-message has to be suppressed.

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 26, 2016

Alright, I'll disable the warning then.

Oh and it would be cool if you could do the integration of AppConfigReceiver and DeviceStateHolder when you get to it :)

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 26, 2016

You mean writing this wiki-article?
(I currently made no change to DeviceStateHolder)

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 26, 2016

No I mean in the code, with the isSyncthingEnabled() method we talked about.

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 26, 2016

Ah - will look into this soon.

@sqrt-1764

This comment has been minimized.

Copy link
Author

sqrt-1764 commented Apr 28, 2016

I just created an article in the wiki for this.
I also tested the behavior of Syncthing in case "always run in background" is enabled. In this case for me the intents are ignored. Will say the configured behavior took precedence.
This seems to be the behavior you wanted. So is this isSynctingEnabled() method currently needed? If yes - where shall we discuss the details? I think a new thread would be appropriate then ...

@Nutomic

This comment has been minimized.

Copy link
Member

Nutomic commented Apr 29, 2016

IRC would be good, are you on there? #syncthing on Freenode.

@syncthing syncthing locked and limited conversation to collaborators Nov 9, 2018

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