This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 43332 - Toolbar button extension to InputOutput
Summary: Toolbar button extension to InputOutput
Status: RESOLVED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Window System (show other bugs)
Version: 4.x
Hardware: All All
: P2 blocker (vote)
Assignee: Milos Kleint
URL:
Keywords: API, API_REVIEW_FAST
Depends on:
Blocks: 48339 53359
  Show dependency tree
 
Reported: 2004-05-17 03:33 UTC by _ tboudreau
Modified: 2008-12-22 19:46 UTC (History)
2 users (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
Interface to be moved into org.openide.io/windows (preference?) (1.96 KB, text/plain)
2004-05-17 11:42 UTC, _ tboudreau
Details
Patch to expose the toolbar button API and also issue 48339 (allow listeners to specify if scrolling should stop) (7.44 KB, patch)
2004-10-01 02:27 UTC, _ tboudreau
Details | Diff
toolbar buttons API change diff (2.80 KB, patch)
2004-12-21 13:11 UTC, Milos Kleint
Details | Diff
new API diff (4.22 KB, patch)
2005-01-11 12:38 UTC, Milos Kleint
Details | Diff
output2 implementation (2.88 KB, patch)
2005-01-11 12:49 UTC, Milos Kleint
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description _ tboudreau 2004-05-17 03:33:12 UTC
I would like to add the attached interface to the openide io API.  
It is a single-method extension which allows the caller to 
provide an array of actions which should be represented in a 
toolbar on the left side of the output window.

Use cases:
 - Allow the user to kill a running process (potentially 
eliminating the execution window in the long term)
 - Allow the user to pause or perform other actions on a running 
process, as supported by the caller
 - Provide a convenient place for a per-tab close button which 
will be more discoverable than right clicking the tab
 - Support re-running a process (for example, over detailed 
search results)
 - Plenty of additional use cases - for examples, take a look at 
the left side of the output window in Intellij idea
Comment 1 Jesse Glick 2004-05-17 04:02:16 UTC
There is no attached interface. ?


Re. per-tab close button: should not be handled this way, IMHO; output
window should provided dedicated GUI for closing a tab if the current
context menu usage is not considered sufficient. Better for
consistency, and permits special GUI displays like the current window
system uses ('x' on tab itself).


Please note that I would probably *not* use such an API from the Ant
module; at least for now, the module has no need for specialized
output behavior beyond what the ExecutionEngine provides in
ExecutorTask, i.e. stop or finish with an int result.

To put it differently: from the perspective of the Ant module, this
API extension would be no more useful than a non-API change in the OW
GUI to make it possible to directly stop a process if the tab
corresponds (*) to an ExecutorTask from the execution engine (which
would require some cooperation between core/output and core/execution,
however).

On the other hand, while I have no current plans to do so, the Ant
module *could* have special pause/continue support at the task level
using build loggers, which could benefit from this API - or (again)
from an extension to the ExecutorTask API, with the precise GUI
behavior expected to be handled by something in core.

So consider this a +0 from me. If someone else (e.g. Search) actually
wants it, fine.


(*) This proposed API might be slightly more convenient for the Ant
module since it would not require that the exact InputOutput to which
output is sent be the same one that is passed to ExecutionEngine.
Currently the Ant module actually passes IO.NULL to EE and then makes
its own IO separately. I don't believe there is any particular reason
for this - probably it could make its IO first and then pass it to EE
- but I haven't tried.
Comment 2 Jaroslav Tulach 2004-05-17 09:04:33 UTC
No attached interface yet, maybe none is needed? Why not just add new
factory method?

InputOutput IOProvider.createIO (String name, Action[] toolbarActions);
Comment 3 _ tboudreau 2004-05-17 11:42:42 UTC
Created attachment 14905 [details]
Interface to be moved into org.openide.io/windows (preference?)
Comment 4 _ tboudreau 2004-05-17 11:53:52 UTC
Sorry, I've attached the interface.  The javadoc is probably relevant.

A factory method would work too - presumably anything that needs toolbar buttons would 
work up front.  Jarda, AFAIK it is still an API change - the new method would have to be 
added to IOProvider to be useful.

A minor API usability nit that could be solved by a new interface is for the interface to 
actually live in org.openide.io, rather than org.openide.windows, which would be 
somewhat more intuitive.  I'm not strongly attached to either approach.

> permits special GUI displays like the current window
> system uses ('x' on tab itself).

Hmm, CloseButtonTabbedPane may not be dead after all :-)

>Please note that I would probably *not* use such an API from the Ant
>module; 

? Its first purpose is to let you provide a Stop button.  Additional use cases have more to 
do with the rule of 3 at this point.



Comment 5 Jesse Glick 2004-05-17 16:25:41 UTC
Slight preference for the new factory method in IOProvider.

Strongly dislike the hardcoding of 5 as the maximum number of actions.
Seems completely gratuitous.

Please use org.openide.windows like the rest of the API. That's where
it is for historical reasons, so too bad.

Re. providing a stop button from the Ant module - please reread my
last comment where I explain that this proposed API is far more than
is needed to be able to stop an Ant process. The Execution View is
able to stop an Ant process *now*, with no special API beyond
ExecutorTask.

Again, suggest that this be put on hold until someone actually
requests it.
Comment 6 _ tboudreau 2004-05-17 17:17:43 UTC
Well, we *could* actually get rid of the execution window for D if this were implemented 
and used.  Or shortly thereafter if not for D.

Re doing it via the execution API, I don't see much reason to tie the output window into 
the execution API - certainly wouldn't be nice for anyone using the platform who didn't 
want the execution API.

Re hardcoding it to 5, it's either limit it, or have a lurking UI bug waiting to happen 
(someone *will* try to do something stupid with it at some point).  So better to just limit 
the way it can be used to what the UI will reasonably be able to display without running off 
screen.  Should the UI be changed, it would not be an incompatible change to remove the 
restriction.

Re supporting a single action vs. multiple actions, we know this will be needed in the 
future;  it doesn't make sense to me to add a setKillAction() in this release, then deprecate 
that and add a setToolbarActions() method in the next.
Comment 7 Jesse Glick 2004-05-17 17:30:22 UTC
Surely the tie between the InputOutput and ExecutorTask could be done
in such a way as to not force core/output to rely on core/execution.
(In fact the reverse is currently true.) There could be a core-private
communication channel permitting the execution engine to register a
stop handler for IO's which it happens to own.

Re. >5 actions "running off the end" - doesn't this depend on the size
of the GUI component anyway? If you resize it the number of
displayable actions will change. Anyway you could use a flow layout or
something that can display an unlimited number of buttons. Also,
anyone who is actually supplying some actions will surely try it to
see what it looks like, and if there are really too many buttons, they
will try to eliminate some. Don't see this as appropriate for the API.

Who suggested setKillAction()? Not me...
Comment 8 _ tboudreau 2004-05-20 18:47:13 UTC
The proposed fix for issue 40178 would definitely be a use case.
Comment 9 Jesse Glick 2004-05-20 19:06:10 UTC
To the contrary, I think #40178 should not need such an API - and in
fact could not use it. In that issue it is suggested only that you
have a tab with Copy, Clear, Discard (this or other) actions. These
are already supplied by the output window. In fact the module can't
implement Copy or Discard Other Tabs actions anyway - these have to be
part of the OW implementation.
Comment 10 _ tboudreau 2004-10-01 02:25:25 UTC
Note the web team has requested this as a way to do something or other with the tomcat 
log.
Comment 11 _ tboudreau 2004-10-01 02:27:37 UTC
Created attachment 17960 [details]
Patch to expose the toolbar button API and also issue 48339 (allow listeners to specify if scrolling should stop)
Comment 12 _ tboudreau 2004-10-01 02:36:26 UTC
The attached patch creates new classes

org.openide.io.InputOutput
and
org.openide.io.OutputListener

which extend their original counterparts in org.openide.windows adding the following:

InputOutput adds two methods:
setToolbarActions (Action[] actions) - set the toolbar actions, and
reset() - clear the output (I deprecated OutputWriter.reset(), which was implemented on 
the wrong class - what does it mean to clear the stderr but not the stdout, and why would 
you ever want to do that?)

 and OutputListener adds a method boolean isImportant().

I also deprecated InputOutput is/setFocusTaken, on the general principle that stealing 
focus is a horrible idea.  Assuming we also integrate the "blinking tabs" patch in 4.0, 
there's a fine way to get the users attention without hijacking the keyboard.

One minor option worth considering - is it worth making subclass/interfaces of IOProvider 
and OutputWriter, so all of the IO APIs are in a single (intuitively named) package?
Comment 13 Jaroslav Tulach 2004-10-04 16:58:37 UTC
NeedSpec: The diff does not seem to be against trunk or there are
missing source files.

Request: Do not create new interface, but final class.
Advice: Do not create new interface at all, just add new factory
method that takes the list of Actions to display. I do not see usecase
for the setter anyway.
Advice: Do not solve more unrelated problems at once - the ambiguity
of reset is more or less independent.
Comment 14 Milos Kleint 2004-11-09 09:09:09 UTC
Tim, you no longer work on this, correct? I'll take over I guess.
Comment 15 Milos Kleint 2004-12-21 13:11:24 UTC
Created attachment 19388 [details]
toolbar buttons API change diff
Comment 16 Milos Kleint 2004-12-21 13:12:27 UTC
diff of the api for the addition of toolbar buttons. (generally actions)

asslo changes entry included.
Comment 17 Milos Kleint 2004-12-21 13:25:33 UTC
none of the previous requests by yarda and co. were related to the
additional actions for output, but rather the other changes that were
put together. Do I have to wait for a week or is it ok to commit the
changes?
Comment 18 Jaroslav Tulach 2004-12-21 15:58:50 UTC
I hope you have a test of the new method somewhere. If yes, I agree
with the commit.
Comment 19 Jesse Glick 2004-12-21 16:13:39 UTC
Re. last diff:

1. Please always use the -u option to diff to make diffs more readable.

2. English: "falls back", not "fallbacks". (Similarly "checks in",
"logs out", "looks up", etc.)

3. Use {@link ...} when referring to another code member in a Javadoc
comment, or <code>...</code> if it is already linked to adequately.

4. "its abstract classes" not "it's abstract classes"

5. "actions" not "actoins"

6. The summary sentence in Javadoc for a method should always be
distinct from that of any other methods in the same class, so if you
have an overload with additional parameters, mention that, e.g.

"Gets a named output tab with additional actions displayed in the
toolbar."

7. Can additionalActions be empty? null? Currently Javadoc implies but
does not state that it can be empty but not null.

8. Don't forget to update manifest.mf to inc spec version.

9. Now to the API itself: are the Action's to be given any context
when they are invoked? Or is it expected that the supplied Action
instances already carry enough context to know what to do, based on
the particular output tab? I am still wondering what the use cases are
- would be nice to use it for Ant processes, but this functionality is
untested (in terms of whether it is actually possible to do everything
desired without hooking into the execution engine).
Comment 20 _ tboudreau 2004-12-21 18:18:43 UTC
> 9. Now to the API itself: are the Action's to be given any context
> when they are invoked?

At least as originally intended, it is assumed that an Action provided by a process that was 
producing output would know what it was supposed to operate on (i.e. you'd probably 
create a new instance of the Action class for each process).

I suppose we could do something like putValue (theInputOutputInstance);  other than that 
the output window doesn't have a lot of context to give it, and presumably the client 
would have to look up the InputOutput instance it was passed to find anything it actually 
could do something useful with.  Seems like little added value and a recipe for memory 
leaks to try to be clever about this - the client knows why it's supplying the Actions, so 
presumably it knows what they operate on at the time it supplies them.  Not much way for 
the output window to help there, really.

Up to Milos at this point, that's my 2c.
Comment 21 Jaroslav Tulach 2004-12-21 18:23:58 UTC
No putValue please. Use ContextAwareAction and pass there Lookup
containing everything you believe such action needs (if anything).
Comment 22 Milos Kleint 2004-12-22 09:01:23 UTC
I've corrected all my bad english, thanks Jesse.

re: context of the actions.
I share Tim's view. I believe the context comes from the user of the
API and the output winodw has nothing relevant to add there.

There's a problem with reusing old IOs though. Is a hypothetical case,
but still. Each call to the getIO() method with intend to reuse the IO
shall reset the previously used actions (because the context has most
probably changed). 
1. that's not the case in the current codebase 
2. Shall this be mentioned in the API docs or is it implementation detail?

re: tests. you mean test the API or implementation?
Comment 23 Jesse Glick 2004-12-22 14:34:54 UTC
Re. context - probably fine, just wanted to make sure it was clear. I
can't think of any useful context the output window itself could add.

Re. tests - I assume Yarda was referring to the implementation, since
there is nothing to test in the API.

Re. reusing an old IO tab - definitely it should clear the old actions
and replace them with any new actions you specify. Really the "reuse"
option of IOProvider was never a great idea, I think; one of many
strange API decisions long ago. Note that the Ant module does not use
this flag (passed newIO=true) - it manages its own tab pool. Currently
it resets reused tabs by calling io.getOut().reset() and
io.getErr().reset() and this is enough. I'm not sure what it would do
to reset reused tabs if they had "stop" buttons, but maybe nothing -
could keep the same actions and just bind them to new processes, maybe.
Comment 24 Jaroslav Tulach 2005-01-03 09:42:45 UTC
If reuse of tabs is not good idea, why not remove the newIO parameter?
Just have:
public InputOutput getIO(String name, Action[] additionalActions);

Yes, I meant tests of implementation. 
Comment 25 Milos Kleint 2005-01-03 13:01:12 UTC
yes, given that you consider reuse of tabs unimportant and actually
harmful, removing the boolean parameter makes sense. Keeping the
correct context of the actions is then fully under the api user control.

re: tests. do you have any suggestions what functionality should be
covered by the test? given that the only thing that is done is keeping
it around, setting it to the view hierarchy for the user to click on it?
Comment 26 Jaroslav Tulach 2005-01-03 13:19:54 UTC
I'd like to know how the button for the action is created. Does it
honor Presenter.Toolbar interface? Does it support rollover icon? If
any context is going to be provided to the action, does it honor
ContextAwareAction interface?
Comment 27 Milos Kleint 2005-01-03 13:35:22 UTC
java.swing.Action only is assumed with name and small icon properties
defined. The implemetation adds the JButton() with the Action to the
toolbar.
Comment 28 _ tboudreau 2005-01-05 09:01:46 UTC
> If reuse of tabs is not good idea, why  not remove the newIO parameter?

Reuse of tabs *is* a good idea; the current java module implementation just does tab 
management on its own because that's what the Term based code did.  There are two ways 
to reuse tabs:
 - getIO ("someNameI_alreadyUsed", false);
or 
 - someTabI_alreadyUsed.reset()

Arguably it might be better to have only one way to do it, and either one is equally good.  I 
somewhat favor keeping the name+boolean as it's sort of silly to ask clients to all write 
their own tab management code when we can do it perfectly well.

> I'd like to know how the button for the action is created. Does it
> honor Presenter.Toolbar interface? 

AFAIK we know Presenter.* is something we ought to kill for a variety of reasons relating 
to menus - why would we want to mislead people and make more of them use this 
interface?  And no, under no circumstances would I want any random component to be 
able to be stuffed into the output window - there are too many things that can go wrong 
when you open up the component hierarchy for someone to do anything they want to.  
Models like Action objects are much safer.  

> Does it support rollover icon? If
> any context is going to be provided to the action, does it honor
> ContextAwareAction interface?

AFAIK rollover icons can be done with standard Action.putValue() hints.

ContextAwareAction - what context?  The output window does not provide an activated 
node.  The only context you'd have is null or a dummy AbstractNode.  Nothing meaningful 
to get from it.


Comment 29 Jaroslav Tulach 2005-01-05 10:49:55 UTC
I agree or do not have opinion to most of Tim's comments, but "AFAIK
we know Presenter.* is something we ought to kill for a variety of
reasons relating to menus" because I do not know the variety of
reasons, please point me somewhere where I can get more information.
And "AFAIK rollover icons can be done with standard Action.putValue()
hints.", because by looking at
http://www.netbeans.org/source/browse/openide/src/org/openide/awt/Actions.java?rev=1.107&content-type=text/x-cvsweb-markup
and to AbstractButton.configureFromActionProperties it seems that you
have to use Actions.connect to do the rollover.

I have modest idea about context. Why not javax.swing.Document or
EditorCookie?
Comment 30 Milos Kleint 2005-01-05 13:28:49 UTC
someTabI_alreadyUsed.reset() has one distinct advantage or the boolean
reuse parameter.

allowing to reuse tab while specifying new actions multiplies the
possible workflows and makes the reuse to have unpredictable contract.

creating a new tab with actions is straightforward. context of the
actions is correct.

creating a new tab with no actions is also clear.

reusing a no-actions-tab is fine.

now what to do when reusing a tab with actions?
shall we keep the old actions? or reset the actions when no new ones
are specified?

the reset() of a tab I've once set the actions to gives a clear
message. I keep the actions and need to provide new context to them if
necessary.

-> i'm against the reuse boolean parameter when using the actions.
Comment 31 Jaroslav Tulach 2005-01-05 13:32:26 UTC
> now what to do when reusing a tab with actions?

io = getIO("X", new Action[] { ... }); // io with actions
io2 = getIO("X", true); // reuse it

imho: io2 should have the same actions as io.
Comment 32 _ tboudreau 2005-01-06 02:58:25 UTC
If we're really going to tweak this API, it would be a good idea to move the reset() method 
to InputOutput and deprecate OutputWriter.reset() - resetting *only* the stdout but not 
stderr makes no sense.
Comment 33 Milos Kleint 2005-01-06 09:20:46 UTC
I'd rather not do 2 things at a time.
Comment 34 Jesse Glick 2005-01-06 23:48:05 UTC
IMHO:

- Agree w/ Milos, meaning of 'reuse' too confusing when actions are
considered, and there is no clear use case for it anyway. Modules
which wish to reuse tabs, and wish to use actions, can figure out for
themselves how they want this to work. Don't complicate the OW with
this. Anyway the only proposed use of toolbars I know of is from the
Ant module, which never uses the 'reuse' parameter; it does its own
caching.

- Let's say for now that Presenter.Popup is not supported, nor
rollover icons, unless and until someone actually needs such things.
There is no problem to compatibly add such features later; no
signature change in the API, just adding some documentation.

- Keep discussion of reset() out of this, so we can get this issue
implemented and closed in a reasonable amount of time.

- Re. context for the actions: there is no use case for it, so don't
support it until there is. Again, such support could be added
compatibly at any time if we ever needed it (which I doubt).
Comment 35 Milos Kleint 2005-01-11 12:38:06 UTC
Created attachment 19605 [details]
new API diff
Comment 36 Milos Kleint 2005-01-11 12:38:47 UTC
ok, here is what I consider final API diff based on discussion here.
implementation diff follows.
Comment 37 Milos Kleint 2005-01-11 12:49:29 UTC
Created attachment 19606 [details]
output2 implementation
Comment 38 Milos Kleint 2005-01-11 12:49:54 UTC
reassigning
Comment 39 Milos Kleint 2005-01-13 12:42:55 UTC
integrated.