Bug 209998

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)

Created attachment 121180 [details]
Proposed Patch

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).

I didn't merge the proposed patch for testing, but the implementation strategy appears to be a clean, working solution.

Jan, please, check new registration of filter for Java files. Thank you.

If there are no objections, I will integrate on Friday. Thanks.

(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).

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.)

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.

(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.

Created attachment 122537 [details]
Proposed Patch

Updated Patch

(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?

(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.

> @ServiceProvider(service=FileFilterProvider.class)
Or better FileFilterFactory.

(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...

Created attachment 122623 [details]
Proposed Patch

Class FileFilterSupport moved to package org.openide.filesystems.
Thank you, Standa.

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?

(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.

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

Created attachment 122801 [details]
Proposed Patch

Updated patch (modified module cnd.utils)

Created attachment 122802 [details]
Alternative patch

Updated alternative patch (modified module cnd.utils)

> 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)?

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)

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.

(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?

(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.

(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.

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.

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.

Created attachment 123197 [details]
Screenshot - List of File Filters in File Chooser

List of File Filters in File Chooser (after application of the patch).

(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

Created attachment 123201 [details]
Proposed Patch

AS01 - Thank you, fixed.

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.

Thank you for your comments.
If there are no objections or suggestions, I will integrate on Wednesday.

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.

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!

(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.

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