Site creation service by hypest · Pull Request #6825 · wordpress-mobile/WordPress-Android

hypest · 2017-11-03T12:33:43Z

Introduces a Service that performs the site creation steps. A simple "placeholder" UI is used for interacting with the Service and will be expanded to conform to a new design for a site-creation wizard.

The Service uses the also newly introduced AutoForeground helper to seamlessly foreground/background the Service. If the app gets backgrounded while the service is still running, a statusbar notification appears to monitor the progress. The notification gets hidden if the app resumes while the service is still running.

This PR targets the dedicated feature branch.

Depends on #6823 to be merged to develop but in the meantime, it already includes the commit needed.

To test:

Edit your gradle.properties file and define the 4 strings needed for the new site creation. See

WordPress-Android/WordPress/src/main/java/org/wordpress/android/ui/accounts/signup/SiteCreatingFragment.java

Lines 82 to 85 in 86ffba9

    
           WordPress.getBuildConfigString(getActivity(), "DEBUG_DOTCOM_NEW_SITE_TITLE"), 
        
           WordPress.getBuildConfigString(getActivity(), "DEBUG_DOTCOM_NEW_SITE_TAGLINE"), 
        
           WordPress.getBuildConfigString(getActivity(), "DEBUG_DOTCOM_NEW_SITE_SLUG"), 
        
           WordPress.getBuildConfigString(getActivity(), "DEBUG_DOTCOM_NEW_SITE_THEME"));

for the field names required. Feel free to use intergalactic-2 for the theme name/id.

Compile the app and run it
With the app logged in to WPCOM, navigate to MySite, "switch" to a different site and use the "+" button on the site picker's toolbar to start the creation process.
Feel free to background the app and resume it. Notice the statusbar notification appearing, updating or disappearing.
When the app UI is in the foreground, the status text below the illustration should update while the process is advancing
Upon success, the new site should be created, it's tagline set and its theme properly set as well. Use the web to verify.

This way, SiteSettingsInterface can be used by Services as well.

The fragment attaches, detaches and spawns the new-site creation Service, updating its UI to reflect the process's stage.

mzorz · 2017-11-08T20:42:24Z

The code looks simply great @hypest 👏 , but I have some questions to ask.

The code itself is brilliant, and the solution is very clever, but (a) it seems not to be a piece of code that is straight-forward to understand and follow (not because of it not being clear, but on the contrary, because of the complexity and levels of abstraction this nice piece you created has) and
(b) not sure we’re trying to solve the right problem. After a few minutes of reading and surfing the code I realised I’d better go back to position zero and ask: what does this do, and why?.

The very purpose of AutoForeground is, as far as I understand, summarised in these 2 points:

make sure a Service is sent to the Foreground when the app is in the background
make sure to stop a foreground service when the application itself (and this actually means, some Activity) comes back to the foreground.

While this might be correct, I’d ask: do we need to do this? Why not having a foreground notification set since the beginning? I think all we need is for the process with all its stages to be kept together, which is already handled by the Service. Thus, just setting a foreground notification for the Service would remove all the complexity. Maybe we don’t like having a foreground notification while the app itself is on the foreground? Is this because of a UX concern? Do we think a user would actively send the app to the background while a site creation is taking place? Certainly getting a call or some other app interrupting in the middle of it would probably be more likely than that, but in that case, just setting the foreground notification for the service right away when starting it seems to be sufficient.

I hope I’m not falling into some trap here 😝 , but if there are reasons to not have the foreground notification for as long as the service creation process last (with all its stages), I’m curious to learn which those are.

Also, an open question: is there a risk in between passing from foreground to background and viceversa? how sure are we about it?

Suppose we went on with the AutoForeground mechanism. The Messaging/WeakHandler structure has its own complexity (I’m thinking maintainability here), we have a quite extensive use of EventBus for communicating things between objects in our codebase already, and if we were concerned about the possible risks of leaks / instances not receiving events when needed (which is a possibility in this case between a Service and Activity, as it is known for Fragments/Activities as well) then the more native LocalBroadcastManager is designed for handling that specifically an would be suitable for this case.
Why didn’t we use EventBus or LocalBroadcastManager (https://developer.android.com/reference/android/support/v4/content/LocalBroadcastManager.html)?
(side note on this: did a quick search on old commits and I can see we were using LocalBroadcastManager before, back in 2015, and replaced with EventBus everywhere, see this https://github.com/wordpress-mobile/WordPress-Android/search?p=2&q=Broadcast&type=Commits&utf8=%E2%9C%93). I think this particular case would be better handled with LocalBroadcastManger though).
This is probably a nit given considerations above, but something to consider for the future in the case we go ahead and use AutoForeground: then if we’d want to reuse the AutoForeground scheme (that is, use it for something different than the Site creation Service), it should have a notificationId generator/provider (and keep track of each id), or the other way around if we don’t want to burden it with this: we could enable it to be told/passed which notification Ids to use for progress, success, and failure. Otherwise the app could be reusing the same notification IDs and these will collide.

hypest · 2017-11-09T13:26:57Z

Thanks for the thorough feedback @mzorz , I appreciate it!

Why not having a foreground notification set since the beginning?

That's actually a design decision we have made in the project: have the notification only when the app is in the background. The assumption here is that the app will actually display the operation's progress on its current UI so the notification is redundant.

In practice, the implementation depends on the registered listeners count. That means the app could be displaying a screen irrelevant to the Service and the notification would show up if no listener is in place.

Do we think a user would actively send the app to the background while a site creation is taking place?

Perhaps, especially if the device is on a slow network connection. It's a basic assumption in Android anyway but beyond that, the implementation here tries to become usable elsewhere too. See #6700 for example. A next goal is to port the Login procedure to a AutoForeground Service too. Maybe even in other places, like media upload which we know for sure that it takes long to complete.

By the way, I'd expect this solution to mature and include an option to always show the notification. The key with AutoForeground is that neither the Service clients nor the Service itself need to worry about the notification... it will surely appear if no one is actively listening while at the same time the system won't limit the Service's execution in Android 8.0.

is there a risk in between passing from foreground to background and viceversa? how sure are we about it?

None that I know of. Promoting a background Service to foreground is standard practice (see "Prior to Android 8.0..." paragraph in the docs) and then the API offers the way to demote back to background (see the docs) if killability is not an issue anymore.

Why didn’t we use EventBus or LocalBroadcastManager?

I opted for Messenger API since it's native and designed to be used with Services. See paragraph "Using a Messenger" in the Bound Services docs.

Among other things, notice how the Messenger API can detect when the the listener is stale/dead/misconfigured, allowing the AutoForeground-er to foreground the Service if no listeners are valid:

WordPress-Android/libs/utils/WordPressUtils/src/main/java/org/wordpress/android/util/AutoForeground.java

Lines 150 to 158 in 5a0007a

    
           try { 
        
               mConnectedClients.get(i).send(Message.obtain(null, MSG_CURRENT_STATE, state)); 
        
               validClients++; 
        
           } catch (RemoteException e) { 
        
               // The client is dead.  Remove it from the list; 
        
               // we are going through the list from back to front 
        
               // so this is safe to do inside the loop. 
        
               mConnectedClients.remove(i); 
        
           }

I'm under the impression that we can't do that check nicely with EventBus nor LocalBroadcastManger but I can be wrong.

I think this particular case would be better handled with LocalBroadcastManger though

Quite possibly. I mainly opted for the Messenger because the Service docs mention it and I didn't think EventBus offers the deep IPC I wanted for abstracting the messaging mechanism.

That said, the overall logic of the "AutoForeground" functionality is not too tied with the messaging mechanism so, we can exchange it with EventBus, LocalBroadcastManger or even direct static method calls if we prefer and turns out to be simpler. I'd say that that would be an optimization at this point and so, we can do it in a separate PR.

Otherwise the app could be reusing the same notification IDs and these will collide.

Totally. I was aware of the notification id mess in Android but didn't want to burden this PR with this. This PR is against a feature branch anyway and was planning to tackle that later.

Thanks again for the deep dive in the code @mzorz ! Let me know if the responses above cover the concerns or perhaps more info is needed. ❤️

mzorz · 2017-11-09T17:09:42Z

Thanks for taking the time to answer all these points @hypest ! I really appreciate it and I like how you’re showing me a different perspective, that allows to see potential in the solution in a different way. I’ll come back to this a few times in my comments below.

tl;dr: Being totally honest, I wouldn’t have taken the decision to go ahead and implement this solution, I would have challenged the design decision and I’d be fine with having little UI redundancy but not pay the burden of adding potential issues with a non trivial solution, maintainability being the most important thing I can foresee.

OTOH the solution is written now, so we have it and I’m trying to explore the potential of doing nice stuff with it.

With this in mind, I’ll comment below.

Why not having a foreground notification set since the beginning?

That's actually a design decision we have made in the project: have the notification only when the app is in the background. The assumption here is that the app will actually display the operation's progress on its current UI so the notification is redundant.

I feel at first the impulse to challenge that decision. The balance between having 1 line of code to maintain (just setting the service to foreground til it’s stopped) versus a +771 −49 change that this PR represents seems a bit too much. The decision and assumption as expressed here are debatable to say the least, and I don’t think having the UI redundancy here is in any way harmful when you weigh in the effort put into building this functionality and the possible problems we may (or may not) have for maintaining it in the future.

Design wise, I think it probably makes lots of sense to eliminate any redundancy, but at which cost?. Also it’s offering marginal value (just for this use case, hide/unhide the foreground notification depending on the app being on the foreground or not, I bet most users won’t even notice).

OTOH the solution is still nice, even though I would have decided against pursuing this effort here in the beginning, for the reasons I just outlined. This triggers a side, philosoraptor note: I honestly wonder whether this is an egg/chicken problem in my head: if I tried to always do the less effort possible (in a good sense), then this kind of things wouldn’t be available in the future to be used in new use cases, because the thing wasn’t built in the first place. But at the same time we can’t be coding for “potential future use cases”. This is a tough call.

In practice, the implementation depends on the registered listeners count. That means the app could be displaying a screen irrelevant to the Service and the notification would show up if no listener is in place.

ahhh!. This is nice. I can see the potential this has. But still, it’s a subtle thing, while the complexity of the mechanism behind this is not proportional to the value it’s offering. Is it worth it?
My technical me says “this is beautiful, shut up and merge”. But from a product/project POV I’m like “this is less than ideal, I don’t want code that comes with a ramp up curve for others to modify, specially when it comes to marginal functionality”.

See #6700 for example. A next goal is to port the Login procedure to a AutoForeground Service too. Maybe even in other places, like media upload which we know for sure that it takes long to complete.

I can’t help the big question coming up again each time: for these cases, does showing the foreground notification all the time, make any difference? The 3 cases (login, site creation, media upload) are things that the user triggers, so it is expected they want to know what’s going on with their request. I’m sure they can cope with having the notification there.

Also as a side note, while I haven’t read #6700 in detail, that looks like the “done” event wasn’t caught by the fragment handling the login UI or something alike. Things like events being lost because of configuration changes or the user sending the app to the background, etc could be solved by using sticky events and adding the logic to remove the sticky event once consumed (http://greenrobot.org/eventbus/documentation/configuration/sticky-events/).

The key with AutoForeground is that neither the Service clients nor the Service itself need to worry about the notification... it will surely appear if no one is actively listening while at the same time the system won't limit the Service's execution in Android 8.0.

☝️ I like this idea a lot ❤️. Super smart!

That said, the overall logic of the "AutoForeground" functionality is not too tied with the messaging mechanism so, we can exchange it with EventBus, LocalBroadcastManger or even direct static method calls if we prefer and turns out to be simpler. I'd say that that would be an optimization at this point and so, we can do it in a separate PR.

I'm glad you're suggesting that, as the main key thing that I’m trying to address with my comments here is that the messaging part is not as clear as it would be if we used EventBus (given its extensive use in the app) or LocalBroadcastManager given the similarities in terms of how to use it. I don’t see it as an optimisation that can be made later though, IMO it should be changed before merging because of maintainability as expressed. The solution itself is correct as is (again it’s obviously a nice piece of code that shows how much you know!), but if we leave it there for a later “optimisation”… I’m afraid very few will dare touching it, which is the exact thing I fear from merging it in the first place. Hope that makes sense!

I think the sanity check you mentioned here

WordPress-Android/libs/utils/WordPressUtils/src/main/java/org/wordpress/android/util/AutoForeground.java

Lines 150 to 158 in 5a0007a

    
           try { 
        
               mConnectedClients.get(i).send(Message.obtain(null, MSG_CURRENT_STATE, state)); 
        
               validClients++; 
        
           } catch (RemoteException e) { 
        
               // The client is dead.  Remove it from the list; 
        
               // we are going through the list from back to front 
        
               // so this is safe to do inside the loop. 
        
               mConnectedClients.remove(i); 
        
           }

is not available as is through EventBus or a broadcast message as there’s no way back.
But, we’ve been relying on registering/deregistering objects from the Bus with no major problems, so I guess unregistering can still be safely done in the majority of cases.

In order to give us a path to move forward to, this is what I’d do:

I’d really ask whether the design decision can still be challenged. My preference would be to have 1 line of code to set the foreground service and that’s it.
If (1) is impossible and so we still need to do the hide/unhide behaviour, then this solution is great except that I wouldn’t do it without changing the messaging system for EventBus as a preferred way. Also I’d add some more comments about what the AutoForeground class is for, how it works, and such, to leave it easy for anyone that happens to wonder why we have this there.

Hope that makes sense! 🙇

hypest · 2017-11-09T18:22:20Z

Thanks for the additional response @mzorz ! I'd like to explore option 2 a bit more. I think that hiding the notification when the app is in the foreground is a good UX for the site-creation case this PR is about so, I'll leave it in for now until exploring option 2 so more. Thanks again for the insights!

Note: EventBus doesn't offer a way to listen to registrations/unregistrations so, need to doing it manually via onBind()/onUnbind()/onRebind(). That means that the client needs to bind to the Service *and* to register to EventBus. The ServiceEventConnection class is introduced as a helper to that end.

mzorz · 2017-11-10T13:15:34Z

WordPress/src/main/java/org/wordpress/android/ui/accounts/signup/SiteCreationService.java

+            public void onErrorResponse(VolleyError error) {
+                setState(SiteCreationPhase.FAILURE);
+//                ToastUtils.showToast(ThemeBrowserActivity.this, R.string.theme_activation_error,
+//                        ToastUtils.Duration.SHORT);


let's not forget to remove this commented code here before 🎗

Truth be told, I left those in to handle right after we had a "green light" for the AutoForeground mechanism. Technically this PR is not ready to be merged yet. But thanks for pointing it out! I will remove the comments so we can ideally merge the PR. 👍

Removed with 311a2cd.

hypest · 2017-11-10T13:43:05Z

Hey @mzorz , I've revised the messaging system to be based on EventBus. Ready for another look!

Some notes:

EventBus has no mechanism to detect registration/unregistration of clients so, AutoForeground cannot detect when the last client disconnects in order to launch the statusbar notification. To mitigate this AutoForeground relies on bind/unbind to detect when clients connect/disconnect. To ease the handling of bind/unbind, AutoForeground now has become a parent class the Service inherits.

Also, to try and make sure the client does both the binding and the EventBus registration, the ServiceEventConnection helper is introduced. Still, the problematic case is still open where the client can unbind but erroneously register to EventBus. The client needs to only do the reg/unreg via the ServiceEventConnection.

mzorz · 2017-11-10T13:45:22Z

WordPress/src/main/java/org/wordpress/android/ui/accounts/signup/SiteCreationService.java

+        AppLog.i(T.NUX, event.toString());
+        if (event.isError()) {
+            setState(SiteCreationPhase.FAILURE);
+//            showError(event.error.type, event.error.message);


Just wondering, do we have this one commented out to avoid dupes with https://github.com/wordpress-mobile/WordPress-Android/blob/issue/6824-site-creation-service/WordPress/src/main/java/org/wordpress/android/ui/accounts/NewUserFragment.java#L747 and https://github.com/wordpress-mobile/WordPress-Android/blob/issue/6824-site-creation-service/WordPress/src/main/java/org/wordpress/android/ui/accounts/NewBlogFragment.java#L341 ?

I left those in just as a reminder-to-self to modify the client on the UI side to display errors. I will remove the comments from here. 👍

mzorz · 2017-11-10T13:53:02Z

libs/utils/WordPressUtils/src/main/java/org/wordpress/android/util/ServiceEventConnection.java

+
+import org.greenrobot.eventbus.EventBus;
+
+public class ServiceEventConnection {


What do you think about moving this entire class into AutoForeground.java? It's unclear what its purpose is if we leave it alone within a util package.

Makes sense! Done in 194b576.

mzorz · 2017-11-10T13:56:17Z

WordPress/src/main/java/org/wordpress/android/ui/accounts/signup/SiteCreationService.java

+    private Intent getPendingIntent() {
+        Intent intent = new Intent(this, NewBlogActivity.class);
+//        intent.addFlags(Intent.FLAG_ACTIVITY_SINGLE_TOP);
+//        intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);


I guess this is only used for the Progress notification (if the user taps on it, they will be taken to the NewBlogActivity), and as such we don't need to create a new task stack, etc. If that's its only purpose I think we can safely remove these commented lines.

I haven't fully implemented the notification tapping behavior and that's why those are still in comments. I'll remove the comments and we can fine tune the notification tapping behavior in a subsequent PR, is that OK?

mzorz · 2017-11-10T14:09:28Z

WordPress/src/main/java/org/wordpress/android/ui/accounts/signup/SiteCreationService.java

+            Intent intent = new Intent();
+            if (site == null) {
+                setState(SiteCreationPhase.FAILURE);
+                //            ToastUtils.showToast(getActivity(), R.string.error_fetch_site_after_creation, ToastUtils.Duration.LONG);


same here, right? (same question as in comment above)

hypest · 2017-11-10T14:25:48Z

I've address the additional feedback @mzorz , ready for another pass! Thanks!

mzorz · 2017-11-10T14:45:45Z

Code looks good! I was testing though and found an inconsistency when sending the app to the background as advised here:

Feel free to background the app and resume it. Notice the statusbar notification appearing, updating or disappearing.

the icon in the notification is not showing
the notification was showing the progress in all its stages, so I assumed things were going well, and finally got the "Site created!" notification (which stays there, should we remove it as soon as we bring the app back to the foreground?)

going back to the app it says "Sorry, failed to properly create your site :(", which makes sense as I was trying to create the same site (taken from debug params)

hypest · 2017-11-10T14:50:14Z

finally got the "Site created!" notification (which stays there, should we remove it as soon as we bring the app back to the foreground?)

Yes, the relevant work in not done yet. Will do in subsequent PRs of the feature. Still, thanks for pointing out.

hypest · 2017-11-10T14:56:10Z

going back to the app it says "Sorry, failed to properly create your site :(", which makes sense as I was trying to create the same site (taken from debug params)

Yeah, the flow there is only a "test" so to put together the Service and a client for it. It will be properly implemented according to the feature design which is in progress.

hypest · 2017-11-10T15:08:54Z

Pushed a commit to fix the notification icon in this PR @mzorz , thanks for the suggestion over chat.

mzorz · 2017-11-10T15:25:27Z

🎉

hypest added 2 commits November 3, 2017 13:20

Context instead of Activity in SiteSettingsInterface

fc2d396

This way, SiteSettingsInterface can be used by Services as well.

Introduce a Service for new-site creation

86ffba9

The fragment attaches, detaches and spawns the new-site creation Service, updating its UI to reflect the process's stage.

hypest added the Signup label Nov 3, 2017

hypest mentioned this pull request Nov 3, 2017

Site-creation rework: Master Issue #6712

Closed

13 tasks

hypest added 3 commits November 6, 2017 09:18

Small optimization by using the builder pattern

c97620d

Let the Listener check Activity finish if needed

0247fd6

Merge branch 'develop' into issue/6824-site-creation-service

5a0007a

mzorz self-assigned this Nov 8, 2017

theck13 mentioned this pull request Nov 8, 2017

Login - "Logging in" progress dialog is never dismissed if you left the app during login #6700

Closed

hypest added 2 commits November 10, 2017 14:49

Remove unneeded class

17b6684

mzorz reviewed Nov 10, 2017

View reviewed changes

hypest added 2 commits November 10, 2017 15:25

Stay in background if EventBus clients active

10be1e0

Move to background when rebound

0d9ac9f

mzorz reviewed Nov 10, 2017

View reviewed changes

hypest added 2 commits November 10, 2017 16:20

Move inside AutoForeground for better correlation

194b576

Remove uneeded comments

311a2cd

Statusbar icon fix to have it being opaque

17e7637

mzorz merged commit 7bd1516 into feature/new-signup Nov 10, 2017

hypest deleted the issue/6824-site-creation-service branch November 10, 2017 15:30

hypest mentioned this pull request Nov 17, 2017

Perform the WPCOM email+password login in the background #6878

Merged

hypest mentioned this pull request Nov 28, 2017

Update SiteCreationService from latest AutoForeground #6916

Merged

	WordPress.getBuildConfigString(getActivity(), "DEBUG_DOTCOM_NEW_SITE_TITLE"),
	WordPress.getBuildConfigString(getActivity(), "DEBUG_DOTCOM_NEW_SITE_TAGLINE"),
	WordPress.getBuildConfigString(getActivity(), "DEBUG_DOTCOM_NEW_SITE_SLUG"),
	WordPress.getBuildConfigString(getActivity(), "DEBUG_DOTCOM_NEW_SITE_THEME"));


		import org.greenrobot.eventbus.EventBus;

		public class ServiceEventConnection {

Conversation

hypest commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzorz commented Nov 8, 2017

Uh oh!

hypest commented Nov 9, 2017

Uh oh!

mzorz commented Nov 9, 2017

Uh oh!

hypest commented Nov 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hypest commented Nov 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hypest commented Nov 10, 2017

Uh oh!

mzorz commented Nov 10, 2017

Uh oh!

hypest commented Nov 10, 2017

Uh oh!

hypest commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hypest commented Nov 10, 2017

Uh oh!

mzorz commented Nov 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hypest commented Nov 3, 2017 •

edited

Loading

hypest commented Nov 10, 2017 •

edited

Loading