F-Droid / Client

Thanks for the clear explanation of what is going on and indeed the MR. It makes sense and I will probably merge soon - just hoping you could respond to my thoughts below.

From memory, I added the calls to onChange() to address the fact that ContentObservers were taking quite some time (e.g. in the order of several seconds) before triggering, thus leaving a weird period where the app looks uninstalled, and there is no "run app" button. Obviously I hadn't thought through the consequences of the activity being killed by Android. Can you think of a way to make it so that the activity is updated as soon as the package manager returns from installing/uninstalling? Perhaps keep the manual invocation of onChange(), but with a guard condition and a comment explaining that it is there for the case where the observers don't get notified for a period of time?

Any thoughts would be welcomed.

As a side note (unrelated to your change, but related to the bug you have fixed), I've always disliked maintaining too much class-wide state that is set, e.g. during onResume(). It means that you have to make more assumptions about the order in which methods are invoked. One way I've started to address this is to use the @nullable and @NonNull attributes from the support library. In this case, it would be good to have the property myAppObserver nullable, but a getter non-nullable - and then only ever access it from the getter:

@Nullable
private ... myAppObserver;

@NonNull
private ... getMyAppObserver() {
  if (myAppObserver == null) {
    myAppObserver = new ...
  }
  return myAppObserver;
}

Then the compiler will check that all calls are guaranteed not to produce NPE.

The problem in this example though, is that it requires a Handler which gets bound to the thread it is created on. Therefore, we are back to making assumptions about the first time getMyAppObserver() is called, and which thread it was invoked from, which is probably worse than your fix.

Added 4 new commits:

• 4e2ab4c0 - Call super.onPause() first
• cda80f5d - Register ContentObserver when activity is started
• 578084ff - Revert "Do not manually call onChange() (fix NPE)"
• 4f514fb3 - Externalize application changed actions

mentioned in commit 578084ff

From memory, I added the calls to onChange() to address the fact that ContentObservers were taking quite some time (e.g. in the order of several seconds) before triggering, thus leaving a weird period where the app looks uninstalled, and there is no "run app" button.

On my tests, it is received immediately. But you are right, it makes sense to update the view as soon as possible, since there is no guarantee about the broadcast delay.

I provide two independant fixes that keep the update as soon as onActivityResult() is called: cda80f5 and eef1e0a.

Each one is sufficient to fix the problem, but IMO they should be applied both (i.e. just merge the full MR) for reasons independant of this bug. Please read the commits comments.

Note: this conflicts with my other current MR. When one is merged, I'll rebase the other resolving the conflicts.

Added 1 new commit:

• eef1e0a4 - Externalize application changed actions

Awesome, code review looks great. I'm downloading the latest SDK to build and test it before merging, but my internet is being silly and slow right now. Once that is done, either tonight or tomorrow morning I'll merge this in. Then I'll try and find some time later in the weekend for the other MR, but on a quick glance it looked good. Thanks again.

Status changed to merged

mentioned in commit d7c19c76