Smalltalk › Squeak › Squeak - Dev

[squeak-dev] Re: PluggableListMorph>>mouseUp: sets index to 0

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

3 messages Options

Jerome Peace

[squeak-dev] Re: PluggableListMorph>>mouseUp: sets index to 0

[squeak-dev] PluggableListMorph>>mouseUp: sets index to 0

Hi Ross,

You have found an integration bug.

See Mantis there are a bunch of reports on why
PluggableListMorph>>mouseUp: was changed.

http://bugs.squeak.org/view.php?id=1347
0001347: Doubleclick on list entries not working properly

see also 2452,0976, 5630.

PluggableListMorphPlus was written before the change
so it is still probably expecting the old behavior.
Andreas's position was that PLMPlus was meant to work with Tweak
and problems would have to be handled if used for regular squeak.
You have apparently found one of those problems.

Please add a report to mantis detailing how to come across your bug.
An sunit test the fails until the problem is fixed would be appreciated.
By the time you do all that you might be able to come up with a fix.
That also gets appreciated.

Hth,

Yours in curiosity and service, --Jerome Peace

***
>Ross Boylan ross at biostat.ucsf.edu
>Fri Jun 13 01:12:32 UTC 2008
>
>
>PluggableListMorph>>mouseUp:
>mouseUp: event
> "The mouse came up within the list; take appropriate action"
>
Specifically,
find out what row we were in.

yada, yada

if row is identical to the selected index
then see if we are to deselect it.
if we are then make sure we are not already deselected
and deselect.
else row and current selection are different.
change the selection.
note this may also cause a deselection
if cursor was over an empty row.(e.g. the last row)

> | row mdr |
> row := self rowAtLocation: event position.
> event hand hasSubmorphs ifFalse: [
> mdr := self mouseDownRow.
> self mouseDownRow: nil.
> mdr ifNil: [^self]].
> (self enabled and: [model okToChange])
> ifFalse: [^ self].
> "No change if model is locked or receiver disabled"
> row == self selectionIndex
> ifTrue: [(autoDeselect ifNil: [true]) ifTrue:[row == 0 ifFalse: [self
>changeModelSelection: 0 "!!!!"] ]]
> ifFalse: [self changeModelSelection: row].
> Cursor normal show
>
>I marked the spot with "!!!!". This leads to the index setter being
>called with argument 0 on the underlying model, which produces an error.

I believe the logic to be correct.

This is autodeselecting if you don't want that behavior set autodeselect to false.
Also the underlying model should handle deselection gracefully.

If its any help, it took me a while to puzzle out the original logic of this and to come up with the simplification of it. The method has too many responsibilities.

>
>I could code around this, but is this supposed to be the way things
>work?

Yes AFAIK.

>The context for this was that I had clicked on a row that was already
>selected. I *did* want the selection to be communicated back to the
>model; the row was selected by default, and the user click indicated it
>was the right selection.

Hmmm. Clicking would deselect when autodeselect is true.
Worse, with autodeselt false no amount of clicking
would send the selection to the model.
That is the right thing for models with toggling actions
but wrong for your needs.
You would have to bring up the list unselected.

>The PluggableListMorph (actually, PluggableListMorphPlus, but that's not
>where the method is implemented) was built by ToolBuilder.
>
>Ross
***

Ross Boylan-2

Re: [squeak-dev] Re: PluggableListMorph>>mouseUp: sets index to 0

On Thu, 2008-06-12 at 23:00 -0700, Jerome Peace wrote:

> [squeak-dev] PluggableListMorph>>mouseUp: sets index to 0
>
> Hi Ross,
>
> You have found an integration bug.
>
> See Mantis there are a bunch of reports on why
> PluggableListMorph>>mouseUp: was changed.
>
> http://bugs.squeak.org/view.php?id=1347
> 0001347: Doubleclick on list entries not working properly
>
> see also 2452,0976, 5630.
>
> PluggableListMorphPlus was written before the change
> so it is still probably expecting the old behavior.
> Andreas's position was that PLMPlus was meant to work with Tweak
> and problems would have to be handled if used for regular squeak.
> You have apparently found one of those problems.

Thanks for the info. PLMPlus may be meant for Tweak, but the
MorphicToolBuilder was the thing that produced it.

Ross
>
> Please add a report to mantis detailing how to come across your bug.
> An sunit test the fails until the problem is fixed would be appreciated.
> By the time you do all that you might be able to come up with a fix.
> That also gets appreciated.
>
As time permits.
Ross

> Hth,
>
> Yours in curiosity and service, --Jerome Peace
>
> ***
> >Ross Boylan ross at biostat.ucsf.edu
> >Fri Jun 13 01:12:32 UTC 2008
> >
> >
> >PluggableListMorph>>mouseUp:
> >mouseUp: event
> > "The mouse came up within the list; take appropriate action"
> >
> Specifically,
> find out what row we were in.
>
> yada, yada
>
> if row is identical to the selected index
> then see if we are to deselect it.
> if we are then make sure we are not already deselected
> and deselect.
> else row and current selection are different.
> change the selection.
> note this may also cause a deselection
> if cursor was over an empty row.(e.g. the last row)
>
> > | row mdr |
> > row := self rowAtLocation: event position.
> > event hand hasSubmorphs ifFalse: [
> > mdr := self mouseDownRow.
> > self mouseDownRow: nil.
> > mdr ifNil: [^self]].
> > (self enabled and: [model okToChange])
> > ifFalse: [^ self].
> > "No change if model is locked or receiver disabled"
> > row == self selectionIndex
> > ifTrue: [(autoDeselect ifNil: [true]) ifTrue:[row == 0 ifFalse: [self
> >changeModelSelection: 0 "!!!!"] ]]
> > ifFalse: [self changeModelSelection: row].
> > Cursor normal show
> >
> >I marked the spot with "!!!!". This leads to the index setter being
> >called with argument 0 on the underlying model, which produces an error.
>
> I believe the logic to be correct.
>
> This is autodeselecting if you don't want that behavior set autodeselect to false.
> Also the underlying model should handle deselection gracefully.
>
> If its any help, it took me a while to puzzle out the original logic of this and to come up with the simplification of it. The method has too many responsibilities.
>
> >
> >I could code around this, but is this supposed to be the way things
> >work?
>
> Yes AFAIK.
>
> >The context for this was that I had clicked on a row that was already
> >selected. I *did* want the selection to be communicated back to the
> >model; the row was selected by default, and the user click indicated it
> >was the right selection.
>
> Hmmm. Clicking would deselect when autodeselect is true.
> Worse, with autodeselt false no amount of clicking
> would send the selection to the model.
> That is the right thing for models with toggling actions
> but wrong for your needs.
> You would have to bring up the list unselected.
>
> >The PluggableListMorph (actually, PluggableListMorphPlus, but that's not
> >where the method is implemented) was built by ToolBuilder.
> >
> >Ross
> ***
>
>
>
>
>

Andreas.Raab

[squeak-dev] Re: PluggableListMorph>>mouseUp: sets index to 0

Ross Boylan wrote:

> On Thu, 2008-06-12 at 23:00 -0700, Jerome Peace wrote:
>> See Mantis there are a bunch of reports on why
>> PluggableListMorph>>mouseUp: was changed.
>>
>> http://bugs.squeak.org/view.php?id=1347
>> 0001347: Doubleclick on list entries not working properly
>>
>> see also 2452,0976, 5630.
>>
>> PluggableListMorphPlus was written before the change
>> so it is still probably expecting the old behavior.
>> Andreas's position was that PLMPlus was meant to work with Tweak
>> and problems would have to be handled if used for regular squeak.
>> You have apparently found one of those problems.
> Thanks for the info. PLMPlus may be meant for Tweak, but the
> MorphicToolBuilder was the thing that produced it.

PLMPlus was meant for Morphic not for Tweak.

Cheers,
- Andreas