OAIDEExtensions queryBlock bug and technique comments

I like the OAIDEExtensions class, this will help me better integrate my
goodies.  However I noticed that the queryBlock: does not seem to function
properly in extendClassCommentMenu:.  If you open a SystemBrower and make
sure no class is selected (by unselecting any selected packages) and then go
to the class comment tab you will see the Emit class Layout Description
option is still enabled.  I also tried using the same technique with a
different test in the queryBlock, and it did not disable either.

This next item may not be a bug, just something to be aware of.  When adding
a menu item to the menubar of a window (not a context menu) with an
accelerator that accelerator will not automatically be registered with the
window.  It must also be added to the combinedAccelerator table of the
window as well.

Chris

Hi Chris,

> sure no class is selected (by unselecting any selected packages) and then
go
> to the class comment tab you will see the Emit class Layout Description
> option is still enabled.  I also tried using the same technique with a
> different test in the queryBlock, and it did not disable either.

I found it necessary to manually set the receiver in my queryBlocks, which I
think has a similar function to true being returned from a presenter's
#queryCommand: method.

For example, in the method OAIDEExtensions class>>extendClassCommentMenu:

Changing the queryBlock to:

  ...
  queryBlock: [:query |
    query receiver: aClassBrowserAbstract.
    query isEnabled: aClassBrowserAbstract hasClassSelected]
  ...

does disable the command when there is no selection.

FWIW: I have put up an IDEExtension class for SUnitBrowser at:
http://www.dolphinharbor.org/dh/projects/goodies/sunitBrowserIDE.html

Any feedback on this appreciated. It would be good to have a set of best
practices for IDE extensions. I believe Bill Schwab has suggested to not use
class initialization to activate extensions. The above package does use
class initialization, but if this is the consensus I will change it.

Thanks,
Steve
http://www.dolphinharbor.org

Steve,

> Any feedback on this appreciated. It would be good to have a set of best
> practices for IDE extensions. I believe Bill Schwab has suggested to not
use
> class initialization to activate extensions. The above package does use
> class initialization, but if this is the consensus I will change it.

I have indeed suggested not enabling extensions from class initialization.
But, I suspect that your approach will become the standard, and I want to
make things easy on users of my extensions w/o shooting myself in the foot
(more below), so, my plan is to add some kind of flag to Migrate so that
packages loaded by it can decide whether or not to enable extensions.  My
packages will check the flag and defer activation if they see that Migrate
loaded them; otherwise, they'll activate on load.  I think that will give
everybody what they want.

Ok, so why do I think enabling extensions on init is a bad idea?   Because
they make assumptions about the base system tools, and seem almost
guaranteed to fail in future versions of Dolphin.  By the time we're all
done extending the browsers, view composer, package browser, probably
workspaces and maybe even the debugger =:0, what will remain uncompromised
to fix extension bugs in the new image?  IMHO, the prudent solution is to
load packages into a new image, make a backup, and then activate the
extensions.  If they all work, you're done; if they don't, then restore the
backup and begin debugging.  The forementioned Migrate extension will likely
allow extensions to contribute an expression to be displayed in a workspace
after all packages are loaded, making them easy to activate when the user is
ready.

Another thing I found helpful: it's possible to write code that extends one
tool shell rather than all new instances of a particular tool, and that
would (will) be very helpful if (when!<g>) an extension breaks.

Have a good one,

Bill

--
Wilhelm K. Schwab, Ph.D.
[hidden email]

Hi Bill,

> (more below), so, my plan is to add some kind of flag
> to Migrate so that packages loaded by it can decide
> whether or not to enable extensions.  My packages will
> check the flag and defer activation if they see that Migrate
> loaded them; otherwise, they'll activate on load.

Will you use package scripts to do this?

> Ok, so why do I think enabling extensions on init is a bad idea?

I agree.

The SUnitBrowserIDE does not enable extensions when it is installed. Unlike
your approach, it does still register for the events, but by default it
handles the events by doing nothing. To enable the extensions, the user must
set a couple of publishedAspects to true via the SUnitBrowser's user
preferences.

For my own images, I regularly build new images using a set of scripts. For
the SUnitBrowserIDE, a script installs the package and then sets the
enable* publishedAspects to true, so it would be just as easy to switch to
the #enableExtensions method.

I wonder if a development system publishedAspect e.g. #enableAllExtensions
would be useful? You could use it as your flag, I could use it to initialize
my local enable* publishedAspects.

> By the time we're all done extending the browsers,
> view composer, package browser, probably
> workspaces and maybe even the debugger =:0.

:) I know what you mean, I am finding them extremely useful. It has replaced
my need to edit the browser resources and therefore makes it much easier to
build new images with customizations.

> what will remain uncompromised
> to fix extension bugs in the new image?

You still have any browsers that were open before the package was installed.

> Another thing I found helpful: it's possible to write
> code that extends one tool shell rather than all new
> instances of a particular tool, and that would (will) be
> very helpful if (when!<g>) an extension breaks.

Nice idea!
Thanks,
Steve

Steve Waring <[hidden email]> wrote in message
news:abci53$h7evk$[hidden email]...
> I found it necessary to manually set the receiver in my queryBlocks, which
I
> think has a similar function to true being returned from a presenter's
> #queryCommand: method.

Thanks.  I recall having had to do something similar in another case.  OA
should probably modify the OAIDEExtensions example to include this.

> FWIW: I have put up an IDEExtension class for SUnitBrowser at:
> http://www.dolphinharbor.org/dh/projects/goodies/sunitBrowserIDE.html
>
> Any feedback on this appreciated. It would be good to have a set of best
> practices for IDE extensions. I believe Bill Schwab has suggested to not
use
> class initialization to activate extensions. The above package does use
> class initialization, but if this is the consensus I will change it.

I concur on the best practices idea.  I am really glad we finally have an
easy way to extent the browsers.  For now I am using the initialize method
on my system only because that was the OA example.  However I can see how
this might not be the way to go in a distributed package.  What I used to do
was have a class side method called addContextMenu (it would actually edit
the view).  I could see how a similar concept could work.  The trick would
be to include example code in the Package comments for the user to evaluate.
The only concern might be a newbie not looking at the package comment.  I
suppose it could ask if someone wanted to add the extensions upon load.  But
that might put the kibosh on people who like to load packages in an
automated way.  I am not sure how I will activate the extensions in my
package yet.

Chris

Chris,

> automated way.  I am not sure how I will activate the extensions in my
> package yet.

FWIW, the scheme I came up with is -

I have defined a class, #IdeExtensionAbstract, with a couple of class side
helper methods.  For every add-on I add a subclass to the above (makes
uninstalling a lot easier) that contains the actual code to interface the
addon to the existing system.

The #IdeExtensionAbstract class contains an #install method that
automatically installs an extension when it's package is loaded. During
development however I can override #install in the specific subclass to
prevent the automatic installation - just in case it breaks everything.

Here are a couple of classes as a demo.

Object subclass: #IdeExtensionAbstract

IdeExtensionAbstract class>>extendedClasses
    ^OrderedCollection new

IdeExtensionAbstract class>>initializeAfterLoad
    self install.
    ^super initializeAfterLoad

IdeExtensionAbstract class>>install
    self extendedClasses do:
        [:each |
            each
                when: #viewOpened:
                send: #onBrowserOpened:
                to: self]

IdeExtensionAbstract subclass: #IdeMethodHistoryExtension

IdeMethodHistoryExtension class>>extendedClasses
    ^OrderedCollection new
        add: ClassBrowserShell;
        add: SystemBrowserShell;
        yourself

IdeMethodHistoryExtension class>>onBrowserOpened: aBrowser
    "initialization"


BTW. I use the class side #initializeAfterLoad, rather than simply using
#initialize, as the latter means that you would have to override #initialize
in each subclass (because the package loading ignores inherited #initialize
methods). This way seems slightly more complicated but, IMHO, is better in
practice.

--
Ian
Due to spamming the reply-to address may only be valid for the next few
days.  Use it to mail me _now_ if you want a longer term contact address.

Steve,

> > (more below), so, my plan is to add some kind of flag
> > to Migrate so that packages loaded by it can decide
> > whether or not to enable extensions.  My packages will
> > check the flag and defer activation if they see that Migrate
> > loaded them; otherwise, they'll activate on load.
>
> Will you use package scripts to do this?

I don't think so, but, things have a way of being more complicated than they
seem in the planning stages.  The plan is to use the class #initialize, but,
with Smalltalk at: to look for the Migrate loader, and do what it says if
present, otherwise, just enable the extension.  However...


> > Ok, so why do I think enabling extensions on init is a bad idea?
>
> I agree.
>
> The SUnitBrowserIDE does not enable extensions when it is installed.
Unlike
> your approach, it does still register for the events, but by default it
> handles the events by doing nothing. To enable the extensions, the user
must
> set a couple of publishedAspects to true via the SUnitBrowser's user
> preferences.

NICE!  I'll think about it some more, but, that seems simpler and just as
effective.  Migrate uses a .st file to build the new image, and it would be
easy enough to have another aspect of the loader to "throw the switch".  If
things got ugly, then one would be able to edit the script to be cautious
about extensions and have a stable new image from which to start debugging
the extensions.


> For my own images, I regularly build new images using a set of scripts.
For
> the SUnitBrowserIDE, a script installs the package and then sets the
> enable* publishedAspects to true, so it would be just as easy to switch to
> the #enableExtensions method.
>
> I wonder if a development system publishedAspect e.g. #enableAllExtensions
> would be useful? You could use it as your flag, I could use it to
initialize
> my local enable* publishedAspects.

Sounds good to me.



> > what will remain uncompromised
> > to fix extension bugs in the new image?
>
> You still have any browsers that were open before the package was
installed.

Typcially there aren't any at the stage of interest; but, Migrate could open
a browser and a workspace before doing much else.

Have a good one,

Bill

--
Wilhelm K. Schwab, Ph.D.
[hidden email]

Steve Waring wrote:

> FWIW: I have put up an IDEExtension class for SUnitBrowser at:
> http://www.dolphinharbor.org/dh/projects/goodies/sunitBrowserIDE.html
>
> Any feedback on this appreciated. It would be good to have a set of best
> practices for IDE extensions. I believe Bill Schwab has suggested to not use
> class initialization to activate extensions. The above package does use
> class initialization, but if this is the consensus I will change it.

Thank you Steve (and others on this thread) for making me aware of the
IDEExtesions. Very neat stuff. I see great promise for being able to integrate
goodies from various sources without needing to get into ongoing manual
maintenance of the integration.

One thing that bugs me in the SUnitBrowser Extensions was the overriding of
SUnitBrowser class publishedAspects. I know Steve has gone to the trouble of
replacing the original version (at the time the package was created) in the
postuninstall script of the package. But it doesn't seem this would work if
multiple parties supplied extensions to the same tool, would it? Both when
packages were loaded and when they were unloaded.

The overriding itself causes a dialog box to pop up when loading the package. I
did figure out how to include loading such a package from an automatic script
that I like to use to build a working image from the distribution. By doing:
    [PackageManager current install: 'pakage.pac']
        on: Package clashSignal
        do: [:ex | ex resume].
it can be loaded automatically. But, it also bothers me that it's ignoring
package clashes. Seems we should figure out a better way somehow.

I not really familiar with all the issues here. But it seems that either the
published aspects shouldn't be used for this type of thing, or we should make
some way that they could be extended dynamically. Right now (unless I'm missing
something) published aspects are rather "hard coded" mostly in the various class
methods. Do we need some sort of hook in the base system whereby published
aspects could be added and removed dynamically? Just a thought.

regards,
-Bill

BTW Steve, I just happened to notice, and was wondering if there was some reason
why in the SUnitBrowser Extension's queryBlocks you use:
    class allSuperclasses anySatisfy: [:each | each name = #TestCase]
rather than the simpler:
    class inheritsFrom: TestCase

-------------------------------------------
Bill Dargel            [hidden email]
Shoshana Technologies
100 West Joy Road, Ann Arbor, MI 48105  USA

Hi Bill,

I agree that replacing the published aspects of SUnitBrowser is not a nice
thing to do.

Another idea would be a base SmalltalkToolModel class. Like
SmalltalkToolShell, publishedAspects of its subclasses could appear in the
User Preferences. This would remove the need in this case to override or
modify the SUnitBrowserShell>>publishedAspects.

It would avoid the problems you identified of extensions stepping on each
others toes, and the package install/uninstall order problem if trying to
save/restore method source.

Any thoughts on this, or is there already a way to have a model's
publishedAspects included in the user preferences without being an
<installableSystemTool> ?

> BTW Steve, I just happened to notice, and was wondering if there was some
reason
> why in the SUnitBrowser Extension's queryBlocks you use:
>     class allSuperclasses anySatisfy: [:each | each name = #TestCase]
> rather than the simpler:
>     class inheritsFrom: TestCase

The reason was that it was a copy and paste from
ClassSelector>>queryCommand:. I assume the reason in that method was to
avoid a prerequisite, which is not necessary in the extension. Thanks for
pointing this out, I will clean it up by making your suggested change.

Thanks,
Steve
www.dolphinharbor.org

Steve Waring <[hidden email]> wrote in message
news:abhkl9$if0kr$[hidden email]...
> I agree that replacing the published aspects of SUnitBrowser is not a nice
> thing to do.
...
> Any thoughts on this, or is there already a way to have a model's
> publishedAspects included in the user preferences without being an
> <installableSystemTool> ?

One approach would be to programmatically modify the
publishedAspectsOfInstances method.  You would open the method text, find
'super publishedAspectsOfInstances', insert a newLine then insert the new
aspects.  Then compile the modified text.  The uninstall code could search
for the added aspects and remove them from the method.  I don't think there
would be much chance of collisions between goodies, and the added aspects
could be commented to note what added them.

Chris

Ian Bartholomew <[hidden email]> wrote in message
news:abg02t$hqcm5$[hidden email]...
...
> I have defined a class, #IdeExtensionAbstract, with a couple of class side
> helper methods.  For every add-on I add a subclass to the above (makes
> uninstalling a lot easier) that contains the actual code to interface the
> addon to the existing system.

That is a good idea.  If my goodie has a few classes it is sometimes hard to
decide which one is the best to handle the hook code.  In your case you
include one just to handle that.  The only problem is the need to have an
extra package for the abstract class.  I would either have to have all my
goodies (multiple packages) in one install file or require that people
download some kind of "Common" package.  I am not sure that I am as
organized as you are, so it might be a little harder for me to keep things
in sync. ;)

A cool idea might be if Dolphin included some kind of extension abstract
class like what you refer to.  It could even have some methods for adding or
removing published aspects (see my other posting in this thread) or other
more complex things.  Then all the IDE extensions would subclass it.  The
other benefit would be that all the IDE extensions would be in the same
place bellow that common class.  Just a thought.

Chris

"Steve Waring" <[hidden email]> wrote in message
news:abhkl9$if0kr$[hidden email]...
> ...
> Any thoughts on this, or is there already a way to have a model's
> publishedAspects included in the user preferences without being an
> <installableSystemTool> ?
>...

Not much is required to be an installable system tool - you need a couple of
methods, but they are just hooks for doing extra init/tear down and can be
(and usually are) empty. You can add any object you like to the user prefs
with SmalltalkSystem>>registerTool: - since implementing
<installableSystemTool> when it isn't needed is a bit tedious, and the
methods are usually empty, I'm going to do a protocol test for PL1 so that
one can #registerTool: anything at all that provides an implementation of
#publishedAspects.

Regards

Blair

Hi Blair,

> Not much is required to be an installable system tool - you need a couple
of
> methods, but they are just hooks for doing extra init/tear down and can be
> (and usually are) empty. You can add any object you like to the user prefs
> with SmalltalkSystem>>registerTool: - since implementing
> <installableSystemTool> when it isn't needed is a bit tedious, and the
> methods are usually empty, I'm going to do a protocol test for PL1 so that
> one can #registerTool: anything at all that provides an implementation of
> #publishedAspects.

Thanks for this.

I have uploaded a new version of the IDE Extension to:

http://www.dolphinharbor.org/dh/projects/goodies/sunitBrowserIDE.html

It removes the overwrite of SUnitBrowser's publishedAspects and registers
itself as an installableSystemTool.

The extensions are disabled by default and can be enabled in User
Preferences.

It also includes the changes suggested by Bill.

Thanks,
Steve
http://www.dolphinharbor.org