Package Pane Browser Broken

The package pane browser is broken in all versions of 4.3 gamma. Selections in the first two panes result in blank class, category and method panes. The browser works if you select nothing in the first pane. I'll have look at it myself. It's about time I'm learned more about the inner workings of browsers and tool builder.

Thanks to everyone for all the work on 4.3. I'm looking forward to using the new functionality.

Glen Paling

On Fri, Dec 09, 2011 at 11:58:07AM -0800, glenpaling wrote:
Yes, it is definitely broken :(

Dave

On Fri, Dec 09, 2011 at 08:42:29PM -0500, David T. Lewis wrote:
Hmmm... it was broken in Squeak 4.2, but worked in Squeak 4.1. Evidently
the package browser is not getting a lot of heavy use ;-) Nevertheless it
is broken and a fix would be much appreciated!

Dave

On Fri, 9 Dec 2011 11:58:07 -0800 (PST)
glenpaling <[hidden email]> wrote:

> The package pane browser is broken in all versions of 4.3 gamma. Selections
> in the first two panes result in blank class, category and method panes. The
> browser works if you select nothing in the first pane. I'll have look at it
> myself. It's about time I'm learned more about the inner workings of
> browsers and tool builder.

See Inbox tools-cao.389.mcz for a possible quick fix.

Poking around I found that the PackagePaneBrowser was using only the category name
unqualified by the package name, e.g. bar rather than foo-bar.  The fix overrides in
PackagePandBrowser the category selecting method in Browser, and does what I believe
to be the appropriate concatenation.  I don't know if this is a tasteful way to
solve this, but it works for me.

--
Christopher Oliver <[hidden email]>

On Fri, 9 Dec 2011 11:58:07 -0800 (PST)

Scratch that.  My patch has some nasty bugs.  Look like I need to dig harder.

--
Christopher Oliver <[hidden email]>

On Fri, 9 Dec 2011 20:49:14 -0500
"David T. Lewis" <[hidden email]> wrote:
> Hmmm... it was broken in Squeak 4.2, but worked in Squeak 4.1. Evidently
> the package browser is not getting a lot of heavy use ;-) Nevertheless it
> is broken and a fix would be much appreciated!

I get a strong feeling this relates to:
   http://permalink.gmane.org/gmane.comp.lang.smalltalk.squeak.general/155606

Browser>>selectedSystemCategoryName provided a hook for subclasses to override.
Eliding this hook broke PackagePaneBrowser.  A hasty optimization oblivious to
Einstein's razor perhaps?

Mumble mumble,

--
Christopher Oliver <[hidden email]>

On 12 December 2011 23:36, Christopher Oliver
<[hidden email]> wrote:
Indeed, I suspected it was me who broke it.

I'd have fixed it, had I been able to find the PPB in the menu. I
poked around a bit, and then gave up. So (a) yes, I (probably?) broke
it  and (b) we obviously need loads more tests.

frank

On 13 December 2011 07:14, Frank Shearar <[hidden email]> wrote:
I forgot to add: thanks, Christopher, for the fix, and thanks, Dave,
for accepting it.

The whole CodeHolder/Browser/subclasses hierarchy is dreadfully
confused. Most things should not subclass Browser, a bunch of things
Browser does should be pushed up, and so on. I can't remember the
exact details - I did all that hacking back in March - but I'm pretty
sure I sent a braindump to squeak-dev.

frank

I'm late to the game here, my replies from my iPhone got stuck in list moderator limbo. Frank, the proximate cause of the bug was using 'selectedSystemCategory' rather than your new method 'selectedSystemCategoryName' in the 'classList' method. Making the replacement restores browser function and passes browser tests. These two method names are too similar so I changed selectedSystemCategoryName to to selectedPackageAndCategory. I successfully used the the browser yesterday for some editing but haven't done any further testing.

To be clear change

classList
        "Answer an array of the class names of the selected category. Answer an
        empty array if no selection exists."

        ^ self hasSystemCategorySelected
                ifFalse:
                        [self packageClasses]
                ifTrue: [systemOrganizer listAtCategoryNumber:
                        (systemOrganizer categories indexOf: self selectedSystemCategory asSymbol)]

to

classList
        "Answer an array of the class names of the selected category. Answer an
        empty array if no selection exists."

        ^ self hasSystemCategorySelected
                ifFalse:
                        [self packageClasses]
                ifTrue: [systemOrganizer listAtCategoryNumber:
                        (systemOrganizer categories indexOf: self selectedPackageAndCategory asSymbol)]


where selectedSystemCategoryName renamed as:

selectedPackageAndCategory

        "Answer the name of the selected system category or nil."

        self hasSystemCategorySelected
                ifFalse: [^nil].
        packageListIndex = 0
                ifTrue: [^ self selectedSystemCategory ].
        ^ self package , '-' , self selectedSystemCategory

On Tue, 13 Dec 2011 07:44:21 -0800 (PST)
glenpaling <[hidden email]> wrote:

> To be clear change
>
> classList

My worry here is that this changes only one use of mapping between packageXcategory and system category.  When I looked at the senders, browser makes several invocations, and likely all need to see this.  I did note that PackagePaneBrowser>>selectedSystemCategoryName was redundant, and have submitted an Inbox to delete this, and I've also submitted a patch to the unit test for PackagePaneBrowser to reflect that selectedSystemCategoryName returns a single symbol for the system category.  I suspect all this pain indicates that the PPB is a bit of a kluge as it creates a fiction, and the lie is hard to keep up over time.

--
Christopher Oliver <[hidden email]>

I doubt there will be any consequences outside of the PackagePaneBrowser since all the changes I propose are within it. Modifying its classList method to call selectedSystemCategoryName rather than selectedSystemCategory restored it to operation. Renaming the method is incidental, the only sender I could find is the classList method in PackagePaneBrowser. It's not redundant in PBB because it reconstitutes the Package-Category string.

Let's wait and hear what Frank has to say, they're his modifications.

I submit a change set to the inbox tonight.

Glen

On Tue, 13 Dec 2011 13:20:32 -0800 (PST)
glenpaling <[hidden email]> wrote:


> I submit a change set to the inbox tonight.

I already fixed this yesterday, and David put it in Trunk.  What's the matter?

--
Christopher Oliver <[hidden email]>

Change Morphic-cmm.523 as merged in Morphic-ul.524 breaks MulticolumnLazyListMorph.  The
rows for a McLLM are arrays of strings, and because LazyListMorph, as it is now, converts
these to strings, McLLM>>drawOn: dies with non-indexable.  I.e. first index gets the
character $# (the start of an array literal), and then deeper, we try to index it again.
Ugh!  Should McLLM just do its own thing, and not subclass LLM, or should we roll back
Muller's change on LLM?

Thoughts?

--
Christopher Oliver <[hidden email]>

Oh, I loaded from the trunk this morning and it was still broken....let me check again.

Sent from my iPhone

On 2011-12-13, at 17:22, Christopher Oliver <[hidden email]> wrote:

On Tue, 13 Dec 2011 17:35:52 -0500
"E. Glen Paling" <[hidden email]> wrote:

> Oh, I loaded from the trunk this morning and it was still broken....let me check again.
>

Do you have Tools-cao.390 in your change sets?  I tried the PPB both opening directly, and via the usual suspects after making PPB the default browser, and I didn't see any warts.

--
Christopher Oliver <[hidden email]>

On the topic of the PackagePaneBrowser, we have had an elegant sufficiency. I do not want to hear about this further until next Wednesday. That would be tomorrow plus one week.

Squeak 4.3 comes out next Tuesday and we who are involved in its release have more important things to do than accommodate an endless round on this topic. Granted I'm just the release manager, and if David T. Lewis  in his authority and wisdom wants to indulge this development, then I will bow to his authority.

But I'm tired of it. You are blithely unaware of a careful release schedule we have been following for six weeks.

I would admonish you to husband your improvements to the PackagePaneBrowser for a week and one day.

Chris Cunnington

On Tue, Dec 13, 2011 at 06:29:51PM -0500, Chris Cunnington wrote:
Fully agree. I knowingly bent the rules by moving the fix into trunk,
on the theory that this one was low risk and that it adequately addressed
a very visible problem. Further embellishments will have to wait until
the release cycle is complete, but are highly encouraged when contributed
to inbox :)

Thanks Chris!

Dave

2011/12/14 Chris Cunnington <[hidden email]>:
> and we who are involved in its release

Who is "we"?

> have more important things to do

Which are?

Just curious,
 Alex

On 13 December 2011 21:20, glenpaling <[hidden email]> wrote:
> I doubt there will be any consequences outside of the PackagePaneBrowser
> since all the changes I propose are within it. Modifying its classList
> method to call selectedSystemCategoryName rather than selectedSystemCategory
> restored it to operation. Renaming the method is incidental, the only sender
> I could find is the classList method in PackagePaneBrowser. It's not
> redundant in PBB because it reconstitutes the Package-Category string.
>
> Let's wait and hear what Frank has to say, they're his modifications.

I don't claim to be an expert on all things Browser; I'm just the last
person who hacked on it. I followed the convention that
#selectedFooName would return a string suitable for displaying in a
list, while #selectedFoo would return the entity itself. I tried, in
other words, to maintain a strict seperation between the view of
something and the thing itself. Relying on Foos to print themselves
appropriately inside what happens to be a PluggableListMorph today and
might be a RandomShinyMorph tomorrow seemed brittle.

As such, it might SEEM superfluous to have both #selectedFooName and
#selectedFoo, but it's not: other non-Foo parts of the UI should know
only about UI stuff.

frank

> I submit a change set to the inbox tonight.
>
> Glen
>
> --
> View this message in context: http://forum.world.st/Package-Pane-Browser-Broken-tp4178019p4192091.html
> Sent from the Squeak - Dev mailing list archive at Nabble.com.
>