square / mortar

A simple library that makes it easy to pair thin views with dedicated controllers, isolated from most of the vagaries of the Activity life cycle.
Apache License 2.0
2.16k stars 156 forks source link

migrating to Flow, mortar, dagger2 (and Anko) woes... #163

Open Lakedaemon opened 9 years ago

Lakedaemon commented 9 years ago

Though I worked around the LayoutInflater bug that swaps PathContext with AppCompatActivity by using Anko to instanciate my views with the right context, I still have problems migrating my app to Dagger2, Flow & Mortar :

1) The way I save/restore state is my ViewPresenter must be wrong as state gets lost (and when I set flow to a new screen. It's really suspicious.),

I know that takeView/dropView might be called out of order and read the recommandations in the source code

I used print statements to figure out what was happening when I set a new View with flow but there is something suscpiscious happening : I get a lot of dropView/takeView (some out of order) and view creations (like a total of 13 of those calls each time I set a view)

2) I don't understand how to get "flow Path"-scoped MortarScopes

I would like to use those, but havent figured out how to have them yet by looking at the doc on those sites :

https://github.com/square/flow (for the flow + path concepts) https://github.com/square/mortar (there doesn't seem to be a sample for mortar+flow+dagger2, though it looks like there is one with mortar+flow+dagger1, with a ScreenScoper that uses Dagger1 ObjectGraph)

https://github.com/pyricau/dagger2-mortar-flow-experiment/issues/2 https://github.com/square/mortar/issues/129

https://github.com/techery/presenta

https://github.com/lukaspili/Auto-Mortar https://github.com/lukaspili/Auto-Dagger2

I was hoping that Flow/Mortar would make things simpler for me but, at this point, this isn't the case. I'll post my progress here (to get some help/insight...and to help others that might have the same trouble than me)

Lakedaemon commented 9 years ago

A) The documentation says that you can create custom mortar scopes but how do you do that ?

These nested scopes can shadow the services provided by higher level scopes.

I would guess that you have to wrap a context in a custom MortarContext, whose getSystemService() method returns the custom mortarScope instead of the root Mortar Scope.

But when/where do you do that with Flow ? In the Flow Dispatcher ?

Could we have a flow+mortar+dagger2 sample with a custom MortarScope for some flow screen ?

Let's see... We use a MortarScreenSwitcherFrame in our rootLayout. This uses a SimplePathContainer (btw, in the official sample it uses an ActionBar instead of dedicated Toolbar for each Flow view or a shared ActionBarOwner to manage shared ActionBars) that has a Path.contextFactory...

This contextFactory is given a new MortarContextFactory() that uses a ScreenScoper that uses dagger1 because of this part :

    MortarScope childScope = parentScope.findChild(name);
    if (childScope == null) {
        childScope = parentScope.buildChild()

.withService(ObjectGraphService.SERVICE_NAME, ObjectGraphService.create(parentScope, childModule)) .build(name); }

So... I would need to change this part from the dagger1 way to the dagger2 way... but is this possible ? (dagger1 uses reflection and can do stuff at runtime that dagger2 who uses annotation processing cannot do, right ?)

Instead of dagger1 subgraph, I guess that we are supposed to use subcomponent (:/ and as I'm a complete begnner with Dagger2, this sounds like a lot of work/boilerplate to me... and I don't know if I'm capable to do that)

Also... dagger2 is for dependency injection... I just want to have a path-scoped MortarScope. Why would I have to make my hands dirty with complex dagger2 code to get a Flow+ Mortar (-dagger2) MortarScope ?

Mmmh, I compared the code in the mortar sample and in the presenta library code and both use a mostly similar MortarContextFactory.

There is a difference in the ScreenScoper and the screenScoper.getScreenScope-) though

The one in the mortar sample supports some dagger1 service, and the one in the presenta library builds a child mortar scope that supports 2 services (Dependency injection and presenter, that works through annotated paths) :

public MortarScope getScreenScope(Context context, String name, Path path) { MortarScope parentScope = MortarScope.getScope(context); MortarScope childScope = parentScope.findChild(name); if (childScope != null) return childScope;

MortarScope.Builder builder = parentScope.buildChild();

ServiceFactory serviceFactory;
serviceFactory = presenterServiceFactory.getServiceFactory(path);
if (serviceFactory != null) {
  Object presenter = serviceFactory.getService(context, path);
  return builder.withService(PresenterService.SERVICE_NAME, presenter).build(name);
}
serviceFactory = componentServiceFactory.getServiceFactory(path);
if (serviceFactory != null) {
  Object component = serviceFactory.getService(context, path);
  return builder.withService(DaggerService.SERVICE_NAME, component).build(name);
}
Log.w(TAG, "Path " + path + " has no additional service in mortar context");
return builder.build(name);

}

...mhhh, so this is how child mortar scope creation looks like....

Lakedaemon commented 9 years ago

B) I have started investigating how to restore/save states....

Though I have created a mortar scope for my paths, it looks like onSave is never called. (I'm pretty sure that at some time I got some onSave calls in a previous attempt).

I'm using a ViewPresenter with the Singleton annotation.... I'll investigate further

Apparently, when my MortarActivity fires onSaveInstanceState(outState: Bundle), the BundleServiceRunner I get from BundleServiceRunner.getBundleServiceRunner(this) is not null but has : scoped services {} and state : IDLE

Which explains why I don't get any onSave call in my presenter... there are no services to save. But why is it like this ? 0.o I probably broke something with the ScreenScoper and stuff, let's look t that

wow... Mortar takes a parent scope and a context to wrap it in a MortarContextWrapper that is itself wrapped in a TearDownContext (that is also a ContextWrapper)

/me's confidence in resolving his problem by himself is slowly dwindling...

Lakedaemon commented 9 years ago

The Exception message in there is wrong : public Builder withService(String serviceName, Object service) { if (service instanceof Scoped) { throw new IllegalArgumentException(String.format( "For service %s, %s must not be an instance of %s, use \"withScopedService\" instead.", serviceName, service, Scoped.class.getSimpleName())); } return doWithService(serviceName, service); }

there is no "withScopedService" method, there is an overloaded withService(String, Scoped) method instead

lukaspili commented 9 years ago

I made a example project with mortar flow dagger2, up to date with latest versions of those libs: https://github.com/lukaspili/Mortar-Flow-Dagger2-demo

If you still have any troubles, you should use stackoverflow rather than this issue page. Or use my repo issue page if you have troubles regarding my implementation of mortar/flow/dagger2.

Lakedaemon commented 9 years ago

Thanks for this sample. I'll take a very good look at it. It's nice to have an up to date dagger2 + mortar + flow sample.

I'm sorry though, despite your advice, I think I'll keep on posting troubles/findings and workaround here because I think I'm really reporting an important issue of the mortar (+ flow+ dagger) library :

It is lacking in documentation (what is in the readme is really minimal,the doc in the code source too sometimes) in a few places and it brings pain to the user of the lib that is left in the dark.

For example : 1) When using Flow (set with a forward movement/animation), you might end up with 2+ added views of the same type. If these views use a Mortar ViewPresenter injected with Dagger, you have to take special non trivial steps to make it work, like the one suggested there : http://stackoverflow.com/questions/26475248/mortar-having-a-few-instances-of-the-same-view-in-one-activity/26539075#26539075

2) Also, it is dangerous to build mortarScopes in getSystemService, like explained here : https://github.com/square/mortar/issues/155

3) ViewPresenter should be Singleton

4) dropView / takeView might be called out of order

5) There aren't much examples/explanations on how/when/why save/restore should be implemented (most samples only use dropView),

Basically, you have to look at the code source or to figure it out yourself. Could we please have more documentation ? a faq ?

I have already spent quite a lot of time looking at code source, googling, reading blogs, trying to figure out how things should work and I'm still not done... :/

Lakedaemon commented 9 years ago

So... In my dictionary app, the user might navigate from screen to screen like this : Search("lov") -> Word("love") -> Word("lovable")-> Verb("to love") -> Search("hate") -> ...

And to synthetize... a) Search/Word/Verb screens are complicated, with ton of stuffs and so need dedicated presenters. b) Those screen have view-state (where the cursor is, scroll...) and model-state (what data is displayed...) those states need to be saved and restored (for navigation/back/screen purpose) c) Models & views need to be bound to presenters d) Presenters need to be Singleton, because mortar make sure they survive orientation change by reinjecting them e) a view (before orientation change) may be dropped after the same view (after orientation change) is added because of the activity life cycle (dropView happens in on ExitScope....) f) we may have 2+ same views attached at the same time (Flow/animation)

-> A presenter that is able to (delegate) handle 2 current views and ignore 0+ obsolete views is needed. When an orientation change happens, it must know which subpresenter it has to inject and where (use tags ?)

-> or maybee, in the App have 2 sets of view :/ and alternate so that animations don't mess with presenters.... like WordA/WordB and use stuff like Flow.set(WordB, Forward) when flow is already displaying a WordA view Code duplication sucks, it's dirty hack but it might work

-> a singleton parentPresenter, that injects/delegates differents subPresenters depending on Path.get(context).someVariable -- a bit like the stackoverflow solution

Lakedaemon commented 9 years ago

I still have issues with converting my App to Mortar + Flow + Dagger2. I spend some time investigating why I don't get onSave calls for my presenters and it looks like :

myPresenter.onSave() method is not called because there is no scoped service in the associated BundleService because the activity scope got teared down and another blank activity scope replaced it... O.o

I have a hard time figuring out what is happening because there is no debug code for MortarScope, BundleServiceRunner... (but luckily lots of vars in there are public or package private/internal) so I had to write my own debug code like

package mortar.bundler

import android.content.Context import mortar.MortarScope import mortar.debug import mortar.fullDebug import org.lakedaemon.L

fun Context.debugServices() : String { val a = BundleServiceRunner.getBundleServiceRunner(this) return """bundleServiceRunner $a services ${a.scopedServices} to be loaded ${a.servicesToBeLoaded} state : ${a.state} """ }

fun Context.mortarDebug(tag:String, details:Boolean = false) :Unit = if (details) L.info("""$tag services ${debugServices()} contexts : ${debug()} scope : ${MortarScope.getScope(this).fullDebug()} """) else L.info("$tag ${MortarScope.getScope(this)}")

and...

package mortar

import android.content.Context import android.content.ContextWrapper import mortar.bundler.BundleServiceRunner

fun MortarScope.debug() =""" name ${getName()} path ${getPath()} destroyed : ${isDestroyed()} parent : $parent children : $children """

fun MortarScope.fullDebug() :String { val s = StringBuilder() var scope = this s.append("+").append(scope.debug()).append("\n") for (entry in children) s.append(entry.getKey()).append(" : ").append(entry.getValue().debug()).append("\n\n") return s.toString() }

fun Context.debug() : String { val s = StringBuilder() var c = this while (c is ContextWrapper ) { s.append(c.toString()).append("\n") c = c.getBaseContext() } s.append(c.toString()).append("\n") return s.toString() }

hint hint : it would be nice to have nice debug/toString methods in the flow/mortar libs

Lakedaemon commented 9 years ago

Also, I have some doubt about the dagger2-sample way of implementing getSystemService in the activity :

@Override public Object getSystemService(String name) { MortarScope activityScope = findChild(getApplicationContext(), getScopeName());

if (activityScope == null) {
  activityScope = buildChild(getApplicationContext()) //
      .withService(BundleServiceRunner.SERVICE_NAME, new BundleServiceRunner())
      .withService(DaggerService.SERVICE_NAME, createComponent(Main.Component.class))
      .build(getScopeName());
}

return activityScope.hasService(name) ? activityScope.getService(name)
    : super.getSystemService(name);

}

If your activityScope got teared down (like in my case) for an unknown reason, the next time you need the activityScope, another blank one will be created... :/ And it will be hard to understand the issue (shrodinger cat issue : you want to inspect the faulty scope but, by inspecting it, you create and inspect another one)

I think that the way the activityScope was created in the dagger1 sample was better, considering that aspect (it will crash the app with the activityScope not being found and a stack trace)

Lakedaemon commented 9 years ago

Now... I don't know if I'll manage to make all of this work and I'm already quite burned out but I really need this to work so let's make some more tries... :/

/me will now look at the suspicious oldPath.destroyNotIn(context, contextFactory) calls that might destroy the bad contexts if I somehow messed the history (right ?)... lots of private vars/methods in there and no debug function :( it's not gonna be fun looking for what's wrong...

Also, I'm still not understanding how the @Scope anotation work/should be used That may be the source of my issues (but again, not much documentation about that on the internet....(look at the JSR ?))

pforhan commented 9 years ago

Minor comment: override getSystemService in your Application object, not your Activity.

Lakedaemon commented 9 years ago

Thanks for the advice, I'll try to do that (yet, we still have to add the flowService in the activity, as in the sample).

Also, I think that I managed to fix my main issue : It was actually a variation on https://github.com/square/mortar/issues/149 and was fixed by https://github.com/square/mortar/pull/161

-> Using class name for scopeNames is bad in most cases : as soon as you have 2+ screens using the same class (with or without different members), you'll run into bugs and really difficult to find/figure out/fix issues, because the scope name collisions will make some scopes to get unexpectly destroyed and you'll then start to get NPE/weird behavior in unexpected places/at unexpected times

It's much better to use copeNames dependant on the instance of the screen. (it fixed my problems anyway)

So, in the MortarContextFactory for the mortar-sample, this piece of code @Override public Context setUpContext(Path path, Context parentContext) { MortarScope screenScope = screenScoper.getScreenScope(parentContext, path.getClass().getName(), path); return new TearDownContext(parentContext, screenScope); }

is really really evil (took me 1 week to solve this :/). Your sample might work (I didn't try), but noobs like me that will want to build on the sample to integrate flow/mortar/dagger in their apps will get burned bad.

Strangely, this snipet is still in the master branch when the following PR should have fixed this already : https://github.com/square/mortar/pull/161. Please fix this.

Zhuinden commented 8 years ago

While i know this is outdated, but I put together a sample (a while ago, actually) that has run into similar caveats and therefore makes a comment on them in that regard - which is why my BasePath has a getScopeName() method.

https://github.com/Zhuinden/MortarFlowInitialDemo