Hi Bob,
I think it was a rather heavy handed approach
but it got the job done
quickly and it's use was only occasional.
Since then it's been used more and more
and I think we have listeners
being dropped and reattached far too often.
Yes, that is what I meant with my comments with the NotationProvider
function updateListener(). Here they are:
* Update the set of listeners based on the given event. <p>
*
* The default implementation just removes all listeners, and then
* re-initialises completely - this is method 1.
* A more efficient way would be to dissect
* the propertyChangeEvent, and only adapt the listeners
* that need to be adapted - this is method 2. <p>
*
* Method 2 is explained by the code below that is commented out.
* Method 1 is the easiest to implement, since at every arrival of an
event,
* we just remove all old listeners, and then inspect the current model,
* and add listeners where we need them. I.e. the advantage is
* that we only need to traverse the model structure in one location,
i.e.
* the initialiseListener() method.
*
Hence there are pros and cons for both methods!
The main advantage for Method 1 is that we need to write a
initialiseListener() method anyhow, for when the Fig is loaded. And removing
all listeners is so easy to do, that writing the updateListeners() function
as follows is just plain easy:
cleanListener(listener, modelElement);
initialiseListener(listener, modelElement);
On the other hand, Method 2 requires the same code to be written, i.e. a
initialiseListener and a cleanListener(), but additionally an real
updateListener() that dissects the model change event.
Hence in most cases Method 1 is used, but there are some exceptions.
All this applies to NotationProviders, but is in fact the same for Figs.
The bottom line:
By dissecting the model change event we will be able to reduce the number of
listeners being dropped and reattached, but at the cost of dedicated code.
This extra code is difficult to write and maintain - hence a trade-off
between bugs and loss of performance.
I choose for loss of performance, except maybe for the simplest cases where
it matters most.
Regards,
Michiel
----- Original Message -----
From: "Bob Tarling" <[EMAIL PROTECTED]>
To: <[email protected]>
Sent: Saturday, April 12, 2008 8:10 PM
Subject: [argouml-dev] Use of FigNodeModelElement.updateListeners
Probably best to split off this subject.
I *think* that updateListeners was introduced when the facility was
introduced to drag an existing association was one class to another.
It drops the listeners from the Fig to the model elements and then
rebuilds them.
I think it was a rather heavy handed approach but it got the job done
quickly and it's use was only occasional.
Since then it's been used more and more and I think we have listeners
being dropped and reattached far too often.
Bob.
---------- Forwarded message ----------
From: Christian López Espínola <[EMAIL PROTECTED]>
Date: 12 Apr 2008 17:51
Subject: Re: [argouml-dev] modelChanged in Figs should invokeLater
To: [email protected]
Hi all,
As Michiel points, we shouldn't abuse of the AWT thread.
updateListeners is thread-safe, so it can be called in any thread
without problems.
I'd like to update Bob's solution as below.
BTW, I agree with Bob that we should investigate about calling
updateListeners. Maybe I'm missing something, but what is the reason
of
updateListeners(getOwner(),getOwner())? In ModelElement, updateListeners
has:
protected void updateListeners(Object oldOwner, Object newOwner) {
if (oldOwner == newOwner) {
return;
}
Shouldn't this call be removed or I'm missing something?
On Sat, Apr 12, 2008 at 6:05 PM, Bob Tarling <[EMAIL PROTECTED]> wrote:
> Hmmm, I suspect that we're doing far too much when we get a model
> event anyway, I've never been sure about things like
> FigNodeModelElement.updateListeners being called so frequently. I
> think that needs some investigation.
>
> But if we are aware that there are certain things that should happen
> in different threads then I think it would be useful to separate
> methods for handling each thread in a consistent way.
>
> e.g. in FigNodeModelElement
>
>
final protected void modelChanged(final PropertyChangeEvent mee) {
Runnable doWorkRunnable = new Runnable() {
public void run() {
modelChangedImpl(mee);
}
};
SwingUtilities.invokeLater(doWorkRunnable);
updateNotation();
updateListeners(); <<<
}
>
> /**
> * Alert the notation subsystem that change has taken place in the
model
> */
> protected void updateNotation(PropertyChangeEvent event) {
> // Here goes the notation code previously in modelChanged
> }
>
> /**
> * Guaranteed to execute in AWT thread to update the layout of the
> Fig composition
> * as a result of a change in the model.
> */
> protected void modelChangedImpl(PropertyChangeEvent event) {
> // Here goes the other code previously in modelChanged
>
>
> }
>
>
>
>
> On 12/04/2008, Bob Tarling <[EMAIL PROTECTED]> wrote:
> > Is it not possibel that updateListeners and
> > NotationProvider.updateListener is ever called from the AWT thread
> > also? So we just move the problem somewhere else.
> >
> > Is there actually an advantage of having those calls on the MDR
> > thread? Is it a performance issue to take work away from the AWT
> > thread?
> >
> > If not then wouldn't it be safer to make sure that everything in that
> > method is on AWT thread by my suggested change to
> > FigNodeModelElement/FigEdgeModelElement.
> >
> >
> > Bob,
> >
> >
> >
> >
> > On 12/04/2008, Michiel van der Wulp <[EMAIL PROTECTED]> wrote:
> > > Hi Bob, Christian,
> > >
> > > The current code is as follows:
> > >
> > > protected void modelChanged(final PropertyChangeEvent mee) {
> > > super.modelChanged(mee);
> > > if (mee instanceof AddAssociationEvent
> > > || mee instanceof RemoveAssociationEvent
> > > || mee instanceof AttributeChangeEvent) {
> > > Runnable doWorkRunnable = new Runnable() {
> > > public void run() {
> > > renderingChanged();
> > > updateListeners(getOwner(), getOwner());
> > > notationProvider.updateListener(
> > > FigClassifierRole.this, getOwner(), mee);
> > > damage();
> > > }
> > > };
> > >
> > > SwingUtilities.invokeLater(doWorkRunnable);
> > > }
> > > }
> > >
> > >
> > > But, since the both updateListener calls only are intended
to query the
> > > model, and do not impact the graphic representation, it should be
as
> > > follows:
> > >
> > > protected void modelChanged(final PropertyChangeEvent mee) {
> > > super.modelChanged(mee);
> > > if (mee instanceof AddAssociationEvent
> > > || mee instanceof RemoveAssociationEvent
> > > || mee instanceof AttributeChangeEvent) {
> > > Runnable doWorkRunnable = new Runnable() {
> > > public void run() {
> > > renderingChanged();
> > > damage();
> > > }
> > > };
> > > updateListeners(getOwner(), getOwner());
> > > notationProvider.updateListener(
> > > FigClassifierRole.this, getOwner(), mee);
> > > SwingUtilities.invokeLater(doWorkRunnable);
> > > }
> > > }
> > >
> > > IIRC, then this kind of seperating between threads has been
discussed
> > > between me and Tom.
> > > Please also read the comments with
> > > NotationProvider.updateListener().
> > >
> > > Regards,
> > > Michiel
> > >
> > >
> > >
> > >
> > >
> > > ----- Original Message ----- From: "Bob Tarling"
<[EMAIL PROTECTED]>
> > > To: <[email protected]>
> > > Sent: Saturday, April 12, 2008 1:48 AM
> > > Subject: [argouml-dev] modelChanged in Figs should invokeLater
> > >
> > >
> > >
> > > >
> > > > Christian came across a problem with concurrency in
FigClassifierRole
> > > > of the new sequence diagrams.
> > > >
> > > > The problem was that drawing of a message edge (user interaction
an
> > > > AWT thread) was effecting the Fig composition at the same time as
the
> > > > same Fig was receiving a model event which also resulted in a
change
> > > > to the Fig composition.
> > > >
> > > > Forcing the handling of the model event to the AWT thread
solved this.
> > > >
> > > > I'm thinking that this is probably something we should do in
general
> > > > in Figs to be safe
> > > >
> > > > Something like the following in FigNodeModelElement (and Edge)
> > > >
> > > > final protected void modelChanged(final PropertyChangeEvent
mee) {
> > > > Runnable doWorkRunnable = new Runnable() {
> > > > public void run() {
> > > > modelChangedImpl(mee);
> > > > }
> > > > };
> > > >
> > > > SwingUtilities.invokeLater(doWorkRunnable);
> > > > }
> > > >
> > > > protected void modelChangedImpl(PropertyChangeEvent
> > > mee) {
> > > > // Here goes the code previously in modelChanged
> > > > }
> > > >
> > > >
> > > > Sound reasonable?
> > > >
> > > > Bob.
> > > >
> > > >
> >
> ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail:
> > > [EMAIL PROTECTED]
> > > > For additional commands, e-mail: [EMAIL PROTECTED]
> > > >
> > > >
> > > >
> > > > --
> > > > No virus found in this incoming message.
> > > > Checked by AVG.
> > > > Version: 7.5.519 / Virus Database: 269.22.12/1373 - Release Date:
> > > 11/04/2008 9:17
> > > >
> > > >
> > > >
> > >
> > >
> >
> ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > > For additional commands, e-mail: [EMAIL PROTECTED]
> > >
> > >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
>
>
--
Regards,
Christian López Espínola
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
--
No virus found in this incoming message.
Checked by AVG.
Version: 7.5.519 / Virus Database: 269.22.13/1375 - Release Date: 12/04/2008
11:32
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
