Bug 184894

VV1: IOContainer  (and IOContainer.Provider) has 3 distinct methods: open(), select(comp), requestActive()
=> Why do we need to have selectLite(JComponent)? Looks like if IOContainer.Provider.select(comp) is implemented correctly (without opening and activating parent container) - it is exactly what you ask for in selectLite() and it match javadoc of select(comp).

VV1: 
? The implementation of IOContainer.Provider.select in output2
calls selectTab:

        public void selectTab(JComponent comp) {
            if (!isOpened()) {
                open();
            }
            if (!isShowing()) {
                requestVisible();
            }
            if (singleTab == null) {
                pane.setSelectedComponent(comp);
            }
            checkTabSelChange();
        }
which isn't lite at all.

In principle one could implement InputOutput.select()
by having it's implementations call IOContainer.open, requestVisible
followed by a redefined lite select(). But I could do
that only in my implementation of IO putting interoperability at risk.
Also, doing that would make InputOutput's select() heavy and IOContainers
select() lite which would lead to confusion because of similarity of names.

(In reply to comment #2)
> VV1: 
> ? The implementation of IOContainer.Provider.select in output2
> calls selectTab:
> 
>         public void selectTab(JComponent comp) {
>             if (!isOpened()) {
>                 open();
>             }
>             if (!isShowing()) {
>                 requestVisible();
>             }
>             if (singleTab == null) {
>                 pane.setSelectedComponent(comp);
>             }
>             checkTabSelChange();
>         }
> which isn't lite at all.
bug against output2 impl?

> 
> In principle one could implement InputOutput.select()
> by having it's implementations call IOContainer.open, requestVisible
> followed by a redefined lite select(). 
This is how I understand it should be.

> But I could do
> that only in my implementation of IO putting interoperability at risk.
Tim can do the same in output2

> Also, doing that would make InputOutput's select() heavy and IOContainers
> select() lite which would lead to confusion because of similarity of names.
Ok. I see. 
InputOutput.select javadoc is clear:
    /**
    * Ensure this pane is visible.
    */
    public void select ();

javadoc of IOContainer and IOContainer.Provider is not (nothing about visibility):
    /**
     * Selects component in parent container
     * @param comp component that should be selected
     */
    public void select(JComponent comp) {

Y01 I'd like to warn you a bit: changing semantics of InputOutput method is usually disastrous. There is too many callers and they rely on current behavior, that even slight shift in behavior will for sure hurt somebody.

Y02 I can see that you are considering shift in IOController.Provider semantics. I think it is relatively low risk, that can be done (if Y01 remains - e.g. no changes to semantics of InputOutput). Inconsistency between those two select methods shall not be important, users of InputOutput don't call the IOController.Provider methods at all.

Y03 Improve the javadoc of IOController.Provider to more exactly specify the expected contract.

Y04 Side thinking: If you want to be backward compatible (rather than changing the semantics as approved by Y02), then create new factory method IOContainer.create(Provider p, boolean supportsLiteSelectPolicy) rather than new interface. I'd also suggest to not introduce new "selectLite" method, but add new method with boolean parameter: IOContaier.select(JComponent c, boolean openWhenHidden). Of course under the assumptiong that this all will still be needed.

Y05 Missing @since, apichanges.xml shall be added for next version of the patch

Y06 I don't understand the actual usecase. Who's going to call the new interface/method (I don't know who is calling the old select, obviously)? Maybe a bit of test code showing the interactions between IOContainer and its Provider would be beneficial.

re Y01, 02: This IZ doesn't propose to change the semantics
of IO.select() for the reasons you state. That was VV's suggestion.

re Y04: A single openWhenHidden might not be enough. I'm trying to 
follow the examples of the variety of methods on TC's (open, requestVisible,
requestActive) which control independent stuff and can be combined as needed.

re Y05: Will add as we approach closure.

re Y06: IOContainer isn't really meant as an end-user API. It's more a
support API for implementing IO's, so it' main (only?) callers are 
InputOutput implementations. 
Specifically IOContainer.select() is called as a result of
InputOutput.select() and any change in semantics of IOCOntainer.select
will impact those of IO.select().

So why isn't there a corresponding proposal for adding selectLite()
to InputOutput? Because for the time being I can add it in 'terminal'
and leave enhancements to 'output2' for later. The nature of 
IOContainer is such that I cannot enhance it using teleinterface
in 'terminal' hence this IZ. 

There's an analogy betwen IO's inside an IOContainer and TC's
inside Modes. The lifecycles and visibility semantics are similar.
This is why (I think) IOCOntainer mimics some of TC's methods. 
In addition an IOCOntainer can be inside a TC (tabs within tabs).
The original IO.select() operates all the way up to the TC, which
sets a precedent for two sets of methods on IO ... ones that operate
on IO vs it's container, and ones that "pass thru" to the containing
TC and it's containing mode.
For example, IOContainer.requestVisible() is a pass-thru to the TC 
while selectLite() is a "requestVisible" directly on the tab.

I like selectLite() because it does one simple primitive operation
on one pair of container/contained objects.

(In reply to comment #5)
> re Y01, 02: This IZ doesn't propose to change the semantics
> of IO.select() for the reasons you state. That was VV's suggestion.
Probably misunderstanding: 
I propose to change semantics of IOContainer.Provider.select in output2 (not IO.select) and forget about new method. 
Of course code in IO.select should be updated to call open, requestVisible, select to implement current semantics (clearly mentioned in javadoc for IO.select)

I'm starting to tend  towards a variation on Yardas suggestion.
Roughly introduce a selet with an EnumSet parameter. Something
roughly like:

enum IOContainer.SelectExt {
    AND_OPEN_TC,
    AND_REQUEST_VISIBLE_TC
}

IOContainer.select(EnumSet<IOContainer.SelectExt>);

With a null set it would be like selectLite().
With all elements of the set it would be like select().
I'll have to work a bit on exactly what should be in the enum.

Do not use the IDE's built-in diff tool for anything. Use 'hg diff' from the command line, passing --git if this is not on by default. See also: http://wiki.netbeans.org/HgHowTos#Develop_API_review_patches_using_MQ

Diffs should also include the complete patch, including to implementation. Certainly for any API change to openide.io we would want to see a matching impl in core.output2 unless such an impl would clearly be inappropriate.


(In reply to comment #6)
> I propose to change semantics of IOContainer.Provider.select in output2 (not
> IO.select) and forget about new method. 
> Of course code in IO.select should be updated to call open, requestVisible,
> select to implement current semantics

That seems a better idea to me too. In output2, Controller.performCommand(...,CMD_SELECT,...) should do some of what IOWindowImpl.selectTab does now. The Javadoc for IOContainer.select clearly says "selects component in parent container", nothing about displaying that container (which is explicitly left to other methods).


At some point, having a call on InputOutput to front the tab without activating the window would be useful. This would follow the usual API/SPI pattern in the package, such as for IOTab (not IOContainer which is unrelated!). For example, when the "Always Show Output" option is unchecked, the Ant module does not call IO.select when starting a process, which is irritating if the Output Window is visible but another tab is fronted; better would perhaps be to front the tab without forcing the whole window to pop up. But Ivan does not seem to be proposing such an API here.

JG: But Ivan does not seem to be proposing such an API
JG: [ InputOutput.selectLite() ] here.

Well, I am, but not in classic InputOutput. I was trying to
touch the minimum amount of code I don't own by adding that API
in 'terminal'. But considering how this is creating confusion perhaps
I should add it to output2 as well. (Uh-oh ... with Tomas Holy gone
am I getting dragged into maintaining output2 :-)

JG: That seems a better idea to me too.

I'm surprised that a semantic change is favored.
The justification here, I suppose, is that we're dealing with a
"closed universe". I.e. IOContainer has only two clients: output2
and terminal. In the past i've seen arguments based on "closed universe"
getting rejected in favor of "practicing compatibility even when we know
there's a closed univese".

It's really IO.select() that matters.

It's control path looks like this:
        IO.select()
        Controller ... CMD_SELECT
                create
                IOC.requestActive
                IOC.select ...
        IOController.select()
        IOController.Provider.select()
        output2...selectTab()
                TC.open()
                TC.requestVisible().
                selectLite()
_if_ we agree to work with the "closed universe" assumption then the above
can, per Vladimir and Jesses opinions, be tansformed to ...
        IO.select()
        Controller ... CMD_SELECT
                create
                IOC.requestActive
                IOC.open()
                IOC.requestVisible().
                IOC.select() ...
        IOController.select()
        IOController.Provider.select()
        output2...selectTab()
                selectLite()

However ... it seems that we _do_ want an IO.selectLite(). I definitely
need one and Jesse makes a case for it too. But the above transformation
doesn't really give us that! Sooo ...

What I think, _conceptually_, would be ideal is something like this:
        io.select(AND_REQUEST_ACTIVE|AND_OPEN|AND_REQUEST_VISIBLE);
Which would be equivalent to the current IO.select(), or
        io.select(0);
which would be equivalent to "selectLite".

This can be achieved in two ways:
1) Enhance IOContainer API:
   I.e. IOContainer also implements a select(EnumSet<SelectOpts>)
2) Change IOContainer.select() semantics:
   I.e. CMD_SELECT checks the EnumSet and calls IOC.requestActive,open,
   requestVisible and a _semantically modfied_ select.
Either would suit me. The tradeoffs, as I see them, are that ...
- (1) Molests IOContainer API.
      Maintains similarity btw IO and IOC.
- (2) Forces us to allow a 'closed universe" mindset.

(In reply to comment #10)
> JG: But Ivan does not seem to be proposing such an API
> JG: [ InputOutput.selectLite() ] here.
> 
> Well, I am, but not in classic InputOutput.

That would be the form in which random modules just using IOProvider to get an InputOutput (of unknown implementation and default display location) could actually use it.

> I'm surprised that a semantic change is favored.
> The justification here, I suppose, is that we're dealing with a
> "closed universe". I.e. IOContainer has only two clients: output2
> and terminal.

More relevant is that the impl in core.output2 appears to be directly contradicting the specification; i.e. VV1 is a bug fix, not an API change.

> _if_ we agree to work with the "closed universe" assumption then the above
> can, per Vladimir and Jesses opinions, be transformed to ...

Looks right, though I'm not an expert in this code. (Tim do you still know core.output2 well?)

> This can be achieved in two ways:

Again I don't think changing IOContainer is right; it already has very specific methods for doing particular things to the tab and to the container, so the API does not need to be touched. The problem is that a normal user of an InputOutput cannot access this functionality; while you can guess that IOContainer.getDefault is the container you are using, you definitely do not know what JComponent corresponds to your InputOutput.

Additions to the functionality of InputOutput are done using the following pattern:

import org.openide.windows.IOSomething;
import org.openide.windows.IOSomething.Action.*;
InputOutput io = ...;
if (IOSomething.isSupported(io)) {
  IOSomething.select(io, EnumSet.of(REQUEST_VISIBLE, SELECT_TAB));
} else {
  ...fallback...
}

where IOSomething has

    private static IOSomething find(InputOutput io) {
        if (io instanceof Lookup.Provider) {
            Lookup.Provider p = (Lookup.Provider) io;
            return p.getLookup().lookup(IOSomething.class);
        }
        return null;
    }

I'm not sure what exactly the "Something" should be; "Tab" is taken, perhaps "Selection". Then TerminalInputOutput and/or NbIO would add an IOSomething impl to their lookups, which would call individual IOContainer methods according to the enum set. In the case of core.output2, Controller.performCommand would receive a Set<Action> as data, and would call methods on ioContainer.

Created attachment 98159 [details]
openide.io.patch

Attached is a patch to openide.io.
I introduced IOSelect to allow an IO to select with finer grain using
the usual extension method. It uses enum SelectExtraOps.

IOContainer is also extended in a similar manner, but I've
swayed towards changing the semantics of IOCOntainer.select()
mainly because the SelectExtraOps applied to IOCOntainer are redundant
in the face of IOCOntainer having open etc. 

I've implemented this in 'terminal' and will proceed to implement it
in 'output2' with altered select() semantics and post an additional
patch for that.

It might be tricky to _exactly_ reproduce the semantics because 
the container implementation tests before applying operations:

        public void selectTab(JComponent comp) {
            if (!isOpened()) {
                open();
            }
            if (!isShowing()) {
                requestVisible();
            }
            if (singleTab == null) {
                pane.setSelectedComponent(comp);
            }
            checkTabSelChange();
        }
These tests are not available in the IOContainer API.
I could move the tests to IOCOntainer.open nd reuestVisible.

At this point testing all of this becomes tricky. There are 
unit tests in output2 but they don't measure visible effect.
I have a small user-driven "play" framework that I can use.

Y11 Rename SelectExtraOps; don't use abbreviations. My personal suggestion is IOSelect.Type or IOSelectType.

Y12 Should not select(JComponent comp, EnumSet<SelectExtraOps> extraOps) have a fallback if the SelectProvider is not provided?

re Y11: SelectType is inappropriate since there is a basic function
that select() will perform hence the word Extra. 
Moving it inside IOSelect is OK now that IOCOntainer will not need it.
    IOSelect.AdditionalActions

re Y12: Yes it should.

Created an actual bug# 185209 to document the eventual change and us in CS.

re Y05 Missing @since, apichanges.xml shall be added for next version of the patch

Does that mean I need to rev up the SpecificationVersion?

Created attachment 98237 [details]
patch for: openide.io core.io.ui core.output2

Allright this should do it. It's a complete patch covering
openide.io
core.io.ui
core.output2

Summary:
- Leave form of IOContainer unmolested.
- Simplify semantics of IOContainer.select()
- Retain semantics of InputOutput.select()
- Compensate for simplification by enhancing implementation of
  InputOutput.select()
- Ensure that _other_ callers of IOCOntainer.select() are not
  surprised by simplification.
- Introduce IOSelect (analog of IOTab) with method
  select(InputOutput, EnumSet<IOSelect.AdditionalOperation>)
  Implement it in Controller using IOEvent.CMD_FINE_SELECT.
- Minimal unit test in core.output2.

The only uncrossed t's and undotted i's are "since" comments
and API changes.

VV2: please, change subject of this IZ, because "Add selectLite() method to IOContainer" was in fact rejected and looks like you have agreed on that. 
Now you are focused on IOSelect capability instead, right => then I agree with adding such capability.

VV3: Do not use EnumSet in API methods, EnumSet is impl (like ArrayList), use Set instead (Clients will use EnumSet)

re VV2: Does it really matter? API proposals evolve based on feedback.
Will you sleep poorly if I don't change the subject?

re VV3: I'm not sure I agree. I recall seeing API reviews recommending
EnumSets, and not Sets, over ints OR'ed together.

(In reply to comment #20)
> re VV2: Does it really matter? API proposals evolve based on feedback.
> Will you sleep poorly if I don't change the subject?
Ok. It's just confusing + changing subject may attract other reviewers :-)

> 
> re VV3: I'm not sure I agree. I recall seeing API reviews recommending
> EnumSets, and not Sets, over ints OR'ed together.
I mean not over ints, just replace in API only:
select(InputOutput, Set<IOSelect.AdditionalOperation>)

internally of course EnumSets are passed as parameter

VV4: IOSelect can be used only by clients who knows about them => I propose to prohibit use of "null" as the second parameter in IOSelect.select. client should use empty EnumSet.nonOf if no extra action wanted (easier to read code then as well)

I think support VV3. Some designers believe that API should be written against interfaces not implementations. java.util.Set is the interface, EnumSet is implementation. As far as I can tell the methods both classes offer are same (plus EnumSet is publicly cloneable, but you don't want to use clone() anyway). The only practical reason for using EnumSet is prevent accidental use of HashSet (which would not be effective use of the API). I believe that a sample code in the javadoc of the method showing use of EnumSet would be enough. Use Set.

Re. Y05: Yes, you need to increase specification version of modules with new classes/methods and also increase dependencies of modules using these classes/methods (core.output2, terminal) to link properly.

Y13: When writing apichanges.xml create (an additional) entry to admit that there is a semantically incompatible change in IOController: "After fixing bug#185209 IOContainer.select() no longer performs these operations for us so we have to do them. ioContainer.open(); ioContainer.requestVisible();"

re VV4
I accept null simply because typing 
    EnumSet.noneOf(IOSelect.AdditionalOperation.class)
is a PIB.

I have also considered a plain IOSelect.select(InputOutput).
with a default empty set. Would that make things more convenient?

re VV3: EnumSet accepts only enums as it's members.
If I don't statically protect against that by using EnumSet 
I will have to dynamically protect against non-enums in a
plain Set.

Don't you prefer static type-checking?

re VV3: Never mind. I guess the Set<T> T parameter will do enough 
static type checking. I'll adjust after my "sleep period".

Re. Y05: Upping the dependency version of modules that directly use 
the new classes/methods makes sense. What to do with core.io.ui though?
It doesn't have a syntactic dependency but if you bundle it with 
an older output2 it will misbehave. I figure if both output2 and 
core.io.ui start depending on the new 1.23 openide.io that will
force core.io.ui to always travel with output2. Right?

Let me second VV3 and VV4 - the param should be @NonNull Set<AdditionalOperation>. A no-op select is quite easy using either EnumSet.noneOf or Collections.emptySet.


(In reply to comment #27)
> What to do with core.io.ui though?
> It doesn't have a syntactic dependency but if you bundle it with 
> an older output2 it will misbehave. I figure if both output2 and 
> core.io.ui start depending on the new 1.23 openide.io that will
> force core.io.ui to always travel with output2. Right?

Wrong; someone could update openide.io and core.output2 but not core.io.ui, without complaint from the module system. Be on the safe side and increment the specification versions of all three, as well as the relevant dependency versions.


By the way, improved behavior for the Ant module seems to work well with the proposed patch:

diff --git a/o.apache.tools.ant.module/src/org/apache/tools/ant/module/run/TargetExecutor.java b/o.apache.tools.ant.module/src/org/apache/tools/ant/module/run/TargetExecutor.java
[...imports...]
@@ -431,6 +433,8 @@
         if (outputStream == null) {
             if (displayed.get()) {
                 io.select();
+            } else if (IOSelect.isSupported(io)) {
+                IOSelect.select(io, EnumSet.noneOf(IOSelect.AdditionalOperation.class));
             }
         }

Ok, will make the additionalOps param non-null and rev up all versions.

Re VV4: @NonNull is for findbugs and doesn't really affect javac behaviour 
        right? Shall I throw an IA exception if a null is passed then?

(In reply to comment #30)
> @NonNull is for findbugs and doesn't really affect javac behaviour right?

Right.

> Shall I throw an IA exception if a null is passed then?

You can do so if you think violations would otherwise be hard to track down, say because the NPE would come much deeper in the stack trace or in another stack altogether. (Parameters.notNull is the easiest way.)

Note that in NB APIs, everything is assumed to be @NonNull unless explicitly stated otherwise.

I really don't get the philosophy of non-null here.
We cannot statically enforce it so we enforce it dynamically?
Why not just do "something reasonable" with null instead of
throwing exceptions?

JG: Note that in NB APIs, everything is assumed to be @NonNull unless explicitly
    stated otherwise.

Does that mean I _don't_ have to put a @NonNull in my code?

(In reply to comment #33)
>> everything is assumed to be @NonNull unless explicitly stated otherwise
> 
> Does that mean I _don't_ have to put a @NonNull in my code?

Sorry for not being clear. You are not required to use @NonNull, call Parameters.notNull, or to even mention in the Javadoc that null is not allowed - it is just assumed by default to be forbidden in every API parameter and return value. Sometimes people do one or more of these for particular reasons:

1. Use @NonNull (or @CheckForNull, etc.) so that FindBugs will produce helpful diagnostics, if you are running FB routinely on this or related modules.

2. Call Parameters.notNull where this would improve diagnostics. For example, if your method constructs an object and initializes fields, one of which accidentally is left null, then later some other method is called on the object and throws NPE, it might not be obvious where the null came from (since the code which passed in the null is nowhere to be seen in the NPE's stack trace). Calling notNull early pins down the culprit in the stack trace. More often a null value would throw NPE further down the same call stack so notNull doesn't help so much.

3. Sometimes Javadoc can be clarified by saying, as in this instance, "the set may be empty but not null".

Vladimir, can you lease clarify what you mean by "prohibiting null" in
comment #22? We'll be done after that. (I'll put out a final patch).

(In reply to comment #35)
> Vladimir, can you lease clarify what you mean by "prohibiting null" in
> comment #22? We'll be done after that. (I'll put out a final patch).

Call Parameters.notNull

Created attachment 98466 [details]
patch for: openide.io core.io.ui core.output2

Last call for patch to
openide.io
core.io.ui
core.output2

Summary:
- @since inserted for class IOSelect
- apichanges updated, including Y13.
- Use Enum instead of EnumSet.
- Applying Paramters.notNull() to extraOps.
  Negative testcase in unit test.
- Per JG#28 
  All affected modules' spec #'s incremented.
  dependency spec #'s adjusted.

I believe I have addressed all comments.

<issue number="168898"/> should I guess be <issue number="184894"/>.

<class package="org.openide.windows" name="IOSelect"/> in #Incompatible-IOContainer-select should rather be ...name="IOContainer"... I guess.

org.openide.io needs no new dep on org.netbeans.api.annotations.common if you are not in fact using @NonNull.

Otherwise looks good to me.

(In reply to comment #38)
> <issue number="168898"/> should I guess be <issue number="184894"/>.
> 
> <class package="org.openide.windows" name="IOSelect"/> in
> #Incompatible-IOContainer-select should rather be ...name="IOContainer"... I
> guess.
> 
> org.openide.io needs no new dep on org.netbeans.api.annotations.common if you
> are not in fact using @NonNull.
> 
> Otherwise looks good to me.

All corrected.
I'll proceed to check in after one last build and test.
Thanks for everyones input.

Integrated into 'main-golden', will be available in build *201005100200* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/
User: 
Log:

Any reason to keep this P2 open after the integration?