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.
During the inception review of bug 24720 an need for deeper review of new I/O API has been requested. This bug is here to track its progress.
There are two possible paths to move from our dis-satisfactory current state. Either keep the total duplication of the API or just remove parts of existing one via patching. If we want to keep the two separate APIs, then: Y01 Binary compatibility is kept (those who use old one, are OK). Those who use new one know what they are doing. core.output2 implements both. Y02 Do we have some other implementations of the API in the release? I think there was terminal based one. It needs to be updated as well. Y03 We want to support easy migration from the old I/O API to the new I/O API. The simplest I can imagine is to: change the module dependency & change imports. Keep the rest of the code unchanged. Of course except remove or replaced concepts like Action. To achieve that: Y03a: Remove the Base prefix of the classes. E.g. keep the class names identical to the old I/O API. Y03b: Don't use name base in the package name. I suggest to put the API into org.netbeans.api.io and possibly org.netbeans.spi.io Y04 Write hints that will convert the old API patterns to new API patterns. Y05 Fix API mistakes. Like: InputOutput should have never been interface, rather a final class. Y06 Don't introduce new API mistakes: I don't understand at all what BaseIOLinkInfo, BaseOutputTag is for.
Y07 The old API was official - the new replacement should be official/stable as well.
The creation of new API is tempting. The current I/O API is really historical. But if we do it, we need to do it properly and in such case I would suggest its own API review before it gets merged to make sure we really improve the experience. An alternative to this is to follow the "patching" approach used in other places and remove references to awt&swing. As far as I know it would mean to: Y11 Create own I/O UI module which would be fragment for original I/O Y12 Move IOContainer into the UI module Y13 Move all Action referencing method from IOProvider to superclass in the I/O UI module Y14 Move IOTab to I/O UI module Y15 Move color related stuff like IOColorLines IOColorPrint IOColors into the I/O UI module In contrast to creation of new API this approach requires us to guarantee backward compatibility (as described in issue 247408), but does not need deep review for usability, evolution, etc. as that basically remains unchanged.
Jarda H. told me he prefers new API and that he can wait and do throughout review of the new API later, after bug 247200 is integrated. Good, I/O deserves new and better API, as the current one is pretty archaic. Two comments to the approach: Y16 It would be nice if openide.io would be an implementation of the new I/O SPI - e.g. no change needed in core.output2 or terminal. The openide.io would do the bridging. Y17 Jardo, start with arch.xml and <usecase> section. Especially the one with OutputListener-less "callback" should be described and motivation for it provided.
Created attachment 149949 [details] Proposed Patch - API Preview Attaching preview of the API. It supports hyperlinks, color printing, and tries to fix some API mistakes. Some of extension classes are now replaced by methods in OutputWriter and InputOutput. I'll consider replacing other extensions classes (IOFolding, IOPosition) tomorrow. Please see arch.xml for usecases. > Y02 Do we have some other implementations of the API in the release? > I think there was terminal based one. It needs to be updated as well. The implementation is in module terminal. I can update it as soon as the API is stabilized, it should be quite straightforward. > Y03 We want to support easy migration from the old I/O API to the new I/O API. Class names are the same, only features of extensions classes were moved. > Y03a: Remove the Base prefix of the classes. OK > Y03b: Don't use name base in the package name. Package names are org.netbeans.api.io and org.netbeans.spi.io as suggested. > Y04 Write hints that will convert the old API patterns to new API patterns. Good idea, but I would postpone it after the API accepted. > Y05 Fix API mistakes. API classes are final (or have private constructors). SPI for output window implementation is a single interface InputOutputProvider. > Y06 Don't introduce new API mistakes. Confusing classes removed. > Y16 It would be nice if openide.io would be an implementation of the new I/O SPI > - e.g. no change needed in core.output2 or terminal. The openide.io would do the > bridging. It seems to be quite difficult. One of the problem is choosing the default provider. Default provider in the old API is the first IOProvider in default lookup. But if we have different list of providers for the new API, which of them should be chosen as the default one? If we do the bridging in the new API, we would need some way to put bridged providers into default lookup, or introduce a new (alternative) registration mechanism. > Y17 Jardo, start with arch.xml and <usecase> section. Especially the one with > OutputListener-less "callback" should be described and motivation for it provided. The API currently uses OpenUriHandler SPI, which is registered for output window only, but there may be some better solution. So I'd like wait for the feedback. I now see the motivation is not described very well in the usecase. I'll try to improve it soon.
Created attachment 149963 [details] Proposed Patch > I'll consider replacing other extensions classes (IOFolding, IOPosition) > tomorrow. The API now contains replacements for IOFolding, IOPosition and IOTab. (The patch is based on jet-main default branch.) Please review. Thank you.
OK, looking at the new patch, I think we can ignore most of the previous Yxx comments and start new set: Y21 I think you'd rather use OpenIDE-Module-Needs in the API to force at least + <answer id="deploy-dependencies"> + <p> + You will very likely also want to declare + </p> + <pre>OpenIDE-Module-Requires: org.netbeans.api.io.IOProvider</pre> + <p>to ensure that an Output Window implementation is in fact enabled.</p> + </answer> one implementation to be provided whenever the API is used. Then for modules depending on I/O API, it is enough to have module dependency and the rest is guaranteed by the "needs" dependency. Or when you have the Trivial implementation, maybe you can use "recommends". Y22 "Should be thread safe." - probably add a note to all API interfaces that they can be called from any thread. Maybe also warning to SPI interfaces that they will be called from any thread by the infrastructure (if that is the case). Y23 "wrinting", "expad()" - typos. Y24 Hyperlink concept is nice. I don't however see a reason why the API should be able to obtain the Runnable or the URI. True, SPI implementors needs to be able to invoke action on a Hyperlink, but that should be part of SPI and not API. Y25 What do you use InputOutput.getLookup for? I have not got it from the diff... Y26 Should InputOutput implement AutoCloseable? E.g. have close() method in addition/instead of closeInputOutput? Y27 There is one setter setDescription. Not sure if that is appropriate. Should not the description be specified at creation time? Y28 We should consider moving OpenUriHandler handler : Lookup.getDefault().lookupAll(OpenUriHandler.class) code into some more general module. It is likely going to be needed in other places as well. Y29 When this module gets integrated, openide.io should be deprecated.
Created attachment 150030 [details] Proposed Patch v2 > Y21 I think you'd rather use OpenIDE-Module-Needs in the API to force at > least one implementation to be provided whenever the API is used. Added "OpenIDE-Module-Needs: org.netbeans.spi.io.InputOutputProvider" to module manifest. Note: I've changed the token to "org.netbeans.spi.io.InputOutputProvider". > Y22 "Should be thread safe." - probably add a note to all API interfaces > that they can be called from any thread. Maybe also warning to SPI > interfaces that they will be called from any thread by the infrastructure JavaDoc of the API classes and SPI interface modified. > Y23 "wrinting", "expad()" - typos. Fixed. > Y24 Hyperlink concept is nice. I don't however see a reason why the API > should be able to obtain the Runnable or the URI. True, SPI implementors > needs to be able to invoke action on a Hyperlink, but that should be part > of SPI and not API. Methods getURI and getRunnable removed from Hyperlink class. The SPI package now contains Hyperlinks and OutputColors helpers, that use accessors to retrieve information from API classes. I've also moved OutputColorType and HyperlinkType enums to the SPI package. > Y25 What do you use InputOutput.getLookup for? I have not got it from the > diff... It can be used to extend InputOutput features. E.g. the implementation in core.output2 puts IOTab into the lookup, which offers a method for setting icon of I/O tab. This cannot be directly in the API, as it would require dependency on AWT/Swing classes. > Y26 Should InputOutput implement AutoCloseable? E.g. have close() method in > addition/instead of closeInputOutput? Method closeInputOutput() closes the I/O tab, which should not be closed automatically. PrintWriter returned from getOut() and getErr() already implements AutoCloseable. See bug 245014 comment 7. > Y27 There is one setter setDescription. Not sure if that is appropriate. > Should not the description be specified at creation time? I've considered this solution, but I concluded that the setter is fine because: - The description can be changed while printing the output (e.g. when build fails). - Two string arguments in IOProvider.getIO(...) could be confusing. Thank you for your comments. JH01 Ondrej Vrabec suggested replacing select/show/activate methods in OutputWriter with a single method that accepts additional properties, something more similar to current IOSelect. This change is included in the patch. See class ShowOperation and method OutputWriter.show(). JH02 We've discussed whether show(), startFold() and getCurrentPosition() methods should be in OutputWriter or InputOuput. Most implementations probably merge the standard and error output into a single stream and displays them in a single GUI panel, but some implementations can keep the stream separated. For them, it is important to know for which of the streams the operation was invoked. So I prefer to keep the methods in OutputWriter, but if it seems confusing to the API clients, we should think about another solution.
Created attachment 150058 [details] Proposed Patch v3 Added class org.openide.windows.BridgingIOProvider to the original API module openide.io. It implements both the new and the old SPI and it handles the bridging. The implementators of the original API can simply extend BridgingIOProvider instead of IOProvider, add dependency on api.io, and add ServiceProvider annotation for the new SPI (this is described in JavaDoc of the class). New SPI InputOutputProvider now also specifies types for Position and Fold objects, which makes it more general and easier to work with for current implementations.
Created attachment 150072 [details] Proposed Patch v4 (includes patch for Intent API) > Y28 We should consider moving OpenUriHandler handler : > Lookup.getDefault().lookupAll(OpenUriHandler.class) > code into some more general module. It is likely going > to be needed in other places as well. Created module io.intent. See bug 248126.
> Created module io.intent. I mean api.intent. I'm sorry.
Created attachment 150125 [details] Proposed Patch v5 (includes patch for Intent API) Updating Intent API to v3 (see bug 248126). No changes in the actual I/O API.
Created attachment 150162 [details] Proposed Patch v6 (includes patch for Intent API) Applied suggestions mentioned during on-site discussion with Petr Hejl, Tomas Stupka, Ondrej Vrabec and Tomas Zezula. Thank you for help. IOProvider.getName() renamed to IOProvider.getId() InputOutputProvider.getName() renamed to InputOutputProvider.getId() InputOutput.closeInputOutput() renamed to InputOutput.close() OutputWriter.show(...) methods moved to InputOutput class InputOutputProvider.showIO(IO, WRITER, Set<ShowOperations>) changed to InputOutputProvider.showIO(IO, Set<ShowOperations>) Hyperlink.onClick(Runnable) renamed to Hyperlink.from(Runnable) Hyperlink.forURI(URI) renamed to Hyperlink.from(URI) HyperlinkType.ON_CLICK renamed to HyperlinkType.FROM_RUNNABLE HyperlinkType.FOR_URI renamed to HyperlinkType.FROM_URI Classes HyperlinkType, Hyperlinks, OutputColorType and OutputColors moved from package org.netbeans.spi.io to org.netbeans.spi.io.support
Created attachment 150259 [details] Proposed Patch v7 Removed support for URI hyperlinks. It can be added later, when the Intent API is finished (this feature was not included in the original API).
Y31 Hide HyperlinkType from the SPI for now/for ever(?) Y32 What about replacing "Runnable getRunnable(@NonNull Hyperlink hyperlink)" with static void invoke(Hyperlink)? Y33 I don't know what isImportant() means! I'd like to remind Y04. Y34 Once the API is published, would organize a usability study on it? E.g. let somebody really use it and send you comments? Community could do it...
Summary of review meeting: > Y31 Hide HyperlinkType from the SPI for now/for ever(?) TCR: Remove the enum and related methods in class Hyperlinks. > Y32 What about replacing "Runnable getRunnable(@NonNull Hyperlink > hyperlink)" with static void invoke(Hyperlink)? TCR: Add method invoke(Hyperlink). It can be used as a convenience method in the future when more types are supported. > Y33 I don't know what isImportant() means! TCR: Add non-normative comments to JavaDoc. > I'd like to remind Y04. TCA: Implement hints. TCR: Module core.outpu2 should depend only on the new API. Bridge the modules using reflection and class loader in default lookup.
> TCR: Module core.outpu2 should depend only on the new API. > Bridge the modules using reflection and class loader in default lookup. Unfortunately, this seems to be impossible. Module core.output2 contains some features that are discouraged (IOColors) by the new API, or that are related to GUI (IOContainer). Bridging would make some features inaccessible or awkward. If there are no objections, I will: - Let core.output2 implement both SPIs - Provide SPI bridging for other modules (old-to-new and new-to-old) - Rename the original API (e.g. to Swing I/O APIs, suggestions are welcome) Thank you for understanding.
Created attachment 150342 [details] Proposed Patch v8 > > Y31 Hide HyperlinkType from the SPI for now/for ever(?) > TCR: Remove the enum and related methods in class Hyperlinks. Enum HyperlinkType removed (moved to non-public package). Method Hyperlinks.getType(Hyperlink) removed (made package-private). Method Hyperlinks.getRunnable(Hyperlink) removed (made package-private). > TCR: Add method invoke(Hyperlink). Method Hyperlinks.invoke(Hyperlink) added. > > Y33 I don't know what isImportant() means! > TCR: Add non-normative comments to JavaDoc. JavaDoc updated. > > TCR: Module core.outpu2 should depend only on the new API. > Unfortunately, this seems to be impossible. [...] > Bridging would make some features inaccessible or awkward. > If there are no objections, I will: > - Let core.output2 implement both SPIs Done. > - Provide SPI bridging for other modules (old-to-new and new-to-old) Done. > - Rename the original API (e.g. to Swing I/O APIs, suggestions are welcome) Renamed to I/O APIs - Swing. Suggestions for better name are still welcome. The patch was updated for current jet-main default branch. If there are no objections, I'll integrate the patch this evening. Many thanks to all reviewers for comments and help!
> If there are no objections, I'll integrate the patch this evening. Integrated as http://hg.netbeans.org/jet-main/rev/05f13c9db88f.