Re: [e4-dev] Suggestion to restructure UIEvents to increase clarity and

[Dean Roberts <Dean_Roberts@xxxxxxxxxx>]

That is an excellent point.

I did an initial implementation of this
and found that the world did not work well.  I had thought that there
where other users of the constants that made presumptions that failed ...
IE they didn't juse use buildTopic.  It could also have been the issue
you are pointing out ... although presumably Eclipse would have rebuilt
all required files.

I have not had a chance to spend more
time on it.... Likely because I have not had to do any event stuff yet.
 I know, a pretty poor excuse.

Having to build a parallel structure
makes me cringe ... but as you point out it might be the only way forward
if we need to be compatible with previously compiled .class files.  And
it does save me having to look through all references to these constants
to find out which ones where using them directly and causing problems.

Do we have to be compatible with previously
compiled .class files ... I'm assuming the answer is going to be yes here.

Dean_Roberts@xxxxxxxxxx

Brian de Alwis <briandealwis@xxxxxxxxx>
Sent by: e4-dev-bounces@xxxxxxxxxxx

11/09/2011 06:50 PM

Please respond to
E4 Project developer mailing list <e4-dev@xxxxxxxxxxx>

To	E4 Project developer mailing list <e4-dev@xxxxxxxxxxx>
cc
Subject	Re: [e4-dev] Suggestion to restructure UIEvents to increase clarity        and performance

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
_______________________________________________
e4-dev mailing list
e4-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/e4-dev