BUG: ClosedCommandDescription deepCopy Issue...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

BUG: ClosedCommandDescription deepCopy Issue...

Christopher J. Demers
ShellView<<updateCombinedAcceleratorTable causes a deepCopy of a
ClosedCommandDescription to be made, which makes a deepCopy of the arguments
and the receiver. This is probably not what a user would want, and can cause
a long delay in opening the window. One fix might be to implement special
deepCopy handling in ClosedCommandDescription and/or higher in the
hierarchy, or not use a deepCopy in updateCombinedAcceleratorTable.

A package with unit tests can be downloaded here:
http://www.mitchellscientific.com/downloads/tmp/MitSciUnitTestsForOA.pac

Chris


Reply | Threaded
Open this post in threaded view
|

Re: ClosedCommandDescription deepCopy Issue...

Blair McGlashan
"Christopher J. Demers" <[hidden email]> wrote in
message news:[hidden email]...
> ShellView<<updateCombinedAcceleratorTable causes a deepCopy of a
> ClosedCommandDescription to be made, which makes a deepCopy of the
arguments
> and the receiver. This is probably not what a user would want, and can
cause
> a long delay in opening the window. One fix might be to implement special
> deepCopy handling in ClosedCommandDescription and/or higher in the
> hierarchy, or not use a deepCopy in updateCombinedAcceleratorTable.
>
> A package with unit tests can be downloaded here:
> http://www.mitchellscientific.com/downloads/tmp/MitSciUnitTestsForOA.pac

Thanks Chris, I've grabbed that and assimilated it :-).

I think the deepCopy was put in place a long time ago when we thought that
applications might be routinely adding their own accelerator tables directly
from a shared resource, and that these would then need to be merged with the
table the system builds from the view menu bar definition in the view
resource. With the benefit of hindsight this mechanism looks rather
over-designed to me (and anyway, #deepCopy is almost always a bad idea
because it is unbounded and therefore unpredictable against externally
supplied objects), but to avoid a breaking change in 5.1.5 I think I would
be inclined to use #copy instead and then implement a #postCopy on
AcceleratorTable to make a shallow copy of the array it holds, pretty much
as the hotfix below.

Regards

Blair

------------------
"#1558"!

!AcceleratorTable methodsFor!

postCopy
 "Apply any final flourish to the copy that may be required in order to
ensure that the copy
 does not share non-shareable  state with the original. Answer the
receiver."

 super postCopy.
 accelerators := accelerators copy.
 ^self! !
!AcceleratorTable categoriesFor: #postCopy!copying!public! !

!ShellView methodsFor!

updateCombinedAcceleratorTable
 "Private - Merge the user set acceleratorTable with any accelerator keys
defined within
 menuBar, forming combinedAcceleratorTable."

 self releaseAccelerators.
 combinedAcceleratorTable := acceleratorTable notNil
    ifTrue: [acceleratorTable copy]
    ifFalse: [AcceleratorTable new].
 menuBar notNil ifTrue: [menuBar registerAcceleratorKeyIn:
combinedAcceleratorTable].
 combinedAcceleratorTable isEmpty ifTrue: [combinedAcceleratorTable := nil]!
!
!ShellView categoriesFor:
#updateCombinedAcceleratorTable!private!realizing/unrealizing! !