Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 425867

Summary: [CommonNavigator] Implementation of Binding isVisibleExtension not excluding as expected
Product: [Eclipse Project] Platform Reporter: Matthieu Wipliez <matthieu.wipliez>
Component: UIAssignee: Paul Webster <pwebster>
Status: VERIFIED FIXED QA Contact: Paul Webster <pwebster>
Severity: major    
Priority: P3 CC: daniel_megert, rstheo
Version: 4.4Keywords: helpwanted
Target Milestone: 4.4 M6   
Hardware: PC   
OS: Windows 8   
Whiteboard:
Attachments:
Description Flags
Simple test case of includes/excludes pattern
none
Contributor patch none

Description Matthieu Wipliez CLA 2014-01-16 05:54:22 EST 
Summary: the 'excludes' pattern in org.eclipse.ui.navigator.viewer.viewerActionBinding/viewerContentBinding is basically useless.

The documentation states that 'excludes' have precedence over 'includes' as one would expect both intuitively. It's just that it does not work, and it turns out this is just because the code says otherwise.

Proposed fix: in class org.eclipse.ui.internal.navigator.extensions.Binding
method boolean isVisibleExtension(String anExtensionId)
=> move the loop over excludePatterns *before* the loop over includePatterns.

This bug report relates to https://bugs.eclipse.org/bugs/show_bug.cgi?id=232860 entitled "[CommonNavigator] excludePatterns field in navigator extensions Binding class is pointless.", but it was never fixed.

Severity 'major' even if this is a trivial fix, because instead of including org.eclipse.ui.navigator.resources.actions.* and just filtering out a couple of actions that I would like to provide myself, I have to write the inclusion in extension:

org.eclipse.ui.navigator.resources.actions.EditActions
org.eclipse.ui.navigator.resources.actions.RefactorActions
org.eclipse.ui.internal.navigator.resources.actions.OpenActionProvider
org.eclipse.ui.navigator.resources.GotoActions
org.eclipse.ui.navigator.resources.GoIntoActions
org.eclipse.ui.navigator.resources.PropertiesActionProvider
org.eclipse.ui.navigator.resources.WorkManagementActionProvider
org.eclipse.ui.navigator.resources.ResourceMgmtActions
org.eclipse.ui.navigator.resources.UndoRedoActionProvider
org.eclipse.ui.navigator.resources.WorkingSetActions
Comment 1 Paul Webster CLA 2014-01-16 05:58:08 EST 
There's no one looking at this component at the moment, but we accept contributions.  See http://wiki.eclipse.org/Platform_UI/How_to_Contribute

PW
Comment 2 Matthieu Wipliez CLA 2014-01-16 06:56:13 EST 
Ok. So be it, it's my first time contributing to Eclipse!
Discovered and used Gerrit for the first time, signed the Contributor License Agreement, and pushed the change.
You can see it at https://git.eclipse.org/r/#/c/20702/

What happens now?
Comment 3 Paul Webster CLA 2014-01-16 07:49:47 EST 
Thanks for the contribution.

We have community contribution days, the next one in about 2 weeks.  I'll look at the contribution then.

PW
Comment 4 Dani Megert CLA 2014-01-20 08:36:29 EST 
Matthieu, can you provide a test case or small test bundle that allows to verify your fix?
Comment 5 Matthieu Wipliez CLA 2014-01-21 11:35:52 EST 
Created attachment 239194 [details]
Simple test case of includes/excludes pattern

Attached a sample bundle that adds a viewerActionBinding including all actions from org.eclipse.ui.navigator.resources.* but excluding org.eclipse.ui.navigator.resources.NewActions

The expected behavior is that the "New" menu does not show when right-clicking in the ProjectExplorer.
Comment 6 Paul Webster CLA 2014-01-24 13:59:16 EST 
(In reply to Matthieu Wipliez from comment #5)
> Created attachment 239194 [details]
> Simple test case of includes/excludes pattern

Thanks Matthieu.  We'll be examining contributions at the beginning of next week.  As Dani mentioned, could you please update your credentials in the copyright header at the top of the file in https://git.eclipse.org/r/#/c/20702/ ?

Thanks,
PW
Comment 7 Matthieu Wipliez CLA 2014-01-25 13:59:02 EST 
Hi Paul,

I did, but I've been unable to push my changes: they have been rejected telling me I should "sign off" on the contribution (even though there were proper change-id and signed-off-by fields).

How should I proceed?

Matthieu

(In reply to Paul Webster from comment #6)
> (In reply to Matthieu Wipliez from comment #5)
> > Created attachment 239194 [details]
> > Simple test case of includes/excludes pattern
> 
> Thanks Matthieu.  We'll be examining contributions at the beginning of next
> week.  As Dani mentioned, could you please update your credentials in the
> copyright header at the top of the file in
> https://git.eclipse.org/r/#/c/20702/ ?
> 
> Thanks,
> PW
Comment 8 Paul Webster CLA 2014-01-25 18:04:23 EST 
If you use git format-patch (or any kind of diff on that commit) and attach the patch I can upload it for you.

Can you check with a git log?  Sometimes if you have a merge node + your commit change it gets rejected (we do fast-forward only commits in platform ui, so you might need to rebase on origin/master)

Thanks,
PW
Comment 9 Matthieu Wipliez CLA 2014-01-27 04:49:48 EST 
Created attachment 239332 [details]
Contributor patch
Comment 10 Matthieu Wipliez CLA 2014-01-27 04:51:11 EST 
I tried to rebase and push but I have the same results, so I'm going with the patch approach, please find it attached.

Matthieu

(In reply to Paul Webster from comment #8)
> If you use git format-patch (or any kind of diff on that commit) and attach
> the patch I can upload it for you.
> 
> Can you check with a git log?  Sometimes if you have a merge node + your
> commit change it gets rejected (we do fast-forward only commits in platform
> ui, so you might need to rebase on origin/master)
> 
> Thanks,
> PW
Comment 12 Paul Webster CLA 2014-03-04 12:44:47 EST 
In 4.4.0.I20140303-2000

Matthieu, could you also confirm it now works the way you expect?

PW
Comment 13 Paul Webster CLA 2014-03-05 05:56:13 EST 
*** Bug 232860 has been marked as a duplicate of this bug. ***
Comment 14 Matthieu Wipliez CLA 2014-03-05 09:25:38 EST 
Yes I confirm it works as expected with integration build 4.4.0.I20140303-2000

Matthieu

(In reply to Paul Webster from comment #12)
> In 4.4.0.I20140303-2000
> 
> Matthieu, could you also confirm it now works the way you expect?
> 
> PW