moxy-community / Moxy

Moxy is MVP library for Android with incremental annotation processor and ktx features
MIT License
324 stars 33 forks source link

Build error "A View method has no strategy!" when view interface override with strategy. #94

Closed ValdZX closed 4 years ago

ValdZX commented 4 years ago

Sample:

            public interface OtherModuleView {
                void helloFromOtherModule();
                void helloFromOtherModule(String reload);
            }
            public interface MainView extends MvpView, OtherModuleView {
                @StateStrategyType(AddToEndStrategy.class)
                void printLog(String msg);

                @StateStrategyType(OneExecutionStateStrategy.class)
                void openKtxActivity();

                @StateStrategyType(AddToEndStrategy.class)
                @Override
                void helloFromOtherModule();

                @StateStrategyType(AddToEndStrategy.class)
                @Override
                void helloFromOtherModule(String reload);
            }
alaershov commented 4 years ago

Hi @ValdZX! Thank you for submitting this issue, it definitely needs to be addressed. We discussed it with @asitnkova and found that the underlying question is even more fundamental:

Given any interface inheritance hierarchy for a View interface, how should the compiler resolve the resulting strategies for all its methods, both enclosed and inherited?

There are many edge cases, like methods with no strategies, clashing methods with different strategies, overridden methods with different strategies and so on. So before accepting any pull requests fixing the issue, we first need to specify the desired behavior for all these cases. We'll create a Wiki page with the spec, and we can discuss it in this issue.

Please do share your thoughts on this matter, as well as any other edge cases that come to mind.

alaershov commented 4 years ago

Here is a list of cases that came to my mind. I'll provide code samples later, we can use them for tests.

MvpView inheritance

  1. MyView extends MvpView, ViewA, ViewA does not extend 'MvpView'.
  2. MyView extends ViewA, ViewA extends MvpView.
  3. MyView extends MvpView, ViewA, ViewA extends MvpView

Suggested behavior: should work the same in all cases

Question: do we even need MvpView at all?

Strategy conflict

  1. MyView extends ViewA, ViewB, both ViewA and ViewB have the same methods with different strategies. Suggested behavior: compilation fails with an error.
  2. MyView extends ViewA, both MyView and ViewA have the same methods with different strategies. Variant: one of the methods does not have a strategy at all. Suggested behavior: the most "bottom" method in a hierarchy wins, if it has a strategy. If it doesn't have a strategy, compilation fails.
  3. MyView extends ViewA, ViewA has a method with a strategy that is not overridden in MyView. Suggested behavior: method from ViewA is inherited with its strategy, and used to generate the resulting ViewState.

@asitnkova @xanderblinov @ValdZX any other suggestions?

aasitnikov commented 4 years ago

MvpView is just a marker interface. I think it's a good idea to explicitly define, what interfaces are view interfaces. And there's no need for an explicit check because MvpPresenter's type parameter is bound by MvpView.

Regarding strategy conflict, I think we need to specify these two points:

  1. How to resolve, which method to take, when a method with the same signature is present in
    • Current view interface and one of a superinterfaces
    • Two of superinterfaces

I think, if the method overrides superinterface's method, then all strategy info from the superinterface should be discarded. And if the same methods with different strategies exist in two superinterfaces, than compilation should fail, and the user has to override this method and specify strategy manually.

  1. When to validate for the absence of strategies?

Right now methods are validated right as they parsed, which causes the reported bug. So we should move validation to the end of processing after all superinterfaces are processed.

There are some snippets showing the described behavior:

MyView overrides someMethod()

interface ViewA {
    @OneExecution fun someMethod()
}

interface MyView : ViewA {
    @AddToEnd override fun someMethod()
}

class ViewState: MyView {
    override fun someMethod() // AddToEnd
}

No override

interface ViewA {
    @OneExecution fun someMethod()
}

interface MyView : ViewA

class ViewState : MyView {
    override fun someMethod() // OneExecution
}

ViewA defines no strategies, MyView overrides someMethod(), strategy placed on interface

interface ViewA {
    fun someMethod()
}

@AddToEnd
interface MyView : ViewA {
    override fun someMethod()
}

class ViewState : MyView {
    override fun someMethod() // AddToEnd
}

Speaking of implementation, current ViewMethod class could be replaced with two classes:

After all of the superinterfaces are processed, ViewMethods will be converted to ViewStateMethods, reporting on missing strategies if any.