Collect: Editing a form instance from an external app shows blank layout

[batkinson]

Software and hardware versions

Unclear whether it is android version dependent. Originally reported from field operators using GalaxyTab 5 (android 5). I reproduced the issue on android 4.4.4 on the latest production release of Collect (v1.4.16).

Problem description

When launching an existing form instance from an external app, using a form instance uri and ACTION_EDIT, the form does not load as in previous versions. Instead a layout showing only the action bar titled "Loading Form" is shown. The contents of the layout are empty as shown in the image below:

Contrast this with the previous behavior for the same form:

I tracked this back to the commit where the original behavior broke. It broke in commit 68dc498516. This means that users running Collect v1.4.15 and later are affected.

Steps to reproduce the problem

Launch form editing from an external app using an existing form instance URI and ACTION_EDIT.

Expected behavior

The form instance loads as it did pre v.1.4.15 (as shown in the images above).

Other information

Users in the field were waiting for the form to load, thinking it was simply slow. You can "work around" the issue by tapping home and resuming the app that launched the ODK form (resuming the activity). Alternatively, you can just swipe left.

[lognaturel]

Ah, interesting, I was wrong and it has nothing to do with removing the instruction screen. It's just a good old regression! @grzesiek2010 you might have some ideas here.

@batkinson are your external app and your form things you can post publicly? I don't even think we have a good external app example around for testing.

[batkinson]

So, all of the code and forms are public, but I suspect it would be easier to code up a simple activity and simple form that go through the pain of configuring things. I'll see if I can post minimial examples.

[grzesiek2010]

I'm going to bed :) I'll look at this tomorrow but... isn't it the same https://github.com/opendatakit/collect/issues/497 ?

[batkinson]

Yeah, reading the description it seems that the end result is the same. However, looking at the code, I don't think this is solely related to unsaved changes - unless that's just the name for very specific changes introduced since v1.4.14.

[batkinson]

I will test out the latest from master and report back.

[lognaturel]

Riiight, I'm sorry, I assumed #497 was due to the instructions screen removal but I don't actually know that. It may very well be the same issue. Looking forward to finding out what you discover, @batkinson!

[batkinson]

Ok, so I don't think this is the same as #497 unless it's not truly resolved yet. I built and ran the latest from master(2c50c692a85b) and the behavior is exactly the same as the latest production release.

[grzesiek2010]

I didn't reproduce this issue but I'm prety sure it's due to our new modes and the problem might be here
If I understand you start FormEntryActivity directly from your app so then you should use one of the mentioned modes (in your case "EDIT_SAVED").
@lognaturel maybe we should check if the mode is null like in this case and then start edit saved mode by default? We missed that case. Or we should docummate this case to inform our users.

@batkinson if you can try to add something like we do here when you start FormEntryActivity and let us know if it works.

[batkinson]

Thanks @grzesiek2010, I will do that to help provide more information about the shape of the defect. For what it's worth, I think this is an outright defect. This breaks documented behavior and requires existing apps that use published APIs to be updated to continue to work with Collect. To give you an idea of what that causes: groups that use Collect will be forced to fork if they can't rely on APIs to be stable. If this activity was only internal (no public intent-filter allowing implicit launching), it wouldn't matter that the semantics changed, but that's not the case here.

[batkinson]

I confirmed that adding the extra patches this back up. So, when creating the intent it will go from something like:

    public static Intent launchIntent(Uri formUri) {
        return new Intent(Intent.ACTION_EDIT, formUri);
    }

To:

    public static Intent editIntent(Uri formUri) {
        Intent intent = new Intent(Intent.ACTION_EDIT, formUri);
        intent.putExtra("formMode", "editSaved");
        return intent;
    }
}

The magic string values are a hint that this isn't how android intended apps to cooperate with implicit Intents. Effectively, it spreads implementation coupling to independent applications.

[lognaturel]

Apologies, @batkinson, we had talked about needing to refactor not to use the constant hack for editing vs. viewing forms but I didn't consider that it had external effects. That's my lack of Android experience showing through. 😔

@grzesiek2010 from an external perspective, Intent(Intent.ACTION_EDIT, formUri) should work as it did before and it should also be possible to call Intent(Intent.ACTION_VIEW, formUri). Internally, you should be able to introduce a superclass for shared behavior. Is that something you can take on? Let us know if you have questions on how to approach it.

@batkinson will you need an emergency release for this? Next official one is planned for 3/26.

[batkinson]

@lognaturel a maintenance release of Collect is not necessary for us. I am publishing a fix as I write this and it should be safe for older deployments. Adding the extra shouldn't affect those running older versions of Collect. However, restoring a sane default behavior for others with apps integrating with Collect would be appreciated by others, I am sure. I have the luxury of a fairly tight control over device deployments, I know others aren't necessarily so lucky.

[lognaturel]

Ok, awesome! I'm glad to hear you can recover quickly. Yes, absolutely, we'll make sure the fix is in the next release. We'll also hopefully have actual documentation on intents soon (#74) so we don't forget about them.

Come help us with code review? 😉

[batkinson]

Come help us with code review? 😉

Haha, yes, thank you for the gentle reminder. I probably would have caught this one. :smile:

[batkinson]

So, @grzesiek2010 as you can see from the recent activity I pushed a branch to my fork that attempts to fix this issue. The approach I took was to make the old behavior the default, so that if no extra was provided, it works as it used to. I also have a hack of an espresso test there to confirm it works. I'm reluctant to submit this without some sort of test coverage for the things I'm touching, but we need to get a decent test harness working before that's realistic. Would you mind taking a look at c90028e0e and maybe trying it out just to see whether there's anything obvious I got wrong? If it's going to take a ton of work to get a harness built, we could split the effort from the fix.

[grzesiek2010]

@batkinson I reviewed your changes and I think we can completely get rid of EDIT_SAVED mode and then use only somethink like:

String formMode = reqIntent.getStringExtra(FORM_MODE);
if (formMode == null) {

} else if (VIEW_SENT.equalsIgnoreCase(formMode)) {

}

so as I understand it you want to fix this issue for ODK too right? Let me know because otherwise I have to do that.

[batkinson]

Hi @grzesiek2010, thanks for the feedback. I actually noticed that too, but I didn't want to remove the constant altogether myself. As for fixing the issues for ODK, yes, it was my intent to fix this issue. However, I didn't want to submit it as a pull request quite yet. I hacked it together and just wanted to check that it made sense to you. Also, the test isn't usable as-is since it required having my local data. There's some work to do to setup the test so it's portable and reliable. Thanks again,

[batkinson]

I think we can completely get rid of EDIT_SAVED mode

So unfortunately, since there is now software in the wild that is setting the mode to work properly, removing the constant and just interpreting null to mean 'edit' will break them again. Such software will have to be changed to not set a value for FORM_MODE to work properly again.

[grzesiek2010]

So we can reverse the order:

String formMode = reqIntent.getStringExtra(FORM_MODE);
if (VIEW_SENT.equalsIgnoreCase(formMode)) {
} else {
}

it should be ok then right?

[batkinson]

Haha, yes, but now we're making this even more fragile if/when you decide to add another mode. In my opinion, we're stuck with both modes. You're better off having the conditions be explicit than trying to get clever to remove them. This is especially true, since now they are actually being used outside of Collect.

Unfortunately, this is the problem with trying to support external integration. Once you publish an API you are stuck supporting the interface or choosing to break people to fix it. More thought needs to be put into these types of decisions, but I understand that external integrations weren't on your minds when making this call.

[batkinson]

I abandoned the previous code and made another attempt with what I've now learned. I submitted it as a PR #555. It attempts to make the minimal changes required to keep backwards compatibility while also maintaining current internal semantics. The test isn't in the PR since it was dependent on local test data and a quality test harness is not quite within reach.

[lognaturel]

@batkinson Thanks for the fruitful discussion around this. As we have discussed, we will treat this as a special case and revert it immediately for the next release.

Do you think you will be able to address it before Wednesday's code freeze or would you like someone else to take it on?

[batkinson]

Hi @lognaturel, there was quite a bit of activity in the files that I was working in. There were just too many conflicts, which is why I ended up just closing the PR I submitted previously. If someone that has been reworking the related files is interested, don't let me hold things up.