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.
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
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.
No attached interface yet, maybe none is needed? Why not just add new factory method? InputOutput IOProvider.createIO (String name, Action[] toolbarActions);
Created attachment 14905 [details] Interface to be moved into org.openide.io/windows (preference?)
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.
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.
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.
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...
The proposed fix for issue 40178 would definitely be a use case.
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.
Note the web team has requested this as a way to do something or other with the tomcat log.
Created attachment 17960 [details] Patch to expose the toolbar button API and also issue 48339 (allow listeners to specify if scrolling should stop)
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?
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.
Tim, you no longer work on this, correct? I'll take over I guess.
Created attachment 19388 [details] toolbar buttons API change diff
diff of the api for the addition of toolbar buttons. (generally actions) asslo changes entry included.
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?
I hope you have a test of the new method somewhere. If yes, I agree with the commit.
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).
> 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.
No putValue please. Use ContextAwareAction and pass there Lookup containing everything you believe such action needs (if anything).
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?
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.
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.
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?
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?
java.swing.Action only is assumed with name and small icon properties defined. The implemetation adds the JButton() with the Action to the toolbar.
> 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.
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?
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.
> 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.
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.
I'd rather not do 2 things at a time.
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).
Created attachment 19605 [details] new API diff
ok, here is what I consider final API diff based on discussion here. implementation diff follows.
Created attachment 19606 [details] output2 implementation
reassigning
integrated.