I was just writing some event code, and again cringed at the buildTopic(), and remembered Dean's email on simplifying the approach. I still think this is a worthy change.
I wondered about backwards compatibility — if we changed the attribute definitions to incorporate the topic directly, such as UIEvents.UIElement.VISIBLE, are we guaranteed that all existing compiled classes that referenced UIEvents.UIElement.VISIBLE would have had the string inlined? Or should we just define a parallel set of strings (e.g., EMF-esque like UIEvents.UIElement_VISIBLE).
Brian. On 22-Sep-2011, at 3:30 PM, Dean Roberts wrote: While trying to understand UIEvents and
how the topics are built up for subscribe operations I found the buildTopic()
methods a little cumbersome and a barrier to understanding what was really
going on.
I would like to make a suggestion that
I think would
1)
Make the code look cleaner
2)
Make the concepts clearer
3)
Improve run time performance (albeit marginally)
For illustration purposes, here is an
example of an interface definition from UIEvents for the UIElement model
object generated by GenTopic
public
static
interface
UIElement {
public
static
final
String TOPIC
= UIModelTopicBase
+ "/ui/UIElement";
//$NON-NLS-1$
public
static
final
String ACCESSIBILITYPHRASE
= "accessibilityPhrase";
//$NON-NLS-1$
public
static
final
String CONTAINERDATA
= "containerData";
//$NON-NLS-1$
public
static
final
String CURSHAREDREF
= "curSharedRef";
//$NON-NLS-1$
public
static
final
String ONTOP
= "onTop";
//$NON-NLS-1$
public
static
final
String PARENT
= "parent";
//$NON-NLS-1$
public
static
final
String RENDERER
= "renderer";
//$NON-NLS-1$
public
static
final
String TOBERENDERED
= "toBeRendered";
//$NON-NLS-1$
public
static
final
String VISIBLE
= "visible";
//$NON-NLS-1$
public
static
final
String VISIBLEWHEN
= "visibleWhen";
//$NON-NLS-1$
public
static
final
String WIDGET
= "widget";
//$NON-NLS-1$
}
To subscribe to an event that tracks
a UIElements visible change you would write
eventBroker.subscribe(UIEvents.buildTopic(UIEvents.UIElement.TOPIC,
UIEvents.UIElement.VISIBLE),
visibilityHandler);
To subscribe to all changes to UIElement
you would write
eventBroker.subscribe(UIEvents.buildTopic(UIEvents.UIElement.TOPIC),
visibilityHandler);
I think it would be much nicer to write,
for the first case,
eventBroker.subscribe(UIEvents.UIElement.VISIBLE),
visibilityHandler);
or, for the second case,
eventBroker.subscribe(UIEvents.UIElement.ALL),
visibilityHandler);
To make this possible I propose that
we would generate the UIElement interface definition like this.
public
static
interface
UIElement {
public
static
final
String ALL=
UIModelTopicBase
+ "/ui/UIElement/*";
//$NON-NLS-1$
public
static
final
String VISIBLE
= UIModelTopicBase
+ "/ui/UIElement/"
+ "visible";
//$NON-NLS-1$
...
}
This would allow us to delete two of
the buildTopic() methods. We could leave the 3 argument version of
buildTopic() to retain construction of topics that filter based on event
type ( although nobody calls this internally at this time so do we really
need it?).
The code would also be more efficient
as we would be removing method calls that do string concatenation with
simple static references. In fact, I believe the compiler will just
in line the references at compile time.
Although I would be a fan of simply
making these changes and requiring consumers to make a few small simplifying
changes, we could provide a stepping stone by regenerating the new shape,
deprecating the buildTopic() methods and all the TOPIC fields. We
would then change the implementation of the buildTopic methods to return
the appropriate constant directly without concatenating.
Admittedly there is higher priority
work to be done but since this class is essentially API, if we don't change
it now, we will never be able to make the change. And I think the
clarity in the subscribe statements make the change worth it. This
change seems to fit well with the Eclipse 4 mantra that simple things should
be simple.
I also believe the effort required to
change GenTopic to generate the new shape is small, and I volunteer to
create the patch. Admittedly changing the calling code to use the
new shape is more wide spread, but a very simple task. Although an
actual committer probably wants to do that work as it would be a LOT of
patches to apply :-)
What does the community think? Does
it make sense? Am I totally off base?
Dean_Roberts@xxxxxxxxxx
_______________________________________________ e4-dev mailing list e4-dev@xxxxxxxxxxx https://dev.eclipse.org/mailman/listinfo/e4-dev
|