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

Bug 447530

Summary:	[CommonNavigator] activebydefault nonvisible commonfilter will no longer be activated once persistFilterActivationState() is called
Product:	[Eclipse Project] Platform	Reporter:	Ghaith Hachem <ghaith.hachem>
Component:	Runtime	Assignee:	Martin Schreiber <Martin.SCHREIBER>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	Martin.SCHREIBER, mistria, pierre.dufay
Version:	4.4.1
Target Milestone:	4.8 M5
Hardware:	All
OS:	All
See Also:	https://git.eclipse.org/r/35109 https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d895c1a28181b69514a8e37bbf5d93cbd5cd1d29
Whiteboard:

Description Ghaith Hachem

2014-10-16 06:31:54 EDT

If a commonFilter is defined as activeByDefault=true and isVisible=false the filter will work as expected on a new workspace, however as soon as persistFilterActivationState() is called, this filter will no longer function. This might also mean that new filters are ignored once the persist function is called, I tested this with setting the current none visible filter to visible and creating a new filter with the same handler code.

The reason is that persistFilterActivationState() saves active filters, which are defined (in restoreFilterActivation()) as either the filters from org.eclipse.ui.navigator.ProjectExplorer.filterActivation if it exists or the visible filters that are active by default, never the non visible filters active by default.

Is this behavior intended?

Comment 1 Ghaith Hachem

2014-10-17 02:50:45 EDT

More tracing shows that the actual search for visible filter happens in persistFilterActivationState where the isVisibleInUi() is checked, this leads to the exclusion of all other filters even though they are indeed enabled

Comment 2 Martin Schreiber

2014-10-20 02:49:27 EDT

Pushed a patch for review: https://git.eclipse.org/r/35109

As Ghaith mentioned, when persisting the active filter ids, the "active by default" non visible filters must also be stored in the pref store, otherwise they won't get activated after restart (restore).

Changed the code, so that only non visible and not active by default filters are skipped when persiting the filter ids.

Comment 3 Martin Schreiber

2014-10-20 04:18:50 EDT

After cross-checking with Ghaith, I submitted another patchset. 
The idea of the FilterActivationPreferenceKey seems to be to store the ids of the filters, the user has activated in the UI. So its not a good idea to store there the "visibleInUI=false;activeByDefault=true" filters there as well (which I used in the first patchset).

The new patchset always adds the "visibleInUI=false;activeByDefault=true" filters to the active filters in the restoreFilterActivation() method. After that, all the visible in ui filters are handled by checking the FilterActivationPreferenceKey. If the key exists, add all filters defined there, if not, add all visibleInUI filters that are activeByDefault.

Comment 4 Pierre Dufay

2017-08-02 10:34:20 EDT

Where do we stand on this bug? I am stuck on same problem. If status is still new, this means that a kind of workaround is actually done by developers, any hint?

Comment 5 Ghaith Hachem

2017-08-02 10:42:43 EDT

(In reply to Pierre Dufay from comment #4)
> Where do we stand on this bug? I am stuck on same problem. If status is
> still new, this means that a kind of workaround is actually done by
> developers, any hint?

This is still an issue for us, we personally didn't find any workaround. The fix submitted by Martin should probably still be valid but I did not check if the code changed much in Oxygen, maybe we should rebase it on the current master state or solve the merge conflict?

Comment 6 Pierre Dufay

2017-08-03 02:35:06 EDT

Thanks for update. Please where could I get this patch?

Comment 7 Ghaith Hachem

2017-08-03 03:20:24 EDT

(In reply to Pierre Dufay from comment #6)
> Thanks for update. Please where could I get this patch?

The fix I mentioned is the one linked in comment 2 (https://git.eclipse.org/r/#/c/35109/)

You could maybe override the INavigatorContentService implementation in the relevant navigator and sneak in your own INavigatorFilterService that has the fix implemented. The INavigatorFilterService interface is however marked with @noimplement and @noextend so proceed with caution.

Comment 8 Mickael Istria

2017-12-20 04:57:16 EST

Patch in https://git.eclipse.org/r/35109 just needs minor tweaks, but it should be easily doable to get it in 4.8.M5.
I'm sorry we missed this patch for 3 years...

Comment 9 Eclipse Genie

2017-12-20 12:25:54 EST

Gerrit change https://git.eclipse.org/r/35109 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d895c1a28181b69514a8e37bbf5d93cbd5cd1d29

Comment 10 Mickael Istria

2017-12-20 12:26:36 EST

Patch merged! Thanks a lot Martin, and sorry for this having been stale for no good reason.

Comment 11 Martin Schreiber

2017-12-21 03:27:39 EST

(In reply to Mickael Istria from comment #10)
> Patch merged! Thanks a lot Martin, and sorry for this having been stale for
> no good reason.

Thanks for reviewing and merging. Don't worry about the delay!