Community
Participate
Working Groups
Currently the filter(*) method is final and prevents filter extension for the FilteredTree. In 3.4 we should provide API to allow PatterFilter to be extended. PW
One way to do this would be to extract the main part of filter() to a new protected method doFilter() which is called from filter() after doing the null check, and making filter() non-final.
Another API prevents extension is the final method select(*). Although it delegates to isElementVisible(*), the parentElement parameter is not passed into isElementVisible(*). The parentElement is useful in some filtering cases when determining if a leaf is visible. One example would be: if a parent matches, then also make all its children as matching.
Hitesh is now responsible for watching bugs in the [Viewers] component area.
How about adding the ability to support java Pattern regular expressions? Just support a setPattern that does Pattern p = Pattern.compile(pattern,Pattern.CASE_INSENSITIVE); and a match(String) that does: return p.matcher(string).find(); Pretty simple mod, and then allows you to find things like; ^a starts with a ^(a|b) starts with a or starts with b a$ ends with a (a|b)$ ends with a or ends with b
Created attachment 205158 [details] Patch for extending PatternFilter This patch may be made against my local git repositiory with wrong formatting options (tabs instead of spaces). I'm not sure about this and would fix it if necessary.
I made an enhancement request for PDE UI which requires to extend PatternFilter and therefore fixed this bug as suggested by Boris Bokowski. The entry that would rely on this patch is Bug 360894.
Hi Platform UI team, I know you guys are busy, but please consider reviewing this patch for M4.
I can't apply the patch using either EGit or command-line Git client. error: patch failed: bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/ dialogs/PatternFilter.java:10 error: bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/dialogs/Patter nFilter.java: patch does not apply - Could you attach a patch against Git master or Git R3_development branch? - Could you describe the changes you are making and their purpose, especially API changes?
Created attachment 206574 [details] Source code of PatternFilter The patch above was made against R_3_7_maintenance branch. Since the patch doesn't work against other branches, here is the plain java class.
- Could you describe the changes you are making and their purpose, especially API changes? The patch fulfills the requirements filed by this enhancement request (Bug 186404). I implemented the issues from the Description, Comment 1 and partly Comment 2. Please compare my latest attachment against the actual PatternFilter and you will find that I have - removed the final modifier from PatternFilter#filter(Viewer viewer, Object parent, Object[] elements) - extracted it's content into PatternFilter#doFilter(Viewer viewer, Object parent, Object[] elements) according to COMMENT 1 by Boris Bokowski - removed the final modifier from PatternFilter#select(Viewer viewer, Object parentElement, Object element) according to COMMENT 2 by Li Ding The changes are necessary because of Bug 360894, which requires the extension of PatternFilter (to org.eclipse.pde.internal.ui.search.ExtensionsPatternFilter). A working example of this enhancement to the ManifestEditor can be found here: http://www.stereotron.com/pde-enhancement
Created attachment 206613 [details] Patch without formatting changes I cleaned up the patch from the formatting changes. 1) The patch increases visibility for a number of fields in the PatternFilter class: protected Map cache -> does not seem to be used by ExtensionsPatternFilter protected StringMatcher matcher -> does not seem to be used protected boolean useEarlyReturnIfMatcherIsNull -> does not seem to be used foundAnyCache useCache First, the fields that aren't used in the PDE part of the enhancement - is there any reason to make them API? Next. the foundAnyCache & useCache. I'd much prefer some wrapper method(s) added in the PatternFilter to access cache. 2) The patch increases visibility of a number of methods, two of those aren't seem to be used by the PDE part: protected boolean match(String string) protected boolean isAnyVisible() Is there a reason to make those APIs? 3) The patch splits filter() method into filter() and doFilter(). However, in the PDE code you still have to override both of those methods. That defeats the purpose of introduction of a doFilter(). Either split has to be done differently, or there is no point in adding doFilter().
I might have been overhasty in making too many methods API. 1) If it's not used, it's not necessary. I don't understand why it's so important to prevent extension, but that's not my business. I just want to contribute to the ManifestEditor. The patch to the PDE enhancement Bug is not the latest. This is newer: http://www.stereotron.com/pde-enhancement/PDE-Enhancement.zip Now I need access to foundAnyCache & useCache in ExtensionsPatternFilter. Access with a wrapper method is fine, as long as I can use the put method. 2) no there is none. I have simply forgotten that I made everything extendable before uploading the patch. 3) this is also not necessary. I only need to overwrite filter()
Created attachment 206792 [details] Smaller patch Hi Sascha, will this work for you?
(In reply to comment #13) > Created attachment 206792 [details] > Smaller patch > > Hi Sascha, will this work for you? No, I need to overwrite filter() as this is the initial request for this Bug, I need to access foundAnyCache as well as useCache, and select should also be not final. If it's so important to not make PatternFilter extendable in a sensible way, why not let FilteredTree accept an interface as the filter and make PatternFilter an implementation for that interface? This is would enable alternating behaviour of the filtering while keeping PatternFilter unchanged.
(In reply to comment #14) > (In reply to comment #13) > > Created attachment 206792 [details] [details] > > Smaller patch > > > > Hi Sascha, will this work for you? > > No, I need to overwrite filter() as this is the initial request for this Bug The patch makes it overridable: - public final Object[] filter(Viewer viewer, Object parent, Object[] elements) { + public Object[] filter(Viewer viewer, Object parent, Object[] elements) { > I need to access foundAnyCache as well as useCache, Please see proposed patch: - private Map foundAnyCache = new HashMap(); + protected Map foundAnyCache = new HashMap(); - private boolean useCache = false; + protected boolean useCache = false; > and select should also be not final. - public final boolean select(Viewer viewer, Object parentElement, + public boolean select(Viewer viewer, Object parentElement, Am I missing something?
Sorry, I confounded the sides of the patch. It's all fine now. Thank you!
The "Smaller patch" released in Git: 3.8 http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R3_development&id=f740fba07449fe293059dbb042e1e486cb6b6632 4.2: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d6709d67620716c0eb872bd732d49cbc1d79662f (In reply to comment #4) > How about adding the ability to support java Pattern regular expressions? In 3.x stream the workbench plugin is expected to run on the Foundation VM which does not support regex. At any rate, this should be possible by either overriding PatternFilter#isLeafMatch() and #isParentMatch(), or by starting with the parent class (ViewerFilter).
The new APIs need some more love: /** * Maps parent elements to TRUE or FALSE * * @since 3.8 */ protected Map foundAnyCache = new HashMap(); /** * Specifies if caching of filter results should be used. * * @since 3.8 */ protected boolean useCache = false; The Javadocs must tell how these fields are used and when they can be read or modified by subclasses, and how the cache will be used. In general, we don't make field accessible without a very good reason (since this unnecessarily leaks implementation details). There's already a package-private method setUseCache(boolean). Why can't you use that one? I didn't look at the concrete client, but I guess you could also replace the direct accesses to foundAnyCache with a new method with a self-explanatory name that surfaces the required information.
> The Javadocs must tell how these fields are used and when they can be read or > modified by subclasses, and how the cache will be used. In general, we don't > make field accessible without a very good reason (since this unnecessarily > leaks implementation details). Getters for foundAnyCache and useCache would also be OK, but Bug 360894 requires to fill foundAnyCache with ExtensionsPatternFilter#filter(). > There's already a package-private method setUseCache(boolean). Why can't you > use that one? I need to read, not write. > I didn't look at the concrete client, but I guess you could also > replace the direct accesses to foundAnyCache with a new method with a > self-explanatory name that surfaces the required information. Here I need to write, not read. I tried hard to minimize the changes to PatternFilter, but enhancing the behaviour of the extensions page of the manifest editor requires at least this. Another way to prevent the extension of PatternFilter: - make PatternFilter implement an interface ( - replace PatternFilter with that interface in FilteredTree#init(int treeStyle, PatternFilter filter) This way the FilteredTree and several other classes could be reused with another PatternFilter and not extending the current one. Please see http://www.stereotron.com/pde-enhancement for an overview. Also an usability problem with FilteredTree is that filtered tree elements loose their children. It wasn't easy to modify the behaviour in the Manifest Editor / ExtensionsPage, so that filtered matches have children AND these child elements are instantly collapsed. With large tree structures this is essential for gaining a quick overview on the search result.
This bug has been fixed with 3.8M4 and can be set to FIXED. It already works with the patch required for Bug 360894 Thanks!
> This bug has been fixed with 3.8M4 and can be set to FIXED. No. I've reopened this bug because the API in HEAD doesn't have Eclipse quality, see comment 18. If e.g. field foundAnyCache is really necessary as API, then its lifecycle needs to be specified. An API specification should be all a client needs to know to use the API. This is clearly not the case here, and the accessible field is a problem for future evolution. An alternative to a full-blown API is to add @noreference and then add an API filter to the PDE plug-in, or make it a friend.
It works without access to foundAnyCache, I've learned. But nevertheless it requires to call setUseCache() to bypass the caching and implement a cache on my own. Would it be possible to make at least setUseCache() protected? I still need filter() and select() to be non-final.
Or what about a constructor PatternFilter(boolean useCache)? The behaviour of the class wouldn't change during the lifecycle, yet it could be determined at instantiation and setUseCache() could remain package private.
Now I also don't require to access useCache thanks to the snippet 'How to show children of matched tree nodes in a Filtered Tree' provided at http://rcpexperiments.blogspot.com I'd find this to be a useful (and maybe: optional) behaviour for the PatternFilter. Missing children in filtered results are annoying, because one has to clear the filter text and look for the former results in the whole tree.
Created attachment 211149 [details] Minimal patch for bug 186404 The original proposal: only filter() made non-final This would un-block Bug 360894
(In reply to comment #25) > Created attachment 211149 [details] > Minimal patch for bug 186404 > > The original proposal: only filter() made non-final > This would un-block Bug 360894 Thanks! I made changes in R3: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R3_development&id=0bf67b68cc62c58a7ac3f12856ba70db74236dc9 and master: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=f51f7f0457d3727f0f24997ce54c0eaceef38f35
Verified that updated code is present in 4.2 (I20120313-0610) and 3.8 (I20120312-1300) builds.
