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 91665 - Create more scalable registration for DataLoaders
Summary: Create more scalable registration for DataLoaders
Status: RESOLVED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Data Systems (show other bugs)
Version: 6.x
Hardware: All All
: P2 blocker (vote)
Assignee: Jaroslav Tulach
URL: http://wiki.netbeans.org/FitnessAgain...
Keywords: API, API_REVIEW
Depends on:
Blocks:
 
Reported: 2007-01-02 09:53 UTC by Jaroslav Tulach
Modified: 2008-12-22 10:55 UTC (History)
12 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
Proposed patch in core to include Lookup.getDefault().lookupResult(DataLoader.class) in the pool (12.25 KB, patch)
2007-01-02 09:58 UTC, Jaroslav Tulach
Details | Diff
Changes to support loaderless creation of DataObjects (10.24 KB, patch)
2008-06-03 19:59 UTC, Jaroslav Tulach
Details | Diff
Another improved patch: factory method and new file type wizard that uses it (34.43 KB, patch)
2008-06-06 16:23 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2007-01-02 09:53:59 UTC
When using org-openide-loaders.jar in tests or outside of NetBeans, one can 
register loaders in META-INF/services/org.openide.loaders.DataLoader. However 
when running in NetBeans, this registration is ignored, it should not be for 
consistency reasons and to allow migration from special manifest tags like 
OpenIDE-Module-Loader: true and serialization of loaders to loaders.ser.
Comment 1 Jaroslav Tulach 2007-01-02 09:58:49 UTC
Created attachment 36966 [details]
Proposed patch in core to include Lookup.getDefault().lookupResult(DataLoader.class) in the pool
Comment 2 Jaroslav Tulach 2007-01-02 10:00:08 UTC
Before integration I'll add a note to openide/loaders/apichanges.xml
Comment 3 Jesse Glick 2007-01-05 02:26:52 UTC
Could you please use diff -u next time? Meaning, currently, do a diff from the
command line, not the IDE.


Need docs changes. (**/apichanges.xml, **/doc-files/*)


apisupport's File Type wizard should be modified to generate M-I/s registration
rather than manifest registration, for 6.0+ platforms.


Do you plan to change the registration of existing loaders? Or deprecate
manifest-based registration?


Can we deprecate serialization to loaders.ser? Or is that part of a separate change?
Comment 4 Jesse Glick 2008-02-26 17:49:42 UTC
Is this still active?
Comment 5 Jaroslav Tulach 2008-02-26 19:57:44 UTC
Well, yes, someone could implement this. I do not thing I handle that for 6.1
Comment 6 Jaroslav Tulach 2008-05-13 22:51:22 UTC
Here is completely new attempt how to solve the problem with registration. Let's also tackle few performance aspects 
and make the whole infrastructure more scalable. Here is the proposal:

http://wiki.netbeans.org/FitnessAgainstForgetfulness

I'd like this to be review properly. So please comment on the page or here or on the mailing list. And let's have a 
conf call to resolve the outstanding issue next week.
Comment 7 Jaroslav Tulach 2008-05-13 22:53:26 UTC
> > Dne Friday 02 May 2008 21:03:56 Ivan Soleimanipour napsal(a):
> > > We'll end up eliminating these:
> The tags.

These tags will no longer be needed. Instead we will use regular positioning of files on layer: <file 
name="x.instance> <attr name="position" intvalue="9"/></file>, etc.

> But conflicts/mistakes in position ordering of loaders is
> going to have more impact than UI items.
> The ui item ordering with the "/" syntax was poorly conceived,
> but loader dependency is a bit cleaner ... more DAG-ish why
> not keep it?

I see. You are proposing to use relative ordering for loaders. Fine with me, it still works, we will just need to 
convince Jesse that it is the right thing.


Ivan sent me his comment via email, so I guess I need to formalize it somehow:

I01 Use relative ordering for positioning loaders
Comment 8 Jesse Glick 2008-05-13 23:35:26 UTC
I am in favor of this effort. I guess the idea is that the loader pool would do something to the effect of

String mimeType = fo.getMIMEType();
for (DataLoader dl : Lookups.proxy(Lookups.forPath("Loaders/" + mimeType + "/Factories").lookupAll(DataLoader.class),
Lookup.getDefault()) {
  // check dl on fo, use it if recognized
}

[JG01 vs. I01] Use numeric positioning as with everything else. It will be a relief to get away from the evil relative
positioning of Install-Before/After. Never worked very reliably.

[JG02, though perhaps not appropriate here?] The GUI in
http://ui.netbeans.org/docs/ui/OptionMenuDialogues/#FileTypesredesign looks a little dubious. Not clear how this will
map to declarative MIME resolvers. E.g. for Ant files, the default resolver uses a mixture of various techniques; no GUI
will really capture this adequately or allow it to be meaningfully edited.

[JG03] Would be nice if DataLoader did not extend SharedClassObject. Then we could have a factory to create loaders
whereby you would just need to specify the DataObject class (a constructor taking a FileObject parameter would be
called). But not very important.

[JG04] Ensure that so long as MIMEResolver's are correctly registered, DataObject recognition works identically inside
unit tests as in the full platform.
Comment 9 Jaroslav Tulach 2008-05-20 14:49:34 UTC
Here is answer to some of your questions:
http://wiki.netbeans.org/Diff.jsp?page=FitnessAgainstForgetfulness&r1=13&r2=11

Ondřeji, please attach the new UI spec, so JG02 is addressed. Thanks.

The review will be on Thursday, May 22, 2008, at 17.30CET, 7.30am pacific time, I'll send dial in note to everyone 
interested before the meeting.
Comment 10 Ondrej Langr 2008-05-21 10:39:42 UTC
New UI spec linked from wiki.
Comment 11 deniss 2008-05-24 16:15:09 UTC
I do not understand the "Open with" dialog, it will allow me to open unrecognized file as some mimetype but what if I
have two editors(Loaders) for the same mimetype? Is that supported? I will have two image viewers and then I want easy
way to switch between them. 

maybe I'm not getting something...
Comment 13 Ondrej Langr 2008-05-26 12:00:18 UTC
deniss: You would have to go to option panel to do this, which should be still way easier than current solution, AFAIK.
Can you clarify the case when you need this?
Comment 14 deniss 2008-05-27 00:12:12 UTC
it's ok, I understand
Comment 15 Andrei Badea 2008-05-30 14:33:48 UTC
Since the loaders now implicitly specify the MIME types they handle, I wanted to remove the ExtensionList initialization
from SQLDataLoader:

    ExtensionList extensions = new ExtensionList();
    extensions.addMimeType("text/x-sql");
    setExtensions(extensions);

However, apparently this code is still needed: UniFileLoader.findPrimaryFile() still checks ExtensionList. So then I
tried to remove SQLDataLoader entirely and replace it with an impl of DataObject.Factory. But I couldn't do that either,
since I didn't see an easy way to assign the layer-specified node actions ("Loaders/text/x-sql/Actions") to the SQL
nodes. At least not as easy as overriding DataLoader.actionsContext(). It seems such support is needed if we want to
attract people toward DataObject.Factory and away from DataLoader.
Comment 16 Jesse Glick 2008-05-30 17:41:44 UTC
To abadea: there are several options (I don't have a strong opinion as to which is preferable):

1. Fix UniFileLoader to recognize all files in case no extensions have been registered. E.g. change

return getExtensions().isRegistered(fo) ? fo : null;

to

ExtensionList l = (ExtensionList) getProperty(PROP_EXTENSIONS);
return l == null || l.isRegistered(fo) ? fo : null;

2. Change your loader to say

protected @Override FileObject findPrimaryFile(FileObject fo) {
  return fo;
}

3. Use DataObject.Factory and in your DataNode use Lookups.forPath to determine the action list. You need to look for
both Action and JSeparator instances (mapping the latter to null); probably this common idiom (repeated in a number of
places in NB) should be factored into a utility method somewhere.


BTW [JG05] The usefulness of the new DataObject.Factory is questionable given that the DataObject constructor requires
you to pass in a DataLoader! DataLoaderInLayerTest cheats by making SimpleFactory actually delegate to a DataLoader,
which seems to defeat the purpose. When I said during the review "check that DataObject.Factory actually works", I meant
actually try to use it in a realistic module and see if it works as expected (i.e. no DataLoader in sight), not just
write some unit test.
Comment 17 Jaroslav Tulach 2008-06-03 19:59:50 UTC
Created attachment 62332 [details]
Changes to support loaderless creation of DataObjects
Comment 18 Jaroslav Tulach 2008-06-03 20:03:16 UTC
I see the previous patch as an answer to JG05. Unless there are big objections, I'll use these API changes to create 
the new wizard for "File Type". It will not generate DataLoader and its BeanInfo anymore. It is possible not to 
generate a subclass of DataNode as well, I do not if I should generate it or not.
Comment 19 Jesse Glick 2008-06-04 05:36:22 UTC
Commenting on the new proposed API: where is MIMEFactory.java? Seems missing from patch.


[JG06] DLP.factory Javadoc messed up. Missing @param image. SFS.lI values should be URLs, e.g.
urlvalue="nbresloc:/.../foo.png". "&attr" should be "&lt;attr".


[JG07] What is the image param to DLP.factory for? If it's only for the DataNode icon, I think it should be deleted.
Comment 20 Jaroslav Tulach 2008-06-06 16:23:15 UTC
Created attachment 62481 [details]
Another improved patch: factory method and new file type wizard that uses it
Comment 21 Jesse Glick 2008-06-10 00:41:14 UTC
Looks like you ignored the comment about SFS.lI in JG06.