Bug 182538 - getIO() not flexible enough in view of multiple impl's.
getIO() not flexible enough in view of multiple impl's.
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Output Window
6.x
All All
: P3 with 1 vote (vote)
: 7.3
Assigned To: Jaroslav Havlin
issues@platform
: API, API_REVIEW_FAST
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-23 04:02 UTC by ivan
Modified: 2012-07-29 02:16 UTC (History)
1 user (show)

See Also:
Issue Type: DEFECT
:


Attachments
Proposed Patch (12.90 KB, patch)
2012-06-22 13:26 UTC, Jaroslav Havlin
Details | Diff
Proposed Patch (14.15 KB, patch)
2012-06-22 14:11 UTC, Jaroslav Havlin
Details | Diff
Proposed Patch (14.43 KB, patch)
2012-07-24 14:47 UTC, Jaroslav Havlin
Details | Diff
Fix to make TerminalIOProvider to compile (516 bytes, patch)
2012-07-25 04:55 UTC, ivan
Details | Diff
Proposed Patch (15.04 KB, patch)
2012-07-25 12:05 UTC, Jaroslav Havlin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ivan 2010-03-23 04:02:45 UTC
The existing IOProvider.getIO() functions, in union,
take the following parameters:
- name
- actions
- newIO
- container
Yet none cover all the possible parameters.

In view of multiple possible implementations of IOProviders
and IOContainer.Providers this lack of flexibility can be awkward.

For example, consider, ...

IOProvider myIOProvider = IOProvider.get("Terminal");
IOContainer myContainer = (new MyContainer()).ioContainer();
InputOutput io1 = myIOProvider.getIO("io1", null, myContainer);
io1.getOut()....
// at this point io1 is not reusable because it's getOut() hasn't been closed
boolean reuse = true;
InputOutput io2 = myIOProvider.getIO("io1", reuse);

Even though 'reuse' is true this call to getIO() will not
find an available "io1" and will have to create a new one.
The question is, which container will it go into?

It might pick IOCOntainer.default(), but that's probably not
what the caller would expect. They would expect that io2 would
appear in the same container as io1.

Perhaps the "reuse" variation of getIO() should use the default
container associated with the IOProvider being used, i.e. MyContainer?
That might address the above example, but will not work in general.
The problem is that MyContainer might not be associated with a 
TopComponent so the first time IOPRovider has to get one ... it can't.

Perhaps IOProvider should remember that "io1" was in myContainer?
But what if myContainer is "gone"?

The natural solution seems to be a variation ongetIO() that takes
all 4 parameters. That way someone can use

InputOutput io2 = myIOProvider.getIO("io1", null, reuse, myContainer);

Ironically all three getIO()'s call a common getIO() like the above which
is unfortunately private.
So perhaps the simplest solution is to make that common getIO() public?
Comment 1 Jaroslav Havlin 2012-06-22 13:26:26 UTC
Created attachment 121222 [details]
Proposed Patch

Proposed Patch
 - added method IOProvider.getIO(String, boolean, Action[], IOContainer)
 - updated NbIOProvider, added NbIOProviderTest
Comment 2 Jaroslav Havlin 2012-06-22 13:28:56 UTC
Please review.
Comment 3 Petr Hejl 2012-06-22 13:43:45 UTC
PH01: I would suggest annotating the params with annotations from api.annotations.common.
Comment 4 Jaroslav Havlin 2012-06-22 14:11:03 UTC
Created attachment 121225 [details]
Proposed Patch

PH01 - Arguments and return value of the new method were annotated. Thank you.
Comment 5 ivan 2012-06-22 19:36:07 UTC
(In reply to comment #1)
> Created attachment 121222 [details]
> Proposed Patch
> 
> Proposed Patch
>  - added method IOProvider.getIO(String, boolean, Action[], IOContainer)
>  - updated NbIOProvider, added NbIOProviderTest

I see that the private method had become public, but I see some other
changes that are not described in this IZ.

It will take me a bit to give feedback on this, I'll have to go 
home and resync all my NB-related stuff and double-check with 
the trerminal io provider code. I"ll try and do that tonight.
Comment 6 Jaroslav Havlin 2012-07-09 14:17:37 UTC
(In reply to comment #5)
> I see that the private method had become public, but I see some other
> changes that are not described in this IZ.
The original mapping [IO name] -> [IO object] has been replaced
with mapping [IO container, IO name] -> [IO object].
This mapping is used for reusing of IO objects.

> It will take me a bit to give feedback on this, I'll have to go 
> home and resync all my NB-related stuff and double-check with 
> the trerminal io provider code. I"ll try and do that tonight.
Please, have you tried the patch? Any comments/ideas/problems? Thank you.
Comment 7 ivan 2012-07-09 19:21:22 UTC
(In reply to comment #6)

> > It will take me a bit to give feedback on this, I'll have to go 
> > home and resync all my NB-related stuff and double-check with 
> > the trerminal io provider code. I"ll try and do that tonight.
> Please, have you tried the patch? Any comments/ideas/problems? Thank you.

Sorry, not yet.
Initially I had what I thought were hg problems. You
might've seen my nbdev posts.
Now I'm having trouble building the IDE.
Comment 8 Jaroslav Havlin 2012-07-24 13:26:43 UTC
(In reply to comment #7)
> Now I'm having trouble building the IDE.
Please, any news?
Comment 9 Jaroslav Havlin 2012-07-24 14:47:05 UTC
Created attachment 122306 [details]
Proposed Patch

Updated JavaDoc for the new method. Added note that returned InputOutput should be closed when not needed.
Comment 10 ivan 2012-07-25 04:54:30 UTC
Apologies for taking so long.
Paging in NB development stuff after 1.5 years of inactivity
wasn't easy.

IS01: Making IOProvider.getIO(<all args>) public will break the build
      because TerminalIOProvider also has a private method getIO() with
      an identical signature.

      But given that there are just two implementations of the IOProvider 
      SPI you can just adjust TerminalIOProvider. It's trivial. See
      appended diff (sorry I'm not very handy with patch stuff).

      After adjusting TerminalIOProvider I verified that the <all args>
      implementation of TerminalIOProvider.getIO() works as expected.
      See below for details.

IS02: The new semantics of having each container have it's own 
      "name" space makes a lot of sense.
      I've filed IZ 215913 to ensure that TerminalIOProvider provides
      identical semantics although I'm not sure when I'll get to it.

module 'terminal' which is part of the cnd cluster has a second, after
output2, implementation of IOProvider. You can run it's junit test to
witness the motivation for this bug. The junit test runs in batches 
where a standalone TC is put up, exercised and then brought down.
Pay attention to the first batch. Everything will happen in one 
TerminalIOProvider provided TC except that another standalone TC
will be created with the message "Hello to io2" in it.

The relevant junit code is in T1_Close_Test.testStreamClose().
Search for 182538. A bit above it is a line
    io2 = ioProvider.getIO("io1", false);
This is the line that causes io2 to show up in the default as opposed
to the given ioContainer. If you replace this line with
	io2 = ioProvider.getIO("io1", false, null, ioContainer);
then the standalone TC with  "Hello to io2" won't show up.
I verified this in my workdir.

By the way, thanks for fixing this.
If you're going to be working more with "output" ...

- There are a coupla readmes that, while written for Terminal, might
  be useful in the context of output2:
    terminal/README.attributes
    terminal/README.close_semantics       # especially this one

- The terminal module extends the IO interface using the standard
  static method + Lookup technique with classes like IOConnect,
  IOEmulation, IOREsizable, IOTErm, IOVisibility, etc.
  These, by rights should be moved to openide.io and if possible
  implemented in output2.
Comment 11 ivan 2012-07-25 04:55:45 UTC
Created attachment 122330 [details]
Fix to make TerminalIOProvider to compile
Comment 12 Jaroslav Havlin 2012-07-25 12:05:14 UTC
Created attachment 122351 [details]
Proposed Patch

The two patches were merged.

Thank you very much for your great help, Ivan.

IS01 - TerminalIOProvider adjusted.

> If you're going to be working more with "output" ...
I've created separate bug 215940 (enhancement) with your suggestions.

Thanks again for your effort, explanation and ideas!
Comment 13 Jaroslav Havlin 2012-07-25 12:11:58 UTC
If no objections are filed, I'll integrate on Friday.
Comment 14 Jaroslav Havlin 2012-07-27 14:33:26 UTC
http://hg.netbeans.org/core-main/rev/6cb43817877c
Integrated.
Comment 15 Quality Engineering 2012-07-28 02:22:03 UTC
Integrated into 'main-golden', will be available in build *201207280002* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/6cb43817877c
User: Jaroslav Havlin <jhavlin@netbeans.org>
Log: #182538: getIO() not flexible enough in view of multiple impl's
Comment 16 Quality Engineering 2012-07-29 02:16:43 UTC
Integrated into 'main-golden', will be available in build *201207290002* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/334fea338aa7
User: ivan@netbeans.org
Log: nobugid - adjust Terminal unit tests now that BZ #182538 is fixed


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2014, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo