Smalltalk › Usenets › Dolphin Smalltalk

Bug in SmalltalkToolShell class>>setAdditionalAccelerators:

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

7 messages Options

James Foster-3

Bug in SmalltalkToolShell class>>setAdditionalAccelerators:

Andy/Blair,

When uninstalling the package 'Intelli-Dolphin Professional' I get an error
during the uninitialize step when IdeaSpaceShell class (SmalltalkToolShell
class)>>setAdditionalAccelerators: is passed the value previously returned
by IdeaSpaceShell class (SmalltalkToolShell
class)>>>>getAdditionalAccelerators. It seems that it ought to be possible
to return the environment to the state it was in before. Also, the idea that
if the passed-in array is empty there is no update seems to miss the point
that maybe something that is present ought to be removed.

Comments?

James Foster

Blair McGlashan-4

Re: Bug in SmalltalkToolShell class>>setAdditionalAccelerators:

"James Foster" <[hidden email]> wrote in message
news:[hidden email]...
> Andy/Blair,
>
> When uninstalling the package 'Intelli-Dolphin Professional' I get an
> error during the uninitialize step when IdeaSpaceShell class
> (SmalltalkToolShell class)>>setAdditionalAccelerators: is passed the value
> previously returned by IdeaSpaceShell class (SmalltalkToolShell
> class)>>>>getAdditionalAccelerators. ...

What is the error? I can hazard a guess it is a nil DNU #isEmpty.

The bug here is in the add-on package, not in Dolphin. The package should
not be calling #setAdditionalAccelerators: and passing it the result of
#getAdditionalAccelerators. This is incorrect because
#getAdditionalAccelerators is allowed to return nil, and indeed does for
IdeaSpaceShell.

>...It seems that it ought to be possible to return the environment to the
>state it was in before.

Well it is, but it is the package author's job to participate in the
uninstall process correctly to ensure that happens.

>...Also, the idea that if the passed-in array is empty there is no update
>seems to miss the point that maybe something that is present ought to be
>removed.

I think you are misreading the statement:

additionalAccelerators := anArray isEmpty ifFalse: [anArray]

This will assign nil to the additionalAccelerators class instance variable
if the array is empty.

>
> Comments?
>

Again the bug here is in the extension package. Please do at least try to
eliminate such causes first, as it saves us the time of investigating issues
that turn out not to be our responsibility.

Regards

Blair

Tim M

Re: Bug in SmalltalkToolShell class>>setAdditionalAccelerators:

> The bug here is in the add-on package, not in Dolphin. The package should
> not be calling #setAdditionalAccelerators: and passing it the result of
> #getAdditionalAccelerators. This is incorrect because
> #getAdditionalAccelerators is allowed to return nil, and indeed does for
> IdeaSpaceShell.

As the author of said package - I will look into it when I get a spare
moment. I have to confess that I havne't done any uninstall testing in
a while - so possibly have got it wrong.

Tim

James Foster-3

Re: Bug in SmalltalkToolShell class>>setAdditionalAccelerators:

In reply to this post by Blair McGlashan-4

Blair,

Thanks for your explanation. I apologize for assuming that the get/set
methods were intended to be compatible. I'll try to be more careful.

James

"Blair McGlashan" <[hidden email]> wrote in message
news:[hidden email]...

> "James Foster" <[hidden email]> wrote in message
> news:[hidden email]...
>> Andy/Blair,
>>
>> When uninstalling the package 'Intelli-Dolphin Professional' I get an
>> error during the uninitialize step when IdeaSpaceShell class
>> (SmalltalkToolShell class)>>setAdditionalAccelerators: is passed the
>> value previously returned by IdeaSpaceShell class (SmalltalkToolShell
>> class)>>>>getAdditionalAccelerators. ...
>
> What is the error? I can hazard a guess it is a nil DNU #isEmpty.
>
> The bug here is in the add-on package, not in Dolphin. The package should
> not be calling #setAdditionalAccelerators: and passing it the result of
> #getAdditionalAccelerators. This is incorrect because
> #getAdditionalAccelerators is allowed to return nil, and indeed does for
> IdeaSpaceShell.
>
>>...It seems that it ought to be possible to return the environment to the
>>state it was in before.
>
> Well it is, but it is the package author's job to participate in the
> uninstall process correctly to ensure that happens.
>
>>...Also, the idea that if the passed-in array is empty there is no update
>>seems to miss the point that maybe something that is present ought to be
>>removed.
>
> I think you are misreading the statement:
>
> additionalAccelerators := anArray isEmpty ifFalse: [anArray]
>
> This will assign nil to the additionalAccelerators class instance variable
> if the array is empty.
>
>>
>> Comments?
>>
>
> Again the bug here is in the extension package. Please do at least try to
> eliminate such causes first, as it saves us the time of investigating
> issues that turn out not to be our responsibility.
>
> Regards
>
> Blair
>

Tim M

Re: Bug in SmalltalkToolShell class>>setAdditionalAccelerators:

In reply to this post by Blair McGlashan-4

Hi Blair,

> The bug here is in the add-on package, not in Dolphin. The package
> should not be calling #setAdditionalAccelerators: and passing it the
> result of #getAdditionalAccelerators. This is incorrect because
> #getAdditionalAccelerators is allowed to return nil, and indeed does
> for IdeaSpaceShell.

I can obviously fix this in Intelli-Dolphin (and do an isNil check) but this
implementation feels a bit cheap. Why would a getter be allowed to return
a value that the setter cannot handle?

I feel that that you should log this as a bug - and not return null - but
an empty collection. Its certainly non-inuititive (although it has an easy
fix, which I have applied to Intelli-Dolphin).

Tim

Blair McGlashan-4

Re: Bug in SmalltalkToolShell class>>setAdditionalAccelerators:

"macta" <[hidden email]> wrote in message
news:[hidden email]...

> Hi Blair,
>
>
>> The bug here is in the add-on package, not in Dolphin. The package
>> should not be calling #setAdditionalAccelerators: and passing it the
>> result of #getAdditionalAccelerators. This is incorrect because
>> #getAdditionalAccelerators is allowed to return nil, and indeed does
>> for IdeaSpaceShell.
>
>
> I can obviously fix this in Intelli-Dolphin (and do an isNil check) but
> this implementation feels a bit cheap. Why would a getter be allowed to
> return a value that the setter cannot handle?
>

They are not getters/setters in the Java sense, but part of the private
implementation. The public "getter" and "setter" are #additionalKeyBindings
and #additionalKeyBindings: respectively. It's a necessary for the
implementation that #getAdditionalAccelerators: return nil. If you take a
look at it you'll see why. #setAdditionalAccelerators: should only ever be
called from the public #additionalKeyBindings: "setter".

> I feel that that you should log this as a bug - and not return null - but
> an empty collection. Its certainly non-inuititive (although it has an easy
> fix, which I have applied to Intelli-Dolphin).

Wouldn't it be easier (and more robust) to just use the public accessors
provided (and tested) for the purpose?

Blair

Tim M

Re: Bug in SmalltalkToolShell class>>setAdditionalAccelerators:

Hi Blair,

> They are not getters/setters in the Java sense, but part of the
> private implementation. The public "getter" and "setter" are
> #additionalKeyBindings and #additionalKeyBindings: respectively. It's
> a necessary for the implementation that #getAdditionalAccelerators:
> return nil. If you take a look at it you'll see why.

You got me - yes I've been doing some java again for the last few weeks and
its turned my brain backwards ;-)

Taking a closer look - I see what you mean, and have reworked how I do the
whole thing (I had made a bit of a mess - and hadn't noticed I was using
those private methods).

Tim