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.
Summary: | Editor/MimeLookup and Editor/Settings APIs Review | ||
---|---|---|---|
Product: | editor | Reporter: | Martin Roskanin <mroskanin> |
Component: | Settings | Assignee: | apireviews <apireviews> |
Status: | RESOLVED FIXED | ||
Severity: | blocker | CC: | jjancura, jlahoda, jtulach, ppisl, tpavek |
Priority: | P1 | Keywords: | API_REVIEW |
Version: | 4.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | TASK | Exception Reporter: | |
Bug Depends on: | 62674, 62675 | ||
Bug Blocks: | 59009, 59458, 60995 | ||
Attachments: |
The zip containing javadoc documentations including arch documents
EMMA tests coverage updated EMMA tests coverage Updated javadoc and arch documentations editor/settings/storage arch document commit log |
Description
Martin Roskanin
2005-06-09 13:29:30 UTC
Created attachment 22587 [details]
The zip containing javadoc documentations including arch documents
Short description: Editor MimeLookup module - Each editor provides an EditorKit which controls the policy of specific MIME content type. The policy of content type should be easily registered and found via some lookup mechanism, that will provide convenient way of using it either for kit provider or base editor infrastructure. In addition to this, the policy can be inherited, (for example in case of embeded kits like JSP) and the content types need to be merged in this case. MIME Lookup API should provide all mentioned requierements via easy lookup query, so content type policy user need not to solve this searching and merging on its own side. Editor Settings module - The main purpose of this project is to create editor/settings API, that will contain settings classes, which will be lookup-able via mimelookup. The aim is NOT to provide physical implementation of editor settings storage. The module will be just interface between the settings storage and the settings clients like <mime-type> editors, externaleditor, etc. Reassigning to apireviews@netbeans.org in accordance with Step 2 of: http://openide.netbeans.org/tutorial/review-steps.html Arch documents are already attached. We are at step 4. The currently assigned reviewers are Jan Lahoda, Petr Pisl, Yarda Tulach and Tomas Pavek. The review is planned to be done in June 15. I am glad this interface implements standard Lookup. Y01 MimeLookup shall not expose its internals (e.g. ProxyLookup superclass). Can you just make it factory class where methods would work only on Lookup and hide the impl? I know there is problem with the one non-static method on MimeLookup, but it may be wise to turn it into static one and anyway when it is supposed to be used? Y02 Why Class2Layer? Cannot you scan recursively? Is there some performance problem? Measured? Or provide some default mapping? Or annotate the folders with <attr name="instancesClass" stringvalue="FoldManagerFactory" />? Y03 Please link to CVS in sentence "please look at the simplified TestMimeLookupInitializer" and on other places. This will help readers to easily navigate to the code and check it. Y04 Use <api group="layer" /> to enlist all the possible registration points. Now it is not used at all. Also use <api group="lookup" /> for anything you lookup in META-INF/services. Btw. javadoc shall now be named org-netbeans-modules-editor-mimelookup... Y05 Can you reuse lookup tests from org-openide-util.jar, so we have confidence that your implementation matches the one in org-openide-util.jar? The basic trick is to extends AbstractLookupBaseHid test and implement few factory methods as is done in AbstractLookupTest. Y06 What is coverage of your tests? Can you generate the emma results output (project node/popup/measure coverage)? Y07 OpenAPIs are there no more, you cannot depend on that project in your arch.xml Y08 This javadoc is not much descriptive: "public abstract List getKeyBindings() Gets the keybindings list. Returns: List of KeyBindings" I support Y01. It seems to me that MimeLookup.childLookup(String) can be replaced by a factory method getMimeLookup(String[] mimeTypes). Created attachment 22624 [details]
EMMA tests coverage
Adding EMMA tests coverage. I am going to update mimelookup tests with testing of template lookuping, since the issue #58941 is already fixed (Thanks Yarda). Actual status of Ycomments: Y03-Y08 are already resolved. ad Y01) Regarding usecase it's e.g. obtaining syntax coloring colors for java embedded in jsp. There should be a usecase in the arch for this. We are aware that the lifetime of the MimeLookup instances is sort of artificial as the clients will only attach listeners to the returned Lookup.Result (not to the MimeLookup itself) so we planned to cache the instances using soft-ref maps. Regarding factory methods there is an issue with the childLookup() as Yarda pointed out. I do not prefer passing String[] mimeTypes. We were considering MimePath encapsulation like this: public final class MimePath { public static MimePath get(MimePath parent, String mimeType) {...} private String mimeType; private MimePath parent; } but finally we thought that the present solution would be somewhat better. ad Y02) For <attr name="instancesClass" stringvalue="FoldManagerFactory" /> we would either have to annotate it for every mime-type which is IMO extra work for clients. Or we would annotate just the global folders on the root level e.g. "Editors/foldManager" but those folders will be empty for most of the classes registered. So the only benefit would be that the class SPI would be replaced by the same layer-based SPI. Does it bring any benefit? Regarding default folder names do you mean to take e.g. the class name without package as the folder name e.g. "FoldManagerFactory" for class o.n.spi.editor.fold.FoldManagerFactory"? We already have several such folders used that do not comply with this policy and that we thought that we could turn to the MimeLookup use so if this would be preferred we would have to change the folder names in the layer.xml files. There is also usecase for preprocessing the files in a particular folder before turning it into instance i.e. the present InstanceProvider (e.g. the editor's Popup Menu classes where the files are either the actions' instances or names of the editor actions) that we need to cover somehow. Fixed mime lookup rebuilding problems Adding tests for templates lookuping and re-creating deleted files Deleting obsolete class Attaching updated emma tests coverage Created attachment 22645 [details]
updated EMMA tests coverage
Regarding Y01 I've forgot to mention that regarding MimeLookup we are willing to replace extending of the ProxyLookup by just Lookup or stop extending anything completely. Created attachment 22685 [details]
Updated javadoc and arch documentations
Mila's Lifecycle comment: Usually Lookup.Result points to its Lookup (at least in Proxy & Abstract lookup impls). Check about Y05, are you really reusing the tests? I still think ProxyLookup should not appear in javadoc. >Check about Y05, are you really reusing the tests? MimeLookup is represented via ProxyLookup that contains a set of AbstractLookups. I have reused the tests and AbstractLookup is tested in StandardLookupTest, which ensures that MimeLookup is also OK. >I still think ProxyLookup should not appear in javadoc. There is already no ProxyLookup occurence in mimelookup or settings module updated javadocs. Please review also newly created module editor/settings/storage that is reponsible for storage implementation of editor/settings module. The module is a result of TCR - issue #60995 More details are in the arch document. I will attach it. Created attachment 23945 [details]
editor/settings/storage arch document
Why is the arch document not checked into CVS ;-? Y22 - "XML files are read and written on disk." well, could you describe what files, from where and what is their format. I guess the stability is going to be "stable" otherwise our users would get upset, imho. Reference to DTD would be nice. Can the arch doc be updated? Y23 - was not one of the decisions on inception review that the edit/settings objects are going to be immutable? Then I am not sure why the listeners are needed. Y24 - why is there warning about subclassing the class in API only in one class? Y25 - I do not like the hierarchy of the classes in API. Classes extends classes named by the same name but different package. Moreover I expect another class extending this one lays somewhere in the impl. Let's remove all the classes from the API and replace them with one: public static class EditorSettingsStorage { public static void setAllFontColors(FontColorSettings fromLookup, ...); public static void setKeyBindings(KeyBindingSettings fromLookup, ...); } Y26 - I do not know what EditorSettings are supposed to do, there is not test calling them, nor they extend any other api class. So I cannot recommend on how to change them. Sorry. >Why is the arch document not checked into CVS ;-?
It is. editor/settings/storage/arch/arch-editor-settings-storage.xml
Y24: Only font and colors and keybindings settings were implemented in
editor/settings/storage module for now. Other settings will be implemented later.
Y22 - files format and layout should should be same. No incompatible changes, small additions only. I can not find old documentation... Y23 - there are no listeners on org.netbeans.api.editor.settings.* Y25 - Are you really reviewing Editor/Settings? There is no hierarchy of classes... Y26 - again. this is Editor/Settings/Storage API, not Editor/Settings Re. arch.xml: It has to be used when generating javadoc, either rename the file or use property in project.properties. See http://openide.netbeans.org/tutorial/api.html#doc Re. comments by jjancura: of course my comments are about editor/settings/storage, as that is what Martin asked us to review as well. And it probably should be reviewed as one of the blockers for editor/settings is missing implementation, which afaik editor/settings/storage are going to provide. >Re. arch.xml: It has to be used when generating javadoc, either rename the
>file or use property in project.properties
fixed, thanks for the hint.
Y22 - arch updated with the requested info
Few editor/settings semantics questions: MM01) Will FontColorSettings handle printing settings as well? At present the editor's printing settings are sort of buried under Tools->Options->IDE Configuration->System->Printing Settings-><editor-type>. Hanzi I suppose that you will cover them under the new options somewhere, right? MM02) We should polish the constants in SimpleValueNames. Some constants are IMHO deprecated e.g. *MARGIN* ones or e.g. LINE_HEIGHT_CORRECTION and some names could be improved e.g. JAVADOC* => COMPLETION_DOC*. editor/settings and editor/settings/storage modules was integrated into the [maintrunk] Created attachment 24034 [details]
commit log
|