Breaking change proposed: future proofing Activity in 2.1.1

[Ray Ryan]

We're making a few breaking changes in 2.1.1 to the new features introduced in 2.1. (We're not supposed to do that kind of thing, but are hoping to get away with it in this quick follow up release before there is much adoption.) 

I'd like to add a change to Activity to that list, in order to allow it to evolve in later releases when breakage of any kind won't be an option: I'd like to make Activity an abstract class instead of an interface, basically rename AbstractActivity.

Any objections?

rjrjr

[Amir Kashani]

We've adopted the new MVP framework pretty heavily in a couple of new projects and at this point, I don't think we've ever not used AbstractActivity. So, as long as the existing methods in Activity don't become inaccessible to non-GWT code (i.e. not package protected or final), I don't see a problem.

My only concern is that while making it an abstract class makes it easier to evolve, it also make it easier to default behavior that the end-user may not want. If a mechanism isn't provided to override this behavior, then someone may end up having to roll their own MVP framework for an otherwise minor issue. With a something as powerful and complex as this framework, that'd be a real shame. So, please please please, be cautious with this power: reasonable default behavior and the ability to override.

Thanks!

- Amir

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Neil Fuller]

Full disclosure - I am the one who has suggested a possible evolution of Activity. I was suggested the introduction of a mechanism to allow an existing Activity to be informed when the Place has changed but the Activity has not (e.g. when there is a change in Place tokens). I commented about how difficult it would be to add a method to Activity without introducing a break and Ray kindly started this thread.

I agree with Amir about the concerns. The AbstractActivity class and the interface it implements has to have sensible, obvious, do-nothing implementation otherwise subclasses have to override the behavior they don't want and it all gets messy. Also, in Java we get to have one base class, so extending AbstractActivity precludes extending something else. Several APIs I've worked with have moved away from a design that requires developers to extend a particular class. e.g. JUnit

Removal of the interface entirely would make mocking with frameworks like EasyMock more difficult. Although things like the EasyMock classextension make it possible to mock non-interfaces it's not ideal and obviously isn't an option in a GWT test case.

Personally I'd like to try to keep the interface for these reasons. How about the documentation is beefed up on the interface to say "You should extend AbstractActivity rather than implement Activity directly otherwise you may be broken" if it doesn't already, but leave the interface in place for people who want to take on the risk of a break? I guess it depends on how much Activity is expected to change in future. In the case I have in mind to Activity the implementation would probably have an obvious no-op implementation, but without having talked about that it's hard to be sure. Luckily, I'm pretty sure that the use of AbstractActivity is what most people will have done anyway.

I've just started looking at the Activity framework and this particular wrinkle was the first we hit, but I wonder how many more things the ActivityManager (and associated classes) would want to call on the Activity interface so the need to evolve Activity much further may not be required. I do agree that now would be a good time to do this kind of change, though, because it's only going to get harder.

Neil.

--
Google UK Limited

Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ
Registered in England Number: 3977902

--
Google UK Limited

Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ
Registered in England Number: 3977902

[Ray Ryan]

Let me push harder for the abstract class. If the class is documented to forbid non-trivial default implementations, there would be no need to mock it, and no chance of breaking people who decide to use the interface directly for whatever reason. 

WRT to the single base class problem, I was initially going to propose that we also add interface IsActivity { Activity asActivity(); } and make Activity implement it. But I didn't see any immediate use for it, since nothing at the moment accepts an Activity as an argument, and nothing returns one except for ActivityMapper. If such a need appeared it would be trivial to add.

You buying it? Is the loss of easyMock too much?

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Patrick Julien]

making it a class instead of an interface means we can't mock it anymore.

> --
> http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Ray Ryan]

Any part of my point is that making sure it remains a trivial class with only no-ops means you don't need to mock it. Is that a reasonable assumption?

> http://groups.google.com/group/Google-Web-Toolkit-Contributors

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Patrick Julien]

I don't know since I don't know what your plans are, will just have to
trust you.

That being said, the Activity interface is currently really nice and
it doesn't tie us down to a single class for inheritance.

> --
> http://groups.google.com/group/Google-Web-Toolkit-Contributors

[John Tamplin]

On Fri, Dec 3, 2010 at 2:58 PM, Patrick Julien <pju...@gmail.com> wrote:

I don't know since I don't know what your plans are, will just have to
trust you.

That being said, the Activity interface is currently really nice and
it doesn't tie us down to a single class for inheritance.

I have been very happy with the recent cases where I have used an interface for the API but provided a default implementation, with the admonishment that implementing the interface without extending the default implementation is likely to be broken in the future.  That way the people that care more about being able to substitute alternate implementations or to use it without having to extend the implementation can implement the interface, and those that care more about not being broken by future updates can extend the default implementation.

--
John A. Tamplin
Software Engineer (GWT), Google

[Patrick Julien]

This is more in line with what we're doing. With what we experienced
with the ramp up to 2.1.0, we only use the Activity interface, we
don't use the default implementation and instead make our own for
common classes of use cases.

> --
> http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Ray Ryan]

Patrick, you're the case in point. Because you don't use the abstract class, if we change the API later we will break your app. 

Were you unable to use the abstract class? If the Activity interface were documented to encourage you to do so, would you have? When we break your app, will you be okay with that?

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Amir Kashani]

I don't know what features are planned or contemplated beyond what Neil mentioned, but could the use of sub-interfaces help? For example, Neil's feature (+1 BTW) might use a PlaceAwareActivity which extends Activity? AbstractActivity can potentially implement them all new sub-interfaces.

The downside being that other framework classes, such as ActivityManager would be littered with instanceof checks.

- Amir

--

http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Ray Ryan]

One more question for Patrick: would you be better able to use AbstractActivity if the IsActivity interface were available?

[Patrick Julien]

On Fri, Dec 3, 2010 at 3:18 PM, Ray Ryan <rj...@google.com> wrote:
> Patrick, you're the case in point. Because you don't use the abstract class,
> if we change the API later we will break your app.

I guess I don't understand how you're making it better then. I'm
currently working on Android and I have two problems that I hope won't
be introduced in GWT:

1. The activity class is huge. Anything that doesn't seem to fit
somewhere else either goes in Context or Activity (with Activity
inheriting from Context)
2. Activity isn't Guice friendly because the framework creates
activities for you, we don't get absolute control of the ctors. It's
driving me insane. It's nice we don't have to worry about creating
activities and other system level objects but kind of sucks that we
can't get any kind of support beyond that.

Your argument about keeping the class simple helps reassure #1 won't
come true... for now at least, I'm more worried about the implications
of someone else becoming the steward of such a class. I can only hope
#2 will remain like it is now in GWT and not like Android.

> Were you unable to use the abstract class? If the Activity interface were
> documented to encourage you to do so, would you have? When we break your
> app, will you be okay with that?

The abstract class didn't provide anything useful so multiple abstract
base classes were made. In all, I have list, detail and edit abstract
base activities and each define a view interface. Each of those
scenarios provided a better, more suitably precise implementation than
what's already in AbstractActivity. So even if you add breaking
changes to Activity here, and I'm assuming by changes, you mean new
methods with a default implementation, otherwise an abstract class
wouldn't do much better, I have at most 3 classes to fix. All other
activities are rooted in one of those 3 and are pretty light.

But again, I wanted to voice my concern, not really trying to change
the tide. I don't really mind Activity becoming a class under the
conditions you mentioned. At first, I was just a little bit worried
of getting something that, sooner or later, would match the stature of
android's activity class.

[Patrick Julien]

On Fri, Dec 3, 2010 at 3:35 PM, Ray Ryan <rj...@google.com> wrote:
> One more question for Patrick: would you be better able to use
> AbstractActivity if the IsActivity interface were available?

I don't want to speak for anybody else but not to me no. What
AbstractActivity provides is so little that it's either sufficient or
not. In my case, it's not. Introducing IsActivity doesn't make it
any more useful to me. For list and edit, you can edit, so we need to
take charge of all the Activity entry point in order to provide the
user with a message about discarding changes. For details, it's fine,
but it hasn't really saved us much now has it?

I'm just surprised that you would want to make it an abstract class.
If we were to re-live the gwt 2.1.0 development cycle, I would have to
say that the activity package of classes was one of the most volatile
out there. Why? Because making an activity that is both useful and
generic for everyone out there is extremely hard and complex.

AbstractActivity is certainly generic enough for everyone but how
useful is it in its current implementation? If you want to improve on
that, great, but how much experimentation would be involved in such a
process? I'm just saying that anything added to an abstract Activity
that works for all GWT users/applications would have to be out of this
world crazy good or be benign like it currently is.

That being said, I don't know what new additions you're planning.

[Thomas Broyer]

We're not using AbstractActivity, but only because we do provide our
own implementation for all the Activity methods, so AbstractActivity
doesn't bring us anything. So, AbstractActivity replacing Activity
wouldn't change anything for us (search "implements Activity" and
replace with "extends Activity"). Moreover, we do have a base class
(enforcing MVP "best practices") that almost all our activities
extend, so the change would be very localized.

In brief: no objection. I'd be OK too with keeping it an interface and
instructing developers to rather extend AbstractActivity (that's
something that has been proposed for widgets too).

That being said, I don't understand the objections re. mocking (and
EasyMock 3.0 now integrates class mocking, it's no longer an
"extension"). The only issue with class mocking is final methods, and
the existing methods won't become 'final' and that would break a lot
of apps; new methods introduced later have few reasons to be 'final',
and if they ever are the'll delegate to non-final ones so mocking will
still be possible to some extent.

[Neil Fuller]

Sorry for being so prescriptive in my first email about interfaces and
mocking and starting a bit of a side discussion about interfaces Vs
abstract classes. It smells a little like a no-one-right-answer
discussion that has probably been debated in every API forum.

I think most people could live comfortably with each approach.
However, I've seen many cases of frameworks starting out with abstract
(or concrete) classes and then going through pain of introducing
interfaces when the problems crop up. I think it would be a shame to
go the other way. GWT is going through to the trouble of introducing
IsWidget, etc., presumably to support better composition. In GWT's
case that's always smelled to me like something that might have been
done differently if the designers could start with a clean slate. If
the convention in GWT it to have Is{Abstract|Concrete} and a default
implementation of the interface then it might be worth moving to that
model for consistency, but that would probably break even more code!

The Interface Vs AbstractClass discussion so far seem to break into
the two main points:
1) Multiple inheritance.
2) Mocking

My view:

1) Multiple Inheritance:
I think the problem usually comes down to: What if a developer class
naturally behaves like an Activity and a "something-else", where
"something-else" is written in some framework the GWT team have no
control over and also requires you extend another Abstract class.
Well, then we get into writing shims like getAsActivity(),
getAsSomethingElse() to get around the Java single inheritance model.
With interfaces, internally, the implementer may well be using
delegation/composition to avoid writing the same code again. Use of
inheritance for code re-use rather than true specialization is
convenient, but I've become less tolerant to it over time when it is
presented as the only way to do things (rather than a convenience)
because it seems to cause a lot of pain.

2) On the mocking point - perhaps everybody uses EasyMock now and
that's ok. My understanding is that EasyMock won't work under standard
GWT tests (somebody please correct me if I'm wrong). The class
extension code is, IMO, a clever way of mocking classes that were not
written with an API / testing in mind. Having previously measured the
performance impact of mocking classes Vs interfaces I can tell you
there was at one point a measurable several-order-of-magnitude
difference in performance. I don't know if that's still the case but
the reason we looked at it at all was that our small tests were taking
many seconds to run and we couldn't work out where all the expense
might be coming from. That's not a compelling argument backed up by
evidence but something people might want to debunk if using mocking-of-
classes extensively.

I guess Activity may be so simple this isn't something that I should
be concerned about, but having had to hand-roll stubs/fakes/mocks for
classes that were never designed for testing in mind (and also not
very amenable to EasyMock-style expectation-setting) I tend to have a
knee-jerk "interfaces are good, abstract classes are bad" reaction.
One problem is, if somebody adds a method to AbstractActivity and I'm
hand-rolling test classes there's no compile time error to say that
I've forgotten to override a method and so my AbstractActivity is now
only partially mocked. Those can be fun to debug. The "easy to evolve"
advantage of an abstract class actually causes problems in this one
case.

The other problem for me is that if you have the following:

new MyClassUnderTest(fooActivity)

And fooActivity is of type FooActivity. The developer made it an
interface because a mocking framework didn't fit, or because that's a
project standard, or they wanted a fake, not a mock or a stub. The
internals of MyClassUnderTest cannot then take FooActivity and pass it
to something expecting an AbstractActivity (without a nasty, test-
breaking cast) even if the developer has go to the trouble of
declaring identical methods on FooActivity as are inherited from
AbstractActivity. I've hit this a few times and had to resort to
EasyMock-class extension, or the "override everything" approach to
writing mocks.

Just my 2c.

Neil.

[Jeff Larsen]

Personally, I'm a fan of having both. The default implementation can
be an abstract class but have that abstract class implement the
Activity interface. Developers will be making a conscious choice to
use the interface only knowing that they can introduce bugs.

[PhilBeaudoin]

Tell me if I get this right, but the most important advantage of
having only an abstract class is that you are guaranteed your user
extends the abstract class instead of implementing the interface,
which let you easily extend it later (i.e. add methods) without
breaking existing user code?

On the other hand, it looks like, in a world of unchanging APIs, the
interface might be the best way to go, facilitating things like reuse
via a composition/delegation pattern. For example, MyPresenter could
inherit from BasePresenter class and implement Activity, delegating
all the calls to a composed AbstractActivity. Without an interface I
would have to refactor BasePresenter to inherit from the Activity
abstract class, making it impossible to have non-activity presenters.

So it looks to me like it boils down to:
1) How likely is it that the Activity interface evolves in a way that
would not be handled by adding subinterfaces or extra interfaces?
2) How frequently are users expected to compose/delegate with
Activity?

A concluding remark: GWTP went the abstract class way for its
hierarchy of Presenter classes. It makes it quite easy to use, but the
composition problem has reared its ugly head a couple of times. If I
had to do it again I would use interfaces and favor composition of
default implementations.

Cheers,

Philippe

[Ray Ryan]

Basically we don't know exactly how we want to change the thing, but have a feeling something will be needed. Re: composition or delegation, it always happens, but I'm not sure that's a concrete issue yet. We could introduce an IsActivity interface, but I don't see anywhere in the current GWT code we would actually call it. People implement their own ActivityMappers by hand, so they could use that convention themselves.

Sounds like there aren't super strong feelings on this, so today for 2.1.1 I'm inclined to 

• drop the interface
• rename AbstractActivity to Activity
• document as being forbidden from developing any non-trivial behavior
• and perhaps document the intent to retroactively introduce an interface when it's had a chance to settle

Last passionate objections?

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[John Tamplin]

On Wed, Dec 8, 2010 at 2:17 PM, Ray Ryan <rj...@google.com> wrote:

Basically we don't know exactly how we want to change the thing, but have a feeling something will be needed. Re: composition or delegation, it always happens, but I'm not sure that's a concrete issue yet. We could introduce an IsActivity interface, but I don't see anywhere in the current GWT code we would actually call it. People implement their own ActivityMappers by hand, so they could use that convention themselves.

Sounds like there aren't super strong feelings on this, so today for 2.1.1 I'm inclined to 

• drop the interface
• rename AbstractActivity to Activity
• document as being forbidden from developing any non-trivial behavior
• and perhaps document the intent to retroactively introduce an interface when it's had a chance to settle

Last passionate objections?

I still feel like there is little cost in having the interface, which is what is used in the API, and a default implementation where any new methods added will get default behavior.  Then document that if you implement the interface but don't extend the default implementation, you will be broken by future updates. That lets users decide whether they care more about not being broken by updates or more about not having to extend a base class.

[Ray Ryan]

I hope that doesn't come across as having ignored Neil, John et al. I do prefer using interface + abstract class, but I don't really believe that people actually read JavaDoc, and I'm certain we need to mess with this interface just a bit more. 

[Philippe Beaudoin]

Just ran into an interesting little hack today. Basically, the
interface includes a method:

public void __do_not_implement_this_interface_extend_FooImpl_instead();

I'm far from convinced I like it, but it sure is right in your face in
case you don't read javadocs! ;)

Philippe

> --
> http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Thomas Broyer]

+1

Though I'm OK with the proposed (abstract)Activity and SimpleActivity (i.e. just make Activity an abstract class rather than an interface –with all methods being abstract– and rename AbstractActivity into SimpleActivity, rather than just renaming the current AbstractActivity to Activity, with the no-op methods)

[Ray Ryan]

Nnnnnnnnevermind. I think it's too late for me to make this not-terribly-popular change. It's already more widely adopted than I realized internally, so I have to assume that's even more true externally. I can't imagine such a break being well received.

(Yes, we're making more significant changes to RequestFactory in 2.1.1, but I suspect that has a lower adoption rate so far, and client side the impact is actually fairly minimal, except for the dropped UserInfo stuff.)

rjrjr

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[Thomas Broyer]

On Thursday, December 9, 2010 7:04:39 PM UTC+1, rjrjr wrote:

Nnnnnnnnevermind. I think it's too late for me to make this not-terribly-popular change. It's already more widely adopted than I realized internally, so I have to assume that's even more true externally. I can't imagine such a break being well received.

If you mean that the "temporary rollback" on trunk is finally not "temporary", then do not forget to roll it back on releases/2.1 too ;-)

[Patrick Julien]

On Thu, Dec 9, 2010 at 1:04 PM, Ray Ryan <rj...@google.com> wrote:
> Nnnnnnnnevermind. I think it's too late for me to make this
> not-terribly-popular change. It's already more widely adopted than I
> realized internally, so I have to assume that's even more true externally. I
> can't imagine such a break being well received.

I think it's for the best.

> (Yes, we're making more significant changes to RequestFactory in 2.1.1, but
> I suspect that has a lower adoption rate so far, and client side the impact
> is actually fairly minimal, except for the dropped UserInfo stuff.)

That and RequestFactory on the server is still pretty hellish effort wise.

[Thomas Broyer]

On Wednesday, December 8, 2010 9:16:16 PM UTC+1, PhilBeaudoin wrote:

Just ran into an interesting little hack today. Basically, the interface includes a method: 

public void __do_not_implement_this_interface_extend_FooImpl_instead(); 

I'm far from convinced I like it, but it sure is right in your face in case you don't read javadocs! ;)

interestingly, that's exactly what Hamcrest (used in many testing tools: JUnit, Mockito, etc.) does:

http://code.google.com/p/hamcrest/source/browse/trunk/hamcrest-java/hamcrest-core/src/main/java/org/hamcrest/Matcher.java?r=645 

[Philippe Beaudoin]

Exactly, this is where I ran into this.

> --
> http://groups.google.com/group/Google-Web-Toolkit-Contributors