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

Bug 275996

Summary:

PreferenceManager works incorrectly with activities

Product:

[RT] RAP

Reporter:

Igor Pavlenko <dopperst>

Component:

Workbench

Assignee:

Project Inbox <rap-inbox>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

Version:

1.2

Target Milestone:

1.2 RC1

Hardware:

All

OS:

All

Whiteboard:

Attachments:

Description	Flags
Patch to fix problem	none
Alternate patch, keeps WorkbenchPlugin#getPreferenceManager intact	none

Description Igor Pavlenko

2009-05-12 22:38:18 EDT

Due to RAP architecture the WorkbenchPlugin#getPreferenceManager works
incorrect. For example - let some activity to filter out some preference
page in some cases (it can be necessary if you have to entry points and the
first one allows users access to some preference pages and the other one -
denies)


There is a simple solution: move the implementation of
WorkbenchPlugin#getPreferenceManager to Workbench#getPreferenceManager.


It also will be better to mark WorkbenchPlugin#getPreferenceManager as
deprecated.

I attached the patch.

Comment 1 Igor Pavlenko

2009-05-12 22:39:58 EDT

Created attachment 135488 [details]
Patch to fix problem

Comment 2 Rüdiger Herrmann

2009-05-14 09:12:36 EDT

Created attachment 135775 [details]
Alternate patch, keeps WorkbenchPlugin#getPreferenceManager intact

Igor, could you verify if this patch also works for you?

Comment 3 Igor Pavlenko

2009-05-15 03:16:11 EDT

Hi. I wrote about PreferenceManager, not about PerspectiveRegistry. But the second is important too.

But you approach (to make some classes session singletons) is not so good as it looks like. It may cause some problems. For example, if you allows getting PreferenceManager from WorkbenchPlugin class, be sure that somebody will try to use this method in the activator of some plugin, or in static context, etc.

I think that a lot of methods in WorkbenchPlugin should be marked as deprecated with additional comments. But it does not matter for my application.

Comment 4 Rüdiger Herrmann

2009-05-15 05:03:35 EDT

(In reply to comment #3)
> Hi. I wrote about PreferenceManager, not about PerspectiveRegistry. But the
> second is important too.
Did I accidentially mention the PerspectiveRegistry somewhere?

> 
> But you approach (to make some classes session singletons) is not so good as it
> looks like. It may cause some problems. For example, if you allows getting
> PreferenceManager from WorkbenchPlugin class, be sure that somebody will try to
> use this method in the activator of some plugin, or in static context, etc.
I agree, there is potential for misuse. But to be compatible with RCP we already provide methods that are accessible from static/application context in a lot of places. In addition the WorkbenchPlugin is internal. Clients that use this class directly are at risk anyway.

> 
> I think that a lot of methods in WorkbenchPlugin should be marked as deprecated
> with additional comments. But it does not matter for my application.
Right, it would be a good idea to document, which scope (application, session) a method or class has.

Thanks for your contribution, applied the second patch to CVS HEAD.

Comment 5 Igor Pavlenko

2009-05-15 08:03:07 EDT

Thank you, the patch and fix looks good.

> > Hi. I wrote about PreferenceManager, not about PerspectiveRegistry. But the
> > second is important too.
> Did I accidentially mention the PerspectiveRegistry somewhere?
It is my error. I saw a notification about applying patch of the other bug and a notification about creating patch to this one. This two events mixed up in my head. So I discovered only the source from CVS, where found a similar fix for PerspectiveRegistry.

> In addition the WorkbenchPlugin is internal. Clients that use
> this class directly are at risk anyway.
But the PlatformUI#getPreferenceStore() is not. So, maybe it should be deprecated.