I've attached a patch to defect 5016 that fixes Christians problem. It clearly defines modelChanged as being for model thread and updateLayout as AWT thread.
Let me know if anyone can see any problems with this. Bob. On 13/04/2008, Bob Tarling <[EMAIL PROTECTED]> wrote: > I've found a way to remove the need for UI update in > FigClassifierRole.modelChanged. > > That doesn't remove the principle of this conversation though. I think > modelChanged in other Figs likely do much in the way of UI updates. > > > On 12/04/2008, Michiel van der Wulp <[EMAIL PROTECTED]> wrote: > > Hi Christian, > > > > I had made a thorough study about the listeners to model changes in the > > Figs, and amended/created most of this code. > > > > The call > > updateListeners(getOwner(),getOwner()); > > in the > > org.argouml.uml.diagram.sequence2.ui.FigClassifierRole > > class indeed does nothing. > > But I suspect that this was copied from the > > org.argouml.uml.diagram.sequence.ui.FigClassifierRole > > class, where it definitively does a lot! > > > > Maybe you better leave it in... > > > > Regards, > > Michiel > > > > > > > > ----- Original Message ----- From: "Christian López Espínola" > > <[EMAIL PROTECTED]> > > To: <[email protected]> > > Sent: Saturday, April 12, 2008 6:51 PM > > Subject: Re: [argouml-dev] modelChanged in Figs should invokeLater > > > > > > > > > > > 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 > > > > > > > > > > > > > -------------------------------------------------------------------------------- > > > > > > 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] > > > > > --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
