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.

Bug 59784 - Editor/MimeLookup and Editor/Settings APIs Review
Summary: Editor/MimeLookup and Editor/Settings APIs Review
Status: RESOLVED FIXED
Alias: None
Product: editor
Classification: Unclassified
Component: Settings (show other bugs)
Version: 4.x
Hardware: All All
: P1 blocker (vote)
Assignee: apireviews
URL:
Keywords: API_REVIEW
Depends on: 62674 62675
Blocks: 59009 59458 60995
  Show dependency tree
 
Reported: 2005-06-09 13:29 UTC by Martin Roskanin
Modified: 2007-11-05 13:42 UTC (History)
5 users (show)

See Also:
Issue Type: TASK
Exception Reporter:


Attachments
The zip containing javadoc documentations including arch documents (183.16 KB, application/x-compressed)
2005-06-09 13:51 UTC, Martin Roskanin
Details
EMMA tests coverage (38.84 KB, application/x-compressed)
2005-06-13 09:13 UTC, Martin Roskanin
Details
updated EMMA tests coverage (37.30 KB, application/x-compressed)
2005-06-13 16:43 UTC, Martin Roskanin
Details
Updated javadoc and arch documentations (190.81 KB, application/x-compressed)
2005-06-15 10:32 UTC, Martin Roskanin
Details
editor/settings/storage arch document (43.39 KB, text/html)
2005-08-16 15:18 UTC, Martin Roskanin
Details
commit log (18.71 KB, text/plain)
2005-08-18 11:31 UTC, Martin Roskanin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Roskanin 2005-06-09 13:29:30 UTC
This task covers review of the new API, SPI and implementation of
editor/mimelookup module and API review of editor/settings module.
The prototype is ready in the 'editor_api' branch. The new modules 
exist under cvs/editor/mimelookup and cvs/editor/settings folders.
Comment 1 Martin Roskanin 2005-06-09 13:51:30 UTC
Created attachment 22587 [details]
The zip containing javadoc documentations including arch documents
Comment 2 Martin Roskanin 2005-06-09 14:20:57 UTC
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.
Comment 3 Martin Roskanin 2005-06-09 14:54:12 UTC
Reassigning to apireviews@netbeans.org
in accordance with Step 2 of:
http://openide.netbeans.org/tutorial/review-steps.html
Comment 4 Martin Roskanin 2005-06-10 09:05:24 UTC
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.
Comment 5 Jaroslav Tulach 2005-06-10 09:33:27 UTC
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" 
 
 
Comment 6 Jan Lahoda 2005-06-10 10:41:22 UTC
I support Y01. It seems to me that MimeLookup.childLookup(String) can be
replaced by a factory method getMimeLookup(String[] mimeTypes).
Comment 7 Martin Roskanin 2005-06-13 09:13:08 UTC
Created attachment 22624 [details]
EMMA tests coverage
Comment 8 Martin Roskanin 2005-06-13 09:19:25 UTC
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.

Comment 9 Miloslav Metelka 2005-06-13 09:47:55 UTC
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.
Comment 10 Martin Roskanin 2005-06-13 16:42:40 UTC
Fixed mime lookup rebuilding problems
Adding tests for templates lookuping and re-creating deleted files
Deleting obsolete class
Attaching updated emma tests coverage
Comment 11 Martin Roskanin 2005-06-13 16:43:34 UTC
Created attachment 22645 [details]
updated EMMA tests coverage
Comment 12 Miloslav Metelka 2005-06-13 17:11:24 UTC
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.
Comment 13 Martin Roskanin 2005-06-15 10:32:44 UTC
Created attachment 22685 [details]
Updated javadoc and arch documentations
Comment 14 Jaroslav Tulach 2005-06-17 15:53:06 UTC
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. 
Comment 15 Martin Roskanin 2005-06-21 09:03:40 UTC
 
>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.

Comment 16 Martin Roskanin 2005-08-16 15:15:11 UTC
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.
Comment 17 Martin Roskanin 2005-08-16 15:18:01 UTC
Created attachment 23945 [details]
editor/settings/storage arch document
Comment 18 Jaroslav Tulach 2005-08-17 11:14:02 UTC
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. 
 
Comment 19 Martin Roskanin 2005-08-17 12:58:17 UTC
>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.
Comment 20 Jan Jancura 2005-08-17 13:26:12 UTC
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
Comment 21 Jaroslav Tulach 2005-08-17 13:39:25 UTC
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. 
Comment 22 Martin Roskanin 2005-08-17 13:57:35 UTC
>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
Comment 23 Miloslav Metelka 2005-08-17 14:06:16 UTC
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*.
Comment 24 Martin Roskanin 2005-08-18 11:30:03 UTC
editor/settings and editor/settings/storage modules was integrated into the
[maintrunk]
Comment 25 Martin Roskanin 2005-08-18 11:31:10 UTC
Created attachment 24034 [details]
commit log