Well, with all of the issues that have popped up, it would be premature to include the code as-is into 3.6. Oops.

Joshua

On Tue, Aug 12, 2003 at 08:26:14AM -0700, schwa@cc.gatech.edu wrote:

This changeset isn't just a clean-up, it fixes a rather large bug. When I talked about possible subclass issues, I was referring to classes that weren't explicitly tested in the SUnit tests, but have their own implementation of some of the add/remove methods. These should not hold up inclusion in 3.6 for several reasons.

First, they occur in classes that wouldn't cause distress to everyday Squeaking if problems turned up, such as ScreeningMorph and PianoRollMorph.

Second, I was tired last night, and was being cautious about the claims I was making on behalf of Andreas' code. Faced with the prospect of postponing this until 3.7, and refreshed after a night's sleep, I have looked again at the code in question more closely, and manually exercised it. None of the code is doing anything fancy. For example, PianoScrollMorph>>removedMorph: overrides the Morph method, but is almost an exact copy of the previously-misused and now-removed PianoScrollMorph>>privateRemoveMorph:. The closer examination and manual exercise turned up no problems.

Another reason is that the next Croquet release will probably be based on 3.6, and almost certainly not on 3.7a. Since hardware-accelerated 3D windows are what brought this bug to light in the first place, it would be nice to include this now rather than having to include it separately in Croquet.

Basically, I think that any risks posed by the changeset (which, upon closer examination, are very small), are outweighed by the immediate fix of an outstanding bug. The code cleanup is icing.

< I'm a bug-fixing machine! >

This post brought to you by the BugFixArchiveViewer, a handy tool that makes it easy to comment on proposed fixes and enhancements for Squeak. For more information, check out the Web page for the BugFixArchiveViewer project: http://minnow.cc.gatech.edu/squeak/3214

< I'm a bug-fixing machine! >

Hi Ned,

I'm pretty certain that this is a conflict with something else you have loaded. I just tried the following in a clean updated 3.6b:

(PopUpMenu labels: 'use the name MorphRemoval-ar1 instead abort loading overwrite the old one') startUpWithCaption: 'There is already a change set named MorphRemoval-ar. What do you want to do?'

And it looks just the way it should. It might be worthwhile to see if there are any conflicting changes between the removal CS and what else you may have loaded. Also, if that notification comes from a background process, all bets will be off ;-)

Cheers, - Andreas

-----Original Message----- From: squeak-dev-bounces@lists.squeakfoundation.org [mailto:squeak-dev-bounces@lists.squeakfoundation.org] On Behalf Of Ned Konz Sent: Tuesday, August 12, 2003 5:39 PM To: The general-purpose Squeak developers list Subject: [BUG][FIX][TEST] Morph adding/removing ( introduces layout bug )

This change set appears to introduce a layout bug.

Perhaps the ordering of the bounds calculations has been changed.

The attached image demonstrates the problem.

Ned Konz http://bike-nomad.com GPG key ID: BEEA7EFE

In a fresh, updated 5395 image, I did this:

Open a file list. Select your CS. Install it. Install it again (which causes the popup to appear).

Err ... I'm getting a "Sorry, this name is already used". What's up here?

Cheers, - Andreas

Hi Scott,

Can you drop the image you're using on the sqftp server or someplace else where I can have a look at it? I just tried with about a dozen 3.6 based images and have not seen the effect ever. Makes me wonder if *my* image is clean...

Cheers, - Andreas

-----Original Message----- From: Scott Wallace [mailto:scott.wallace@squeakland.org] Sent: Tuesday, August 12, 2003 6:43 PM To: Andreas Raab; ned@bike-nomad.com Cc: The general-purpose Squeak developers list Subject: RE: [BUG][FIX][TEST] Morph adding/removing ( introduces layout bug )

I get the "Sorry, this name is already used" wording too -- which would seem to suggest that Ned's test image may not quite be a *completely* vanilla 3.6b ;-) -- but even so, it *does* illustrate the layout bug that Ned reported:

Cheers,

-- Scott

At 6:22 PM +0200 8/12/03, Andreas Raab wrote:

In a fresh, updated 5395 image, I did this:

Open a file list. Select your CS. Install it. Install it again (which causes the popup to appear).

Err ... I'm getting a "Sorry, this name is already used". What's up here?

Joshua 'Schwa' Gargus wrote:

On Tue, Aug 12, 2003 at 06:22:11PM +0200, Andreas Raab wrote:

...

...

In a fresh, updated 5395 image, I did this:

Open a file list. Select your CS. Install it. Install it again (which causes the popup to appear).

Err ... I'm getting a "Sorry, this name is already used". What's up here?

I get the same message, but it looks similar to the .gif that Ned posted. I thought it might be because I tried in my harvesting image, but it also occured in a "clean" image (only a few very minor tweaks, such as to change the default ProjectViewMorph size). I'm downloading a 5395 zip right now...

Hmm, it still appears in a virgin Squeak3.6b-5395.image. (I see that Scott had the same problem). I downloaded this from: ftp://st.cs.uiuc.edu/Smalltalk/Squeak/3.6beta/Squeak3.6b-5395.zip

I'm seeing the same thing as Joshua/Scott/Ned with the partially drawn menu bar background, so it looks like Andreas may be the exception here... :-)

- Doug

With the MorphRemoval-ar.3.cs changeset loaded in a clean 5395 image.

Open an Objects tool. Drop a Star on it. Get a walkback.

12 August 2003 10:58:02 am

VM: unix - Squeak3.6beta of '4 July 2003' [latest update: #5373] Image: Squeak3.6beta [latest update: #5395]

UndefinedObject(Object)>>doesNotUnderstand: #displayWorld Receiver: nil Arguments and temporary variables: aMessage: a Message with selector: #displayWorld and arguments: #() Receiver's instance variables: nil

StarMorph(Morph)>>vanishAfterSlidingTo:event: Receiver: a StarMorph<Star>(1755) Arguments and temporary variables: aPosition: 179@539 evt: [444@472 dropEvent] aForm: Form(28x28x16) aWorld: nil startPoint: 444@472 endPoint: nil Receiver's instance variables: bounds: 430@458 corner: 458@486 owner: nil submorphs: #() fullBounds: 430@458 corner: 458@486 color: Color lightBlue extension: a MorphExtension (2666) [externalName = Star ] [isPartsDonor] [oth...etc... borderWidth: 1 borderColor: Color black vertices: #(447.5355339059328@475.5355339059328 446.212317420825@485.9680224666...etc... closed: true filledForm: nil arrows: #none arrowForms: #() smoothCurve: false curveState: nil borderDashSpec: nil handles: nil borderForm: nil

StarMorph(Morph)>>rejectDropMorphEvent: Receiver: a StarMorph<Star>(1755) Arguments and temporary variables: evt: [444@472 dropEvent] Receiver's instance variables: bounds: 430@458 corner: 458@486 owner: nil submorphs: #() fullBounds: 430@458 corner: 458@486 color: Color lightBlue extension: a MorphExtension (2666) [externalName = Star ] [isPartsDonor] [oth...etc... borderWidth: 1 borderColor: Color black vertices: #(447.5355339059328@475.5355339059328 446.212317420825@485.9680224666...etc... closed: true filledForm: nil arrows: #none arrowForms: #() smoothCurve: false curveState: nil borderDashSpec: nil handles: nil borderForm: nil

PartsBin(Morph)>>rejectDropEvent: Receiver: a PartsBin<parts>(2372) Arguments and temporary variables: anEvent: [444@472 dropEvent] Receiver's instance variables: bounds: 273@237.0 corner: 527.0@487.0 owner: an ObjectsTool<Objects>(2099) submorphs: #(an IconicButton(2118) an IconicButton(1924) an IconicButton(1344) ...etc... fullBounds: 273@237.0 corner: 527@487.0 color: (Color r: 0.86 g: 1.0 b: 0.86) extension: a MorphExtension (2869) [externalName = parts ] [other: (dropEnabl...etc... borderWidth: 1 borderColor: (Color r: 0.861 g: 1.0 b: 0.722) presenter: nil model: nil cursor: 1 padding: 3 backgroundMorph: nil turtleTrailsForm: nil turtlePen: nil lastTurtlePositions: nil isPartsBin: nil autoLineLayout: nil indicateCursor: nil resizeToFit: nil wantsMouseOverHalos: nil worldState: nil griddingOn: nil

--- The full stack --- UndefinedObject(Object)>>doesNotUnderstand: #displayWorld StarMorph(Morph)>>vanishAfterSlidingTo:event: StarMorph(Morph)>>rejectDropMorphEvent: PartsBin(Morph)>>rejectDropEvent: - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - MorphicEventDispatcher>>dispatchDropEvent:with: MorphicEventDispatcher>>dispatchEvent:with: PartsBin(Morph)>>processEvent:using: PartsBin(PasteUpMorph)>>processEvent:using: MorphicEventDispatcher>>dispatchDropEvent:with: MorphicEventDispatcher>>dispatchEvent:with: ObjectsTool(Morph)>>processEvent:using: MorphicEventDispatcher>>dispatchDropEvent:with: MorphicEventDispatcher>>dispatchEvent:with: PasteUpMorph(Morph)>>processEvent:using: PasteUpMorph>>processEvent:using: PasteUpMorph(Morph)>>processEvent: HandMorph>>sendEvent:focus:clear: HandMorph>>sendEvent:focus: HandMorph>>dropMorph:event: [] in HandMorph>>dropMorphs: Array(SequenceableCollection)>>reverseDo: HandMorph(Morph)>>submorphsReverseDo: HandMorph>>dropMorphs: HandMorph>>handleEvent: HandMorph>>processEvents [] in WorldState>>doOneCycleNowFor: Array(SequenceableCollection)>>do: WorldState>>handsDo: WorldState>>doOneCycleNowFor: WorldState>>doOneCycleFor: PasteUpMorph>>doOneCycle [] in Project class>>DoIt [] in BlockContext>>newProcess

On Tuesday 12 August 2003 11:19 am, Andreas Raab wrote:

Good catch. HandMorph>>dropMorph:event: was one of the few places which used #privateRemoveMorph: without setting the owner to nil afterwards.

While the symptom would be trivial to fix (just use "anEvent hand world" in #slideBackToFormerSituation:) there is a good question about whether the morph should be really "out of the world" during the drop.

I don't think it should be. If it's visible, it should be in the world.

I'm not certain about this ... not at all. On one hand it seems as if things should continue to work if we just fix the above problem but conceptually, it sounds very, very wrong. While the morph is "falling down" it should most certainly be in the world (where would it fall if it weren't???) and it would (quite implicitly) end up in the right place (the world) if noone handles that drop. On the other hand, I have no idea what other implicit assumptions about dropping is in the code now, so I simply don't know if anything is going to break.

I'm not sure why the Hand wants to get rid of the dropping morph so soon. After all, the default behavior on drop is to change ownership. And if the drop is rejected, the Hand deletes the morph.

If you just comment out the removeMorph: line in HandMorph>>dropMorph:event: it still seems to work OK.

Though I have to test this some more.

On Tuesday 12 August 2003 12:33 pm, Andreas Raab wrote:

...

If you just comment out the removeMorph: line in HandMorph>>dropMorph:event: it still seems to work OK.

Yes, until you make a mistake in your drag and drop code. The reason why the hand tries to get rid of the morph is only because it makes it much more robust. For example, try to drop some guy who has an error in #wantsToBeDroppedInto: - this will screw the system.

So why not just wrap the call to #dropMorph:event: in an exception handler that just deletes the morph on an exception?

Something like:

HandMorph>>dropMorphs: anEvent "Drop the morphs at the hands position" self submorphsReverseDo:[:m| "Drop back to front to maintain z-order" [ self dropMorph: m event: anEvent ] ifCurtailed: [ self removeMorph: m ]. ].

BTW, on an even larger scale I think that drag and drop shouldn't even be done by adding some morph "physically" to the hand. For me, the hand represents the mouse cursor so everything added to it, logically _is_ the mouse cursor. Dragging objects is something which (in my understanding) happens by having the morph being dropped "follow" the hand. But that's an issue for a different conversation ;-)

I'm doing something like that in Connectors during (the second half of) wiring.

A new, invisible Connector is grabbed by the hand.

When it's dropped, it becomes visible and its second end starts following the hand.

Then when the next mouse event happens (up or down), it stops following the hand.

Oh - now you tell us. :-)

Not that I mind too much - but everything in the system now seems to use hand "grabbing" for drag.

FWIW, I've had really good luck using little "tasks" for dragging, resizing, short duration gestures in other systems. You add a little shepherd object as an eventlistener and it does all the right stuff until some terminating condition (like mouse up or something).

On Tuesday, August 12, 2003, at 01:33 PM, Andreas Raab wrote:

BTW, on an even larger scale I think that drag and drop shouldn't even be done by adding some morph "physically" to the hand. For me, the hand represents the mouse cursor so everything added to it, logically _is_ the mouse cursor. Dragging objects is something which (in my understanding) happens by having the morph being dropped "follow" the hand. But that's an issue for a different conversation ;-)

Cheers,

• Andreas

If you get an Ellipse out of the parts bin, put it on the desktop, and try to get a halo, you don't see the halo till you move the morph.

Ah, yes, I understand what's happening now. I didn't read the comment about #addedOrRemovedSubmorph: carefully - it's not a notification at all; it's an invalidation message. Ho hum... So the notifications (#addedMorph: and #removedMorph:) really shouldn't do any invalidations at all because those have to be handled separately. Guess I'll have to revive #addedOrRemovedSubmorph: again.

BTW, this _still_ doesn't explain why the menus were screwed up. I'm pretty certain there is some deeper bug in this area but I think I'll work around that by simply re-introducing the same invalidation semantics.

Cheers, - Andreas

On Tuesday 12 August 2003 02:45 pm, Andreas Raab wrote:

Okay, here's the second version.

[snip]

• Morph>>addAllMorphs: and Morph>>addAllMorphs:after: have been

folded into #privateAddAllMorphs:atIndex: to keep the (needed) complexity in a single place.

The attached change set adds the reverse notification to the submorph when it's added to a new owner.

As I said before, this is useful in those cases (like in Connectors) where a Morph may be added in a number of different ways to a number of different kinds of owners, and it needs to respond to the addition.

I have attached a changeset that purports to fix the problem described below without wrecking anything else.

Joshua

On Wed, Aug 13, 2003 at 12:57:12AM -0400, Joshua 'Schwa' Gargus wrote:

Morphs that are picked up by the hand are sent #outOfWorld:, but are not sent #intoWorld: when dropped. This results in the original problem that started this whole thread (and possibly others): a 3D window is suspended when it is picked up, and acceleration is not restored when it is dropped.

It sure does pass the tests, though ;-)

Joshua

This is Andreas' v3 from 13 August, with the following fixes:

11 October (Ned Konz): Fixed Genie focus termination logic.

23 September (Ned Konz): Fixed problem with extra #noteNewOwner: notifications. Fixed problem with addMorph: variants when adding morphs that already are in submorphs.

I would very much like to see this approved for the update stream.

I have been living with it in all of my images for over a month and haven't found any problems yet.

< I'm a bug-fixing machine! >

This post brought to you by the BugFixArchiveViewer, a handy tool that makes it easy to comment on proposed fixes and enhancements for Squeak. For more information, check out the Web page for the BugFixArchiveViewer project: http://minnow.cc.gatech.edu/squeak/3214

< I'm a bug-fixing machine! >

< I'm a bug-fixing machine! >

This post brought to you by the BugFixArchiveViewer, a handy tool that makes it easy to comment on proposed fixes and enhancements for Squeak. For more information, check out the Web page for the BugFixArchiveViewer project: http://minnow.cc.gatech.edu/squeak/3214

< I'm a bug-fixing machine! >

| In old days, you used to be able to click on the Debug button of a | notifier, move the pointer in the stack list pane (but not clicking), | and press Alt-t to start debugging right away. This was convenient | when you put 'self halt'. Now, this operation raises an error. This | change set fixes the problem. |

This is a Single line change in Debugger>>checkContextSelection:

original: contextStackIndex = 0 ifTrue: [contextStackIndex _ 1] changed: contextStackIndex = 0 ifTrue: [self context StackIndex: 1 oldContextWas: nil].

This ensures correct initialisation. checkContextSelection is called on doStep, proceed, restart, send, all of them work fine afterwards.

< I'm a bug-fixing machine! >

This post brought to you by the BugFixArchiveViewer, a handy tool that makes it easy to comment on proposed fixes and enhancements for Squeak. For more information, check out the Web page for the BugFixArchiveViewer project: http://minnow.cc.gatech.edu/squeak/3214

< I'm a bug-fixing machine! >