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've just get a chance to check the content of print/api. I have few comments, which I'd like you to address before 6.0 release, as later, the API will not be allowed to change incompatibly: Y01 PrintManager looks like a client API - e.g. majority of modules is going to call it, while only a limited set of modules is going to implement it. As such it is desirable to have the option to add new methods into it in future releases. That is why it shall not be an interface. Make it a (possibly abstract) class. If there is just one implementation (e.g. the one from print module), you can forbid others from subclassing the PrintManager by adding getClass().getName().equals("f.q.n.ManagerImpl") into the PrintManager's constructor. Y02 Remove the PrintManagerAccess and move the static method into the PrintManager class itself. Y03 PrintOption again looks like something that majority of modules calls while nobody needs to implement it. Make it final class instead of interface. The Option in the print module can then be deleted or have only static mehthods!? Y04 The print/api module should need (using OpenIDE-Module-Needs in manifest) an implementation of a token org.netbeans.modules.print.api.PrintManager, the print module shall provide such token, as the print/api does not seem to be able to work without the impl Y05 Usually NetBeans keep API encapsulated - e.g. no references from API to SPI like PrintManager taking PrintProvider as an argument. I would suggest to change it, but that is likely a bigger change. If you want to implement this advice, let me know, I propose it in more detail.
For Y01 you could instead make a final class with static methods for the API plus an interface for the SPI.
yaroslavskiy wrote (in email, next time post to IZ directly): Thank you very much for comments! I would like discuss some questions: 1. After refactoring it seems that there will be 4 classes only. Is it reasonable to merge print.api and print (impl) modules? It resolves YO4. Packages o.n.m.print.api/spi only will be opened for another modules. 2. PrintMamager will be public abstract class PrintManager { private static PrintManager defaultPrintManager; public static ActionManager getDefault() { if (defaultPrintManager == null) { defaultPrintManager = (PrintManager) Lookup.getDefault().lookup(PrintManager.class); } return defaultPrintManager; } public abstract void print(PrintProvider provider, boolean withPreview); public abstract Action getPreviewAction(); } Do you agree? 3. Can 2 classes from spi PrintPage and PrintProvider be moved to api folder? 4. Y03: all implementation in Option will be moved in api, correct? 5. About Y05: what you would suggest?
Re.1: Usually our API modules should be autoloads and have no UI. That means the PrintAction, PageSetupAction and PreviewActions should still stay in the impl module. But I like the idea of moving print.impl.core.Manager to the API module. If this works (e.g. the actions use just API of PrintManager&co.), then just make PrintManager final, you need no lookup. Re.4: Yes, I meant to make all impl part of the PrintOption class from the API. Re.3&5: This requires more complex answer, as I do not understand much why there is PrintProvider and PrintPage in the SPI. That is why I have to ask: Y11 I guess someone is supposed to implement them. I know print/impl does so, anyone else? If not, then we do not need this interface in SPI at all. Y12 I am fascinated by the logic inside CommonAction.getPrintProvider. That is pretty rich API which is not described anywhere. There should be bunch of tests to verify that if a TopComponent has property Printable.class something happens, that if has property Date.class something else happens, etc., etc. I am surprised that one of the options is not topComponent.getLookup().lookup(PrintProvider.class) - but it really does not seem to be. As such you probably do not want others to directly implement the PrintProvider and we are back at Y11. Y13 I'll try to advice more, but I need more info about Y11, Y12 - e.g. if someone implements PrintProvider, what should happen then? I can create instance and then what? How the new PrintAction finds out that there is my instance?
I've added in the action check for topComponent.getLookup().lookup(PrintProvider.class) and the same for dataObject. So, if someone would like to provide own pages, he/she can implement PrintProvider and put it in top component or dataObject. From my point of view, PrintProvider should be in print api/spi. It lets printing to be flexible. Now no client module implements it, but in future it can happen. About moving print.impl.core.Manager to the API: Manager depends on print.impl. If Manager is in API package, is it good idea that api depends on impl?
Can you implement following: Y01+Y02 - make PrintManager non-subclassable class, remove PrintManagerAccess? Y03 - make PrintOption a final class with all the impl from Option being moved here? Y04 - either use OpenIDE-Module-Needs or merge api+impl (as you suggested). Please either create a branch and let me know where to look or attach patch with your changes here. Thanks. PS: I'll provide a patch for Y05 soon.
Created attachment 49036 [details] Initial patch that would satisfy Y05 - e.g. made API and SPI signatures independent
The patch to satisfy Y05 more or less requires that print/api + impl are merged into one module. This is needed for actions in impl to see the Trampoline class. The patch also silently assumes that the actions will search for PrintManager.Job in TopComponent.getLookup() and not PrintProvider directly. I know this makes the API/SPI more ready for evolution, however Y05 is not that important as the previous Yxx, so implement them first.
I see that in patch PrintManager still depends on PrintProvider from spi. As I understood the main goal of Y05 is remove dependency api -> spi. Am I right?
Please, see changes in attached print project: 1. Modules print/api + impl have been merged into one module. 2. PrintManager is abstract class with static accessor. 3. PrintManagerAccess has been removed. 4. PrintOption has been deleted: only three parameters are required for PrintProvider, so the parameters are passed in PrintProvider.getPages(): width, height and zoom of pages. 5. PrintPage became easier: only one method for drawing void print(Graphics g). Question: Utility class PrintUI from print.ui (it used by several modules in SOA pack), Move to print.api?
Created attachment 49161 [details] print module
Ok, I like the direction of the changes. Merging the modules looks really nicer to me. Apply the ZIP file and Y31 Make PrintManager a final class, all its method can just delegate to core.Manager's DEFAULT Y32 I think now it makes sense to do the separation of API and SPI as I outlined in http://www.netbeans.org/nonav/issues/showattachment.cgi/49036/PrintAPIIndependentFromSPI.diff Y33 Re. PrintUI - please do not expose a class that extends WindowAdapater in an API, I am pretty sure, this is not what you want to do. There has to be other interesting methods in the class which you want to expose as API, but not windowClosed, etc.! What is the API usage? If you do the above (Y31, Y32), then your API signatures are in good shape. However here is few additional nitpicks that are missing in your module to satisfy NetBeans API standards (http://openide.netbeans.org/tutorial/api.html): Y34 You should write arch.xml document and fill its arch-usecases section. Describe the stability of your API. Y35 You should have an apichanges.xml document and mark there API changes that you do. Y36 You should have test coverage. Originally I opened this issue to fix mistakes in the API and as such I believe if you do Y31, Y32, we can close the issue. But do not expect that this API is in a shape to be part of platform cluster unless you do Y34, Y35, Y36.
Y31: Re. PrintManager is final class. I investigate print manager functionality and remove method PrintProvider provider, boolean withPreview). The reason is if a client would like to print specific data, it should implement own provider and put it in the lookup of dataObject or topComponent, it is enough. Print/Preview action gets it and invokes Print/Preview dialog. Client shouldn't invoke printManager.print(...) method directly. One thing which can be useful is print/preview action. This action can be used as button on the toolbar of a client view. For example, BPEL design view has the print preview button. Y32: Re. There is no dependency api -> spi.tachment.cgi/49036/PrintAPIIndependentFromSPI.diff Y33: Re. PrintUI is renamed to PrintUtil and lives in api. It provides a piece of useful method used in UI. Please, see attached project.
Created attachment 49233 [details] Next improvment
Added from e-mail thread: > why it is bad idea that api depends on spi? There are two main reasons: #1 - the "conceptual surface" - usually a (significant) group of people is interested in API - they should not see any links in JavaDoc that distract their attention and lead into the (often more complicated) SPI part. #2 - evolveability - I noticed that you have changed PrintProvider yesterday. This is big no no, after the release. You will not be able to touch it then! Now what you will do? PrintProvider2? Ok, but that means to change all the code that manipulates with PrintProvider to also understand PrintProvider2. And you never know where this code is, it can be anywhere in every module around the world. With my patch, there is just 1 piece of code (visible externally) that deals with PrintProvider - the PrintFactory - as such you will be able to only introduce PrintManager.Job create(PrintProvider2 p) and change the implementation inside of your module, while being sure no external user of the API/SPI is affected.
Re. Y31, Y32 - ok, good. Just a small hint(p3): make all methods in PrintManager static. Then you can use them from your layer file. Instead of <file name="org-netbeans-modules-print-impl-action-PrintPreviewAction.instance"/> you can write: <file name="org-netbeans-modules-print-api-PrintPreviewAction.instance"> <attr name="instanceCreate" methodvalue="org.netbeans.modules.print.api.PrintManager.getPrintPreviewAction"/> </file> as a result you do not need to expose the actual implementation classes anywhere in layer. You can stick with API methods. Re. Y33 is not imho fixed. There still some class that extends WindowAdapter in the API. Moreover the PrintUtil class is not documented at all. I do not like it at all. What is your unit test coverage of that class? If less than 80% can it be temporarily removed from the API? Inspite I believe Y33 should be fixed in better way, I see the current state as an improvement over the old version. Please integrate this and then consider addressing Y33, Y34, Y35, Y36.
Y31, Y32 fixed.