Skip to content
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

please annotate method parameters with Nullable and NonNull #42

Closed
Lakedaemon opened this issue Jun 28, 2017 · 12 comments
Closed

please annotate method parameters with Nullable and NonNull #42

Lakedaemon opened this issue Jun 28, 2017 · 12 comments

Comments

@Lakedaemon
Copy link

Hello, I'm giving a try at Simple-stack (a bit pissed of by the slow pace of development of flow and also
hoping that it has better state management...)

Could you please annotate paramethers in methods, like in LayoutInflationStrategy ?
When using Simple-stack from kotlin, lots of things look nullable when they shouldn't be.

@Lakedaemon
Copy link
Author

in
public ViewChangeHandler getViewChangeHandler(@nonnull StateChange stateChange, @nonnull ViewGroup container, @nonnull Object previousKey, @nonnull Object newKey, @nonnull View previousView, @nonnull View newView, int direction) {

there si a null check on previousKey... either it is unnecessary or the annotation is wrong

@Lakedaemon
Copy link
Author

Lakedaemon commented Jun 28, 2017

DefaultStateChanger.NO_OP_VIEW_CHANGE_HANDLER is private... but it is a static immutable variable of a public final class !

@Zhuinden
Copy link
Owner

I'll get home and get to work on that!

@Zhuinden
Copy link
Owner

Zhuinden commented Jun 28, 2017

Can you please verify that 1.6.3 fixes all issues in Kotlin?

Btw: if previousKey is null, then view change handler is not called. So the annotation is ok

@Lakedaemon
Copy link
Author

Lakedaemon commented Jun 28, 2017

The annotations (and the explanation) fix my (minor) issues, thanks.

As I would like to create views through code (anko, anvil, or something else) or xml, I'm not going to use StateKey. I saw in the samples that Key extends StateKey for services, so I'm not going to use that either.

I have a few questions :

  1. when going to a key, I guess that a view is created, and then it's ViewState is restored, before it is added to a container.
    In the view creation step, If I'm bidding RxObservables to the view listeners and subsribes to observables that mutate the view, won't there be issues if the viewState is restored afterwards and that make observers react ?

Should there instead be : view creation, view state restoration, tying the view to boservables/subscriptions ?

  1. what do you recommand for managing services ?
    a) use dagger2 and subscoping (for setting up services and teardown)
    b) use mortar scopes ?
    c) use service-tree ? For simple scopes ? Are nested scope supported ?

My needs aren't extreme : I could make do with app/activity scope
Even though, It would be nicer to have a scope that can span keys of the (somewhat) same type (say Service set up with Key 1, on with Key2, on with Key 1, teared down with Key3)

  1. are nested stack and multistack equivalent to flow's treeKeys and multiKeys ?

@Zhuinden
Copy link
Owner

Zhuinden commented Jun 29, 2017

The NestedStack example's state restoration is bugged ( #35 ), I'll have to rewrite the NestSupportServiceManager class but I didn't feel like it yet. ¬¬ (nested stack example was removed)

I've been thinking a lot about introducing out of the box scoping (and brainstormed about it here #25 ) but I came to the realization that it's only the Backstack that could provide scope per each key, but then managing whether it is the root of a service tree or a child of one is difficult, and also it'd be hard to create a sufficiently customizable way for users. Managing composite keys (MultiKey) is kinda hard! In fact, that's why Flow 1.0-alpha doesn't handle it right either.

So I had to come to the conclusion that "scoping is not the responsibility of the library. For scoping, do it in handleStateChange() method."


The simple-stack-mortar-example has a reasonable implementation of scoping, but that didn't have nesting.

I was experimenting with nesting in this repository, maybe it can give some ideas?


As I would like to create views through code (anko, anvil, or something else) or xml, I'm not going to use StateKey. I saw in the samples that Key extends StateKey for services, so I'm not going to use that either.

If you replace the GetViewChangeHandlerStrategy and LayoutInflationStrategy in DefaultStateChanger, then implementing StateKey is not needed.

when going to a key, I guess that a view is created, and then it's ViewState is restored, before it is added to a container.
In the view creation step, If I'm bidding RxObservables to the view listeners and subsribes to observables that mutate the view, won't there be issues if the viewState is restored afterwards and that make observers react ?

If you start observing in onAttachedToWindow() instead of immediately in onFinishInflate(), then this is not a problem, because it is added to container after state is restored.

Back in Flowless I had a callback for "onViewRestored()" and "onViewDestroyed()", sometimes I think about whether I should have brought it over here as well.

what do you recommand for managing services ?

Subscoped Dagger2 components that are stored in either MortarScope or ServiceTree.Node.

If you check the simple-stack-mortar-example, you'll see that a StateChangeCompletionListener is registered to handle destroying scopes, but the beginning of the handleStateChange() method could handle the creation of them.

Both ServiceTree and Mortar are capable of building the scopes you need; I just don't trust the BundleServiceRunner so the example managed to end up reimplementing it using traverseTree() method.

are nested stack and multistack equivalent to flow's treeKeys and multiKeys ?

No, nested stack example would have been storing backstack instances in subscopes. But I messed up somewhere, so it has state restoration bug atm. :|

The Composite key is MultiKey, and Child key is TreeKey.

@Zhuinden
Copy link
Owner

Zhuinden commented Jun 29, 2017

But if that seemed like rambling I can explain it better, I guess.

I've been meaning to solve the scoping problem, but honestly it's kinda hard.

So in real life what we did is that we used simple-stack with fragments, and used detach/attach/add/remove/show/hide methods of fragment manager; so I didn't have to set up an elaborate scoping mechanism. The simple-stack-mortar-example mixed with the ScopeManager i mentioned above are the closest I got, but aren't part of the library, and I couldn't allocate time on those yet.

I prefer ServiceTree to Mortar because you can traverse your scope hierarchy, while Mortar doesn't let you do that.

@Lakedaemon
Copy link
Author

Lakedaemon commented Jun 29, 2017

I'm indeed using a GetViewChangeHandlerStrategy and LayoutInflationStrategy.

Thanks for the explanation about onAttachedToWindow.
I have been using onAttachToWindow and onDetach to Window in past projects (through mortar ViewPresenters) and that worked fine.
Now, I'm happy to know why it worked and why it will still work with simple-stack.

I'll probably try ServiceTree. I like library that are pure java and avoid android types.
It allows me to use it on desktop too.

Scoping might be a difficult problem (not thought enolugh about it to give valuable advice).
I only know that I like having symetric callbacks : onEnter..., onExit...
It is simple and you can do nice stuff on the user side with that.

Btw, Is there a (reliable) way with simple-stack to get "onEnterActivities" callback when creating an activity
an "onExitActivities" callback when destroying an Activity outside of orientation change ?
It would be nice for creating/tearingDown some expensive services
Using a retained fragment :/ ? or testing isFinishing() in activity onPause, like they suggest here :
https://developer.android.com/guide/topics/resources/runtime-changes.html#RetainingAnObject

a bit like with mortar ? (not that I used it on my dagger/flow/mortar applications : I was recreating my observables when entering/exiting views regardless of orientation change...which was one of the selling point of mortar)

@Zhuinden
Copy link
Owner

Zhuinden commented Jun 29, 2017

onExit and onEnter are provided in ServiceTree via Scoped interface for the service. It is called when it is added to the node, and otherwise called when it is either removed from the node, or the node is removed from the tree.

Config changes don't affect ServiceTree, as long as it is kept alive across config change. But that is also why you need to explicitly create and destroy nodes (similarly to how you'd call MortarScope.get(context).destroy().

Activity callbacks are a pain, Mortar used isFinishing() and I tend to use retained fragment's onDestroy() method.

@Zhuinden
Copy link
Owner

The issue is closed because annotations are added, but feel free to ask any more questions and stuff :)

@Zhuinden
Copy link
Owner

Zhuinden commented Jul 13, 2017

@Lakedaemon Just asking to see if everything's working?

I remember your name from the square/flow repositories, so I'd be happy if you didn't need to resort to extreme hacks anymore to get things working like in times of Flow-Path or ServiceFactory.

Been there, wasn't fun. 😄

@Lakedaemon
Copy link
Author

I haven't ported my complex app to simple-stack yet.
For the moment, I'm learning simple-stack by writing a new app from scratch and researching how I should improve the way I code : I ditched mortar scopes, replaced mortar presenters with custom views and (at last, I wasn't doing it before) implemented state save/restoration...

Everything works so far. So I'm quite happy.
I'll delve deeper in simple-stack (and out of flow) in the coming months...

Btw, Thanks for the library and the support :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants