On Saturday, February 26, 2011 9:14:37 PM UTC+1, Philippe Beaudoin wrote: > > John, I really like this idea. It's well thought-out and the customization > hooks seem at once powerful and easy to use. I like the use of > <replace-with> to provide a simple way to perform an app-wide appearance > switch. Here are a couple of remarks: > > Sorry it's so long. Here is the *tl;dnr* version: > 1) The ButtonCell(DefaultAppearance.Resources) constructor can be > confusing, I think it should be dropped. > 2) Providing a custom DefaultAppearance.Resources lead to somewhat smelly > code. >
-1 on both the above (note: ClientBundle only reads the annotations from the method definition, it does not look at the overridden methods) > 3) Minor one-time style modifications are hard to make. > There's no "one-time style modification"; if you style an instance slightly differently, it's related to the context in which you use it, which means that you're likely to find that context a second time in your app (where you'd want/need to apply the same style modification); if not today, then tomorrow when you'll add a feature. It's similar to "I won't externalize this value –golden number– into a constant because it's only used once" (until it's used a second time, and you're still not sharing the value definition, so next time you'll want to change the value, you'll miss an instance). And it's actually not that hard to override a CssResource: 5 lines of Java and a CSS file. > 4) Contrary to Stephen, I think this design makes it hard to use > selector-based CSS that assumes the widget obeys a precise DOM structure. > 5) We should make sure GIN can be used to inject Appearance > into ButtonCell and Resources into Appearance. > 6) I agree with Thomas about: > - Not being sure making Appearance an abstract class is the right choice > I didn't give an opinion on the subject; I was just dismissing the argument. I believe in most cases Appearance classes would have only 'public' methods (and it could be documented as being how they would evolve, if ever: only adding 'public' methods), and specific subclasses would only override those methods and add non-public ones (private or protected). If a new 'public' method is added in a later version with a signature that conflicts with one you added in a subclass, because your method would have a lesser visibility, your code would no longer compile. If you need public to add methods, then it's probably a sign of code smell, and you could still detect conflicts by configuring your IDE to mark as errors an "overridden method missing the @Overrides annotation" (it wouldn't work if the class is a third-party lib you're using though; should there be an equivalent compiler switch?). Finally, I don't think there would be much inheritance among Appearance classes (even user-made ones), so the case of classes turning into interfaces as we saw in GWT 2.2 is unlikely to happen. In brief, I'm fine with abstract classes, and I'd be fine interfaces too; it's just that the argument about compatibility when the API evolves is to be taken with care. Widgets already have see a few methods added in their lifetime and I don't think anyone complained about a new method breaking their code; it's just how OOP works after all. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
