Bug 209998 - OpenFileDialogFilter should be part of a public SPI
OpenFileDialogFilter should be part of a public SPI
Status: RESOLVED FIXED
Product: utilities
Classification: Unclassified
Component: Open File
-S1S-
All All
: P3 (vote)
: 7.3
Assigned To: Jaroslav Havlin
issues@utilities
: API, API_REVIEW
Depends on:
Blocks: 181989
  Show dependency treegraph
 
Reported: 2012-03-23 12:53 UTC by 280Z28
Modified: 2012-09-01 01:10 UTC (History)
9 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Proposed Patch (58.52 KB, patch)
2012-06-21 15:30 UTC, Jaroslav Havlin
Details | Diff
Proposed Patch (22.35 KB, patch)
2012-07-30 15:14 UTC, Jaroslav Havlin
Details | Diff
Proposed Patch (22.67 KB, patch)
2012-08-01 09:58 UTC, Jaroslav Havlin
Details | Diff
Alternative patch (24.43 KB, patch)
2012-08-01 14:26 UTC, Jaroslav Havlin
Details | Diff
Proposed Patch (27.89 KB, patch)
2012-08-06 15:43 UTC, Jaroslav Havlin
Details | Diff
Alternative patch (31.09 KB, patch)
2012-08-06 15:43 UTC, Jaroslav Havlin
Details | Diff
Proposed Patch (78.59 KB, patch)
2012-08-16 08:56 UTC, Jaroslav Havlin
Details | Diff
Screenshot - List of File Filters in File Chooser (8.74 KB, image/png)
2012-08-16 09:28 UTC, Jaroslav Havlin
Details
Proposed Patch (78.59 KB, patch)
2012-08-16 12:53 UTC, Jaroslav Havlin
Details | Diff
Proposed Patch (83.40 KB, patch)
2012-08-29 16:24 UTC, Jaroslav Havlin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description 280Z28 2012-03-23 12:53:00 UTC
In implementing support for a new language, if a developer wishes to have the newly supported files visible in the File -> Open dialog then they must currently use an SPI from a non-public package [1].

To improve compatibility of an obviously necessary feature of any language extension, this feature needs to be available in a public SPI.

[1] https://blogs.oracle.com/geertjan/entry/open_file_dialog_in_the
Comment 1 280Z28 2012-03-23 12:56:03 UTC
As part of the transition, the language-specific filters currently implemented alongside the SPI should be moved to modules specific to those languages. In particular, the Java-specific filter obfuscates the UI of NetBeans applications which do not ship with Java support. (Part of issue #181989)
Comment 2 Jaroslav Havlin 2012-06-21 15:30:31 UTC
Created attachment 121180 [details]
Proposed Patch
Comment 3 Jaroslav Havlin 2012-06-21 15:47:41 UTC
Please review.

The patch introduces new module api.openfile that contains class
org.netbeans.spi.openfile.OpenFileDialogFilter.

The class was copied from org.netbeans.modules.openfile.OpenFileDialogFilter
(module utilities). The original class was marked as deprecated.

Registration of filter for Java files was moved to module java.source
(org.netbeans.modules.java.JavaOpenFileDialogFilter).
Comment 4 280Z28 2012-07-10 14:55:32 UTC
I didn't merge the proposed patch for testing, but the implementation strategy appears to be a clean, working solution.
Comment 5 Jaroslav Havlin 2012-07-24 13:32:53 UTC
Jan, please, check new registration of filter for Java files. Thank you.
Comment 6 Jaroslav Havlin 2012-07-24 13:34:05 UTC
If there are no objections, I will integrate on Friday. Thanks.
Comment 7 Jaroslav Havlin 2012-07-30 07:32:54 UTC
(In reply to comment #6)
> If there are no objections, I will integrate on Friday. Thanks.
I'm sorry, I will rather wait for more comments or verifications, as this is a full review (not fast).
Comment 8 Jan Lahoda 2012-07-30 08:35:23 UTC
Sorry for late answer.

I must say I don't particularly like having a NetBeans-specific SPI class, that extends standard Java SPI class. I would personally prefer to:
-lookup standard FileFilters, presumably using a named lookup (Lookups.forPath) so that they do not create mess in the global lookup
-as that of course could negatively impact the UI consistency, create support classes/methods to help create consistent display names and behavior.

An ideal support method would IMO be a factory method, taking a mime-type, or a factory method taking a display name and list of extensions. But the standard ServiceProvider annotation does not support registration of factory methods, not even for named lookups, AFAIK. It would be possible to create a specialized annotation and a processor for it, but seems like an overkill to me. So, the best I can imagine so far is to create methods like:
FileFilterSupport.constructFilterDisplayName(String displayName, String...extensions)
FileFilterSupport.accept(File file, String...extensions)
or even something like (which would automagically infer the extensions from the extensions registered for the given mime type, not do the filtering based on the mime type):
FileFilterSupport.constructFilterDisplayName(String mimeType)
FileFilterSupport.accept(File file, String mimeType) //watch out for a collision with the above accept method

The clients would then do:
@ServiceProvider(service=FileFilter.class, path="<some-path>")
public class TXTFilter extends FileFilter {
     private static final String[] EXTENSIONS = new String[] {".txt"};
     public boolean accept(File f) {
          return FileFilterSupport.accept(f, EXTENSIONS);
     }
     public String getDescription() {
          return FileFilterSupport.constructFilterDisplayName(<bundle-lookup>, EXTENSIONS);
     }
}

(If you would find a place where a compile time constant for <some-path> would fit, that would be nice.)
Comment 9 Jaroslav Havlin 2012-07-30 09:14:36 UTC
Thank you very much for your comments.
Do you think that creating a new module (api.openfile) for FileFilterSupport is a good idea (some classes related to file opening can be put there later), or should it be created in an existing module? I prefer new module, but I'm not sure.
Comment 10 Jan Lahoda 2012-07-30 09:48:48 UTC
(In reply to comment #9)
> Thank you very much for your comments.
> Do you think that creating a new module (api.openfile) for FileFilterSupport is
> a good idea (some classes related to file opening can be put there later), or
> should it be created in an existing module? I prefer new module, but I'm not
> sure.

New module may cause discoverability issues - how will the clients find out they need to depend on api.file module? I guess there won't be that many clients, so with a tutorial this probably would not be a problem. Using an existing module would make the intended use a little bit easier, though. Maybe openide.awt would be a reasonable place? No strong opinion.

BTW, the constant for the path might probably be in the FileFilterSupport, even though that is not completely clean. Note that the value of the compile-time constant is also an API, not only the constant itself.
Comment 11 Jaroslav Havlin 2012-07-30 15:14:18 UTC
Created attachment 122537 [details]
Proposed Patch

Updated Patch
Comment 12 Jaroslav Havlin 2012-07-30 15:22:00 UTC
(In reply to comment #10)
> Using an existing module would make the intended use a little bit easier,
> though. Maybe openide.awt would be a reasonable place? No strong opinion.
I've moved FileFilterSupport to openide.awt.
I think it is a good place, both existing clients already depend on openide.awt.
Standa, please, is it OK?

> BTW, the constant for the path might probably be in the FileFilterSupport, even
> though that is not completely clean. Note that the value of the compile-time
> constant is also an API, not only the constant itself.
The path is now stored in constant FileFilterSupport.FILE_FILTER_LOOKUP_PATH. Its value is "OpenFileDialogFilter". Is it correct?
Comment 13 Jaroslav Havlin 2012-07-31 07:26:18 UTC
(In reply to comment #8)
> An ideal support method would IMO be a factory method, taking a mime-type, 
> or a factory method taking a display name and list of extensions. But the 
> standard ServiceProvider annotation does not support registration of factory 
> methods, not even for named lookups, AFAIK.
Would a provider interface be a good alternative?
Example:

@ServiceProvider(service=FileFilterProvider.class)
public class JavaFileFilterProvider implements FileFilterProvider {
  @Override
  public FileFilter createFileFilter() { // or getFileFilter() ?
    return FileFilterSupport.createFilterForMimeType("text/x-java");
    //also FileFilterSupport.createFilterForExtensions("Text Files", "txt");
  }
}
It requires a new SPI class, but the code is quite compact. What do you think?
Would the providers need to be registered using named lookup, too, or global lookup would be correct in this case?
Thank you.
Comment 14 Jaroslav Havlin 2012-07-31 07:28:16 UTC
> @ServiceProvider(service=FileFilterProvider.class)
Or better FileFilterFactory.
Comment 15 Stanislav Aubrecht 2012-07-31 08:17:15 UTC
(In reply to comment #12)
> (In reply to comment #10)
> > Using an existing module would make the intended use a little bit easier,
> > though. Maybe openide.awt would be a reasonable place? No strong opinion.
> I've moved FileFilterSupport to openide.awt.
> I think it is a good place, both existing clients already depend on
> openide.awt.
> Standa, please, is it OK?

org.openide.filesystems package in openide.filesystems might be a better place. There are already some file-related utility classes in that module, FileChooserBuilder...
Comment 16 Jaroslav Havlin 2012-08-01 09:58:12 UTC
Created attachment 122623 [details]
Proposed Patch

Class FileFilterSupport moved to package org.openide.filesystems.
Thank you, Standa.
Comment 17 Jaroslav Havlin 2012-08-01 14:26:41 UTC
Created attachment 122642 [details]
Alternative patch

Registration based on FileFilterFactory, as described in comment 8.

Please compare this path (factory method registration) with the previous one (registration of FileFilters to named lookup). Which is more appropriate?
Comment 18 Jaroslav Havlin 2012-08-01 14:32:10 UTC
(In reply to comment #17)
> Registration based on FileFilterFactory, as described in comment 8.
Sorry, in comment 13.


Jarda, please, can you verify that putting FileFilterFactory or FileFilterSupport to module openide.filesystems is OK? Thank you.
Comment 19 Jan Lahoda 2012-08-06 10:39:34 UTC
Using a Factory might be somewhat cleaner, but seems to be a little bit overkill for this purpose to me. No strong opinion.

BTW, did you peek at the C/C++ implementations, to see if they are doing something peculiar, or if these can be rewritten as well (just asking)? The current list of subclasses of org.netbeans.modules.openfile.OpenFileDialogFilter is:
org.netbeans.modules.cnd.utils.filters.CndOpenFileDialogFilter$Adapter
org.netbeans.modules.cnd.utils.filters.CndOpenFileDialogFilter$CFilter
org.netbeans.modules.cnd.utils.filters.CndOpenFileDialogFilter$CppFilter
org.netbeans.modules.cnd.utils.filters.CndOpenFileDialogFilter$FortranFilter
org.netbeans.modules.cnd.utils.filters.CndOpenFileDialogFilter$HeaderFilter
org.netbeans.modules.cnd.utils.filters.CndOpenFileDialogFilter$QtFilter
org.netbeans.modules.cnd.utils.filters.CndOpenFileDialogFilter$ResourceFilter
org.netbeans.modules.openfile.FileChooser$JavaFilesFilter
org.netbeans.modules.openfile.FileChooser$TxtFileFilter
org.netbeans.modules.openfile.OpenFileDialogFilter$ExtensionFilter
Comment 20 Jaroslav Havlin 2012-08-06 15:43:05 UTC
Created attachment 122801 [details]
Proposed Patch

Updated patch (modified module cnd.utils)
Comment 21 Jaroslav Havlin 2012-08-06 15:43:56 UTC
Created attachment 122802 [details]
Alternative patch

Updated alternative patch (modified module cnd.utils)
Comment 22 Jaroslav Havlin 2012-08-06 15:51:33 UTC
> BTW, did you peek at the C/C++ implementations, to see if they are doing
> something peculiar, or if these can be rewritten as well (just asking)?
Thank you for hint. I did not check C/C++ implementations before, but it seems that they can be rewritten quite easily (the original, non-factory solutions is more straightforward in this case).

Alexander, please, can you review the proposed change (both alternatives)?
Comment 23 Jaroslav Tulach 2012-08-07 13:03:40 UTC
Y01 A bit big change in filesystems which I don't want to accept without tests. As far as I can say there is only one method tested from the Support class which is otherwise full of methods I don't see belonging into any API.

Y02 Instead of FileFilterFactory use annotation and processor. Not only the usage of the API will be simpler, but you will be able to show list of all registered filters without instantiating a single class from CND or Java modules. 

Y03 I am not entirely sure why the API is put into openide.filesystems module, but when it is there, should not it be used by FileChooserBuilder. E.g. add method useDefaultFilters(true)
Comment 24 Jaroslav Tulach 2012-08-08 09:00:49 UTC
Yesterday I was in pub with Jan Lahoda and he explained me some thoughts that I have missed before. According to Jan our primary goal is to show "subset of known mimetypes in the file selection dialog". If that is true, then there is a nice addition to Y02: Expand existing annotation processor for 
http://bits.netbeans.org/dev/javadoc/org-openide-filesystems/org/openide/filesystems/MIMEResolver.ExtensionRegistration.html
http://bits.netbeans.org/dev/javadoc/org-openide-filesystems/org/openide/filesystems/MIMEResolver.Registration.html
to also specify whether this mime type should be listed in 
http://bits.netbeans.org/dev/javadoc/org-openide-filesystems/org/openide/filesystems/FileChooserBuilder.html
dialogs. You can do it by adding one (default) attribute or by new annotation that would accompany the already existing ones.

The good thing is that instead of tons of new classes in our API, we'll add just an attribute or an annotation.
Comment 25 Jaroslav Havlin 2012-08-09 07:55:54 UTC
(In reply to comment #24)
> [...] You can do it by adding one (default) attribute or by new annotation
> that would accompany the already existing ones.
>
> The good thing is that instead of tons of new classes in our API, we'll add
> just an attribute or an annotation.

Thank you, good idea. It works well with MIMEResolver.ExtensionRegistration, but MIMEResolver.Registration does not provide display names for registered MIME types, which would look quite ugly.

I'm not quite sure that display name in ExtensionRegistration is name of the MIME type (Java Files), as it can be name of the resolver (Java Files Resolver).

A think a new annotation processor is still needed.
What about this registration:


Case 1: Registration with Extension MIME Resolver

@MIMEResolver.ExtensionRegistration(
    position=100, displayName="#JavaResolver", extension="java",
    mimeType="text/x-java")
@FileFilterSupport.Registration(displayName="#JavaFilesFilter")

Filtering of files is simple, as we have the extension.


Case 2: Registration with generic MIME Resolver

@MIMEResolver.Registration(displayName="#ExtResolver", position=214,
    resource="resources/mime-resolver-ext-based.xml")
@FileFilterSupport.Registration(extensions={"text/x-c++=C,cpp,c++", "text/x-h=H,h,hpp"},
  mimeDisplayNames={"text/x-c++=#CPPSourceFiles", "text/x-h=#HeaderFiles"})

In this case, we need display names for MIME types, and also list of extensions,
as it is part of display name and parsing the description file may not be convenient here (because content of files is checked).

(Another option is to somehow specify display names in the MIME Resolver 
itself.)

For file filtering, we instantiate the resolver, and accept files for which the resolver resolves the selected MIME type.


Case 3: Register a custom implementation of File Filter.
@FilteFilterSupport.Registration
public class MyFileFilter extends FileFilter {
 // implementation
}

This should not be supported any more. Correct?
Comment 26 Alexander Simon 2012-08-09 08:52:32 UTC
(In reply to comment #25)
> Case 2: Registration with generic MIME Resolver
> 
> @MIMEResolver.Registration(displayName="#ExtResolver", position=214,
>     resource="resources/mime-resolver-ext-based.xml")
> @FileFilterSupport.Registration(extensions={"text/x-c++=C,cpp,c++",
> "text/x-h=H,h,hpp"},
>   mimeDisplayNames={"text/x-c++=#CPPSourceFiles", "text/x-h=#HeaderFiles"})
> 
It seems this case does not applicable for CND.
CND has a own dynamically changed list of extensions. See org.netbeans.modules.cnd.utilsMIMEExtensions class.
Comment 27 Jaroslav Havlin 2012-08-09 10:47:04 UTC
(In reply to comment #26)
> It seems this case does not applicable for CND.
> CND has a own dynamically changed list of extensions. See
> org.netbeans.modules.cnd.utilsMIMEExtensions class.

OK, is the following solution acceptable?

Case 1: Register with MIME extension registration

@MIMEResolver.ExtensionRegistration(
    position=100, displayName="#JavaResolver", extension="java",
    mimeType="text/x-java")
@FileFilterSupport.Registration
public class SomeClass extends Anything { /...

Lets assume that the display name for resolver can be used as description in the filter.


Case 2: Register custom FileFilter implementation

@FileFilterSupport.Registration
public class MyFileFilter extends FileFilter {
  // ...
  public String getDescription() {
    // ...
    return FileFilterSupport.constructDescription(desc, exts);
  }
  public boolean accept(File f()) {
    // ...
    return FileFilterSupport.accept(f, exts);
  }
}

FileFilterSupport probably does not need (public) methods that take mimeType as parameter.

What about using FileFilterRegistration instead of FileFilterSupport.Registration?

Thank you for your comments.
Comment 28 Jaroslav Havlin 2012-08-16 08:56:27 UTC
Created attachment 123188 [details]
Proposed Patch

Y02 After discussion with Jaroslav Tulach, I've implemented only declarative registration of file filters, custom implementations are not supported.

There is new attribute showInFileChooser in @MIMEResolver.Registration (and in @MIMEResolver.ExtensionRegistration), that specifies list of file choosers that accept files resolved by the resolver.

Example:
@NbBundle.Messages({"PlainResolver=Text Files", "ResourceFiles=Resource Files"})
@MIMEResolver.ExtensionRegistration(
    mimeType="text/plain",
    position=140,
    displayName="#PlainResolver",
    extension={ "TXT", "txt" },
    showInFileChooser={"#PlainResolver", "#ResourceFiles"}

> CND has a own dynamically changed list of extensions.
The filter will show extensions registered with the resolver, and also custom extensions assigned to the MIME type by the user.
If it is slow, we will try to find some ways to improve it.

Y03 There is new method addDefaultFileFilters() in FileChooserBuilder that adds all registered filters to new file chooser.
I think it is semantically correct (as it accompany existing addFileFilter), but if you think useDefaultFilters(true) is better, I'll change it.

Abstract class OpenFileDialogFilter is still supported, as there are some tutorials about it on the web (so there may be clients with implementation dependency). The class is deprecated and should be removed after some time.

Note: Class FileChooser in module utilities was removed, FileChooserBuilder is used instead.

Y01 - Most of public methods were removed, test was updated for the new implementation.
Comment 29 Jaroslav Havlin 2012-08-16 09:14:03 UTC
I am going to change MIMEResolver registrations in some other modules.
Can you please review changes in the patch relevant to your modules?

 - cnd.script, cnd: Alexander Simon
 - core.ide:        Jaroslav Tulach
 - html:            Marek Fukala
 - image:           Jan Peska
 - xml:             Svata Dedic

If you are not owners of the modules, please let me know.

Thank you very much.
Comment 30 Jaroslav Havlin 2012-08-16 09:28:38 UTC
Created attachment 123197 [details]
Screenshot - List of File Filters in File Chooser

List of File Filters in File Chooser (after application of the patch).
Comment 31 Alexander Simon 2012-08-16 09:54:16 UTC
(In reply to comment #28)
> Created attachment 123188 [details]
> Proposed Patch
AS01: All warnings:
WARNING [org.openide.filesystems.Ordering]: Found same position
should be fixed
Comment 32 Jaroslav Havlin 2012-08-16 12:53:14 UTC
Created attachment 123201 [details]
Proposed Patch

AS01 - Thank you, fixed.
Comment 33 Jaroslav Tulach 2012-08-17 08:22:00 UTC
I like the minimal API change. I am glad a bit of code in utilities module could be deleted. The new behavior will be more efficient and hopefully satisfiable for most of the use-cases.
Comment 34 Jaroslav Havlin 2012-08-27 11:53:17 UTC
Thank you for your comments.
If there are no objections or suggestions, I will integrate on Wednesday.
Comment 35 Jaroslav Havlin 2012-08-29 16:24:53 UTC
Created attachment 123710 [details]
Proposed Patch

Patch updated for current sources (openide.filesystems version changed to 8.1).
Specification version in dependent modules updated too.

I'll integrate tomorrow, after running build and tests again.
Comment 36 Jaroslav Havlin 2012-08-30 12:23:58 UTC
Patch applied as http://hg.netbeans.org/core-main/rev/0a7f7d6927ea

Legacy filters removed in http://hg.netbeans.org/core-main/rev/5fd50492bf1c
(also removed dependency on utilities from cnd.utils).

Thanks for you help!
Comment 37 Jaroslav Havlin 2012-08-30 13:32:36 UTC
(In reply to comment #36)
> Patch applied as http://hg.netbeans.org/core-main/rev/0a7f7d6927ea

I'm sorry, build of module ide.ergonomics started to fail because of problems with message keys:
/hudson/workdir/jobs/NB-Core-Build/workspace/ide.ergonomics/src-ant/org/netbeans/modules/ide/ergonomics/ant/common-ergonomics.xml:26: Cannot parse layers: key MakeResolver from org/netbeans/modules/cnd/makefile/loaders/Bundle.*properties was already defined among [org/netbeans/modules/cnd/asm/core/dataobjects/Bundle.*properties, ...]

Fixed in http://hg.netbeans.org/core-main/rev/3a062d4d5449
Please check the fix (several bundle messages changed). Thanks.
Comment 38 Quality Engineering 2012-09-01 01:10:44 UTC
Integrated into 'main-golden', will be available in build *201209010001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/0a7f7d6927ea
User: Jaroslav Havlin <jhavlin@netbeans.org>
Log: #209998: OpenFileDialogFilter should be part of a public SPI


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo