Bug 200855 - Option Value Handler called with APPLY on simple edit
Summary: Option Value Handler called with APPLY on simple edit
Status: NEW
Alias: None
Product: CDT
Classification: Tools
Component: cdt-build (show other bugs)
Version: 4.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: cdt-build-inbox@eclipse.org CLA
QA Contact: Jonah Graham CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-08-22 14:18 EDT by Doug Schaefer CLA
Modified: 2020-09-04 15:24 EDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Doug Schaefer CLA 2007-08-22 14:18:53 EDT
I have defined a value handler for my option, which is an enum option, that I'd like to have called when the user hits OK. But I'm finding that it's only getting called when I change it's value (with the event being APPLY) or when the user hits Apply. It does not get called when I hit OK, or worse, when I hit CANCEL.

Am I using value handlers wrongly here?
Comment 1 Doug Schaefer CLA 2007-08-22 14:52:46 EDT
I'm not sure if this is related but the ToolSettingsTab does not have a handler for performOK. It's not obvious how the option settings get saved properly, but apparently they do.

What I'm really trying to do is to clear out the discovery info when a certain set of options change. Mikhail S has suggested an option value handler for that, but apparently it's doesn't work as I thought.

What I really need is just a listener to option value changes. Do we have such a system?
Comment 2 Oleg Krasilnikov CLA 2007-08-23 06:08:27 EDT
Sorry, this mechanism seems to be badly-documented...

Really, property tabs are not required to have specific 
performOK() handler.
Instead, they use given IConfigurationDescription instance
to save all changes to it just after they made in UI.
It allows to reflect changes on other pages (if it needs)
immediately, not waiting for OK press.

In case of OK, modified IConfigurationDescription instance
is applied to the project. In case of Cancel, it's simply
discarded. 

When user presses Apply, changes from active page only are
saved. To do it, corresponding values are copied from one IConfigurationDescription instance to another. It looks 
like modification of "another" values, that's why your
handler is activated.

On the other hand, we can add performOK handler to specific 
tab, like EnvironmentTab.java does, for example.

Sorry once more for inconvenience.
  
Comment 3 Mikhail Sennikovsky CLA 2007-08-30 07:41:14 EDT
Hi Doug,

I guess the root cause of your problem is in "inconsistency" of the "clone" configuration concept used by the option settings UI (and most other CDT project settings UI) and the scanner discovery settings mechanism that (historically) knows nothing about the "clone" configuration concept.

Some background: while project settings modifications all changes are being made to the "clone" configuration which is actually a copy of the "cached" project configuration. 
All UI is communicating directly to this configuration thus ensuring the UI settings integrity, e.g. in case one setting change results in some other settings change, the change will be reflected in UI.
In case of OK the "clone" configuration gets applied.
In case of CANCEL the "clone" configuration gets simply discarded.
In case of APPLY all settings from the "clone" configuration get copied to the other "clone" configuration which then gets applied.

From the option value handler mechanism perspective the "EVENT_APPLY" event is posted when the option value is modified. As a result you are receiving this event when the option value is modified in UI and when the "APPLY" button is pressed since in this case the other "clone" configuration (to be applied) receives all option values from the currently used one. And you see no events on CANCEL because in this case the "clone" gets simply discarded and on OK because in this case the "clone" gets applied with the current modifications being made with no additions option modifications.
 This approach works fine till you are using/adjusting settings within the "clone" configuration context, which is not true for the scanner discovery mechanism which (historically) knows nothing about the "clone" configuration concept. 

A quick solution I could think of is to present additional option value handler events and functionality:

1. For handling configuration APPLY (both for whith Ok and Apply buttons) present a EVENT_CONFIG_APPLY option value handler event that would be posted to all value handlers when the configuration gets applied.
2. The correct cancel notification implementation seems more difficult since configuration settings get canceled by simply discarding the clone configuration. Even if we implement some event notification posted to the handler by the UI, this event will not be posted, e.g. when modifying and discarding settings programmatically. I guess we could introduce, e.g. the close() method to the ICProjectDescription that would tell that the current settings copy is not used any more. In this case the close will be propagated to all configurations and to value handlers. In this case we could introduce additional EVENT_CONFIG_CANCEL event that would be posted to all value handlers on cancel.

Note: all the above modifications do not seem to require any API breakage and thus could be implemented in the maintenance release.

Mikhail
Comment 4 Doug Schaefer CLA 2007-08-30 14:12:52 EDT
I like the CONFIG_APPLY option. That's essentially what I was looking for, a notification that the change got applied to the config and I should update the system state.

In the meantime, the simple APPLY is working, it just does extra work when not needed if the user changes the setting multiple times before applying, or if the user cancels.
Comment 5 Mikhail Sennikovsky CLA 2007-08-31 06:48:25 EDT
(In reply to comment #4)
> I like the CONFIG_APPLY option. That's essentially what I was looking for, a
> notification that the change got applied to the config and I should update the
> system state.
> 
> In the meantime, the simple APPLY is working, it just does extra work when not
> needed if the user changes the setting multiple times before applying, or if
> the user cancels.
OK, I guess implementing CONFIG_APPLY should be an easy task.. I'll have a look..

Comment 6 Leo Treggiari CLA 2007-08-31 14:07:35 EDT
I'm not sure how CONFIG_APPLY would help, but maybe I'm misunderstanding how this currently works.

Looking at the code, EVENT_APPLY gets fired when ManagedBuildManager.setOption is called and when OK or APPLY is selected on the options property page.  Does this mean that EVENT_APPLY is fired twice for options when OK or APPLY is pressed?

ManagedBuildManager.setOption takes a configuration and an option and can be called by anyone, not just because of property page value changes.  I'm assuming that if the options you (Doug) care about were to change programmatically, outside of property page handling, you would want to know about it.

So, I don't think that adding a CONFIG_APPLY event that only gets fired when OK or APPLY is selected really fixes the problem.

I have a vague memory that at one point we used the IConfiguration.isTemporary mechanism to distinguish between the case when the value of an option in a "clone" vs. "real" configuration was changed.  I'm having some trouble understanding how the "temporary" flag is currently used - it seems to get set more ofthen than I would have expected.  Again, this is just from looking at the code and not trying any experiments...

Leo
Comment 7 Doug Schaefer CLA 2007-08-31 15:39:05 EDT
(In reply to comment #6)
>> Looking at the code, EVENT_APPLY gets fired when ManagedBuildManager.setOption
> is called and when OK or APPLY is selected on the options property page.  Does
> this mean that EVENT_APPLY is fired twice for options when OK or APPLY is
> pressed?

I saw it getting called when the user changed the enum option. But *not* when they hit OK or APPLY. An isTemporary wouldn't help in that situation since I don't know when it becomes permanent.

Maybe this points to a missing call when OK or APPLY is hit. The old build UI seems to do this. The new one doesn't. That's what lead me to see what performOK was doing.
Comment 8 Leo Treggiari CLA 2007-08-31 15:46:42 EDT
> I saw it getting called when the user changed the enum option. But *not* when
> they hit OK or APPLY. An isTemporary wouldn't help in that situation since I
> don't know when it becomes permanent.

The way that isTemporary would help is that when you received an EVENT_APPLY invocation on an Option, you could look at its "parent" configuration and ask whether it is "temporary".  If it is "temporary", then the option is part of one of these "clone" configurations and you could ignore it.  If it is not "temporary", then the "real" configuration is being changed.  But, as I mentioned, I have not verified the usage of the "temporary" flag.

Comment 9 Doug Schaefer CLA 2007-08-31 15:49:44 EDT
(In reply to comment #8)
> > I saw it getting called when the user changed the enum option. But *not* when
> > they hit OK or APPLY. An isTemporary wouldn't help in that situation since I
> > don't know when it becomes permanent.
> The way that isTemporary would help is that when you received an EVENT_APPLY
> invocation on an Option, you could look at its "parent" configuration and ask
> whether it is "temporary".  If it is "temporary", then the option is part of
> one of these "clone" configurations and you could ignore it.  If it is not
> "temporary", then the "real" configuration is being changed.  But, as I
> mentioned, I have not verified the usage of the "temporary" flag.

I can see how it would help. The problem is that I don't get an event when the change is made permanent. If I got that event, then I would definitely use the isTemporary check.
Comment 10 Leo Treggiari CLA 2007-08-31 15:54:34 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > > I saw it getting called when the user changed the enum option. But *not* when
> > > they hit OK or APPLY. An isTemporary wouldn't help in that situation since I
> > > don't know when it becomes permanent.
> > The way that isTemporary would help is that when you received an EVENT_APPLY
> > invocation on an Option, you could look at its "parent" configuration and ask
> > whether it is "temporary".  If it is "temporary", then the option is part of
> > one of these "clone" configurations and you could ignore it.  If it is not
> > "temporary", then the "real" configuration is being changed.  But, as I
> > mentioned, I have not verified the usage of the "temporary" flag.
> I can see how it would help. The problem is that I don't get an event when the
> change is made permanent. If I got that event, then I would definitely use the
> isTemporary check.

I see.  That part (not getting called when the change is made permanent) just seems like a bug that should be fixed.
Comment 11 Leo Treggiari CLA 2007-09-02 21:19:59 EDT
Sorry about my confusion.  I didn't realize that the property page handling had changed to apply the entire temporary (cloned) configuration in a single operation.  I believe in the past, the changed values from the temporary configuration were re-applied to the "real" configuration when OK or Apply was selected.

Here's what I think is the relevant stack trace:

Thread [main] (Suspended (breakpoint at line 747 in Configuration))	
	Configuration.applyToManagedProject(ManagedProject) line: 747	
	ManagedProject.applyConfiguration(Configuration) line: 589	
	ConfigurationDataProvider.applyConfiguration(ICConfigurationDescription, ICConfigurationDescription, CConfigurationData, IModificationContext, IProgressMonitor) line: 158	
	CProjectDescriptionManager.applyData(CConfigurationDescriptionCache, ICConfigurationDescription, CConfigurationData, SettingsContext, IProgressMonitor) line: 1679	
	CConfigurationDescriptionCache.applyData(CSettingEntryFactory, SettingsContext) line: 136	
	CProjectDescription.applyDatas(SettingsContext) line: 217	
	SetCProjectDescriptionOperation.executeOperation() line: 66	
	SetCProjectDescriptionOperation(CModelOperation).execute() line: 342	
	SetCProjectDescriptionOperation(CModelOperation).run(IProgressMonitor) line: 607	
	Workspace.run(IWorkspaceRunnable, ISchedulingRule, int, IProgressMonitor) line: 1797	
	SetCProjectDescriptionOperation(CModelOperation).runOperation(IProgressMonitor) line: 639	
	CProjectDescriptionManager.setProjectDescription(IProject, ICProjectDescription, int, IProgressMonitor) line: 1291	
	CProjectDescriptionManager.setProjectDescription(IProject, ICProjectDescription, boolean, IProgressMonitor) line: 1254	
	CProjectDescriptionManager.setProjectDescription(IProject, ICProjectDescription) line: 1247	
	CoreModel.setProjectDescription(IProject, ICProjectDescription) line: 1374	
	CDTPropertyManager.performOkForced(Object) line: 128	
	CDTPropertyManager.performOk(Object) line: 102	
	AbstractPage$5.run(IProgressMonitor) line: 485	
	WorkspaceModifyDelegatingOperation.execute(IProgressMonitor) line: 68	
	WorkspaceModifyOperation$1.run(IProgressMonitor) line: 101	
	Workspace.run(IWorkspaceRunnable, ISchedulingRule, int, IProgressMonitor) line: 1797	
	WorkspaceModifyDelegatingOperation(WorkspaceModifyOperation).run(IProgressMonitor) line: 113	
	ModalContext.runInCurrentThread(IRunnableWithProgress, IProgressMonitor) line: 369	
	ModalContext.run(IRunnableWithProgress, boolean, IProgressMonitor, Display) line: 313	
	ProgressMonitorDialog.run(boolean, boolean, IRunnableWithProgress) line: 495	
	Page_BuildSettings(AbstractPage).performSave(int) line: 494	
	Page_BuildSettings(AbstractPage).performOk() line: 425	
	PreferenceDialog$12.run() line: 922	
	SafeRunner.run(ISafeRunnable) line: 37	
	Platform.run(ISafeRunnable) line: 850	
	JFaceUtil$1.run(ISafeRunnable) line: 46	
	SafeRunnable.run(ISafeRunnable) line: 193	
	PropertyDialog(PreferenceDialog).okPressed() line: 902	
	PropertyDialog(FilteredPreferenceDialog).okPressed() line: 374	
	PropertyDialog(PreferenceDialog).buttonPressed(int) line: 230	

So there are at least two changes which result:

1.  The option value notification is gone when OK is pressed.  This is wrong.
2.  If a change were made programmatically to the real config while the cloned config was active, it would be lost when the clone replaced the real config.  We could argue whether this is good or bad, but it seems like a change.

Are there any other "change" callbacks, in addition to option value handlers, that get called for the temporary config?

Leo
Comment 12 Mikhail Sennikovsky CLA 2007-09-03 04:54:57 EDT
Hi Leo, Doug,

As I mentioned in the Comment#3 , historically EVENT_APPLY is not APPLY actually :). It is more like ON_CHANGE in that it is being fired when an option value is changed, but not applied.
As a result you should be seeing event being fired _only_ in case an option value is being changed that occurs in the following situations:
1. user modifies the value of a temporary configuration via UI.
2. the APPLY button is pressed -> the option values get copied to the configuration to be applied.

The APPLY event does not get fired in case of OK is pressed since in this case no option values get changed. Instead the "clone" configuration simply gets applied becoming non-temporary.

We could implement posting the APPLY event always when the configuration gets applied (no matter whether option values are changed or not), and, as Leo mentions, a call-back would be able to check the isTemporary flag to determine whether this is the case of applying configuration or the case of an option value change, but I tend to think that making a separate CONFIG_APPLY event would make more sense since
1. on_apply event seem to have quite a different meaning than the on_change one
2. two separate event seem to be more intuitive to the users. Having a separate CONFIG_APPLY event a tool-integrator would not need to guess that the isTemporary should be checked to get the "real" meaning of the APPLY event. This would also help integrators to more clearly understand the meaning of the APPLY event.

What do you think?

Mikhail
Comment 13 Mikhail Sennikovsky CLA 2007-09-03 06:33:46 EDT
(In reply to comment #11)
Hi Leo,

Sorry I've overlooked your comment#11 . Please see my comments embedded below.

> So there are at least two changes which result:
> 
> 1.  The option value notification is gone when OK is pressed.  This is wrong.
I think that a separate CONFIG_APPLY would be better since it would have some different meaning than the current APPLY one (see comment#12 for detail).

> 2.  If a change were made programmatically to the real config while the cloned
> config was active, it would be lost when the clone replaced the real config. 
> We could argue whether this is good or bad, but it seems like a change.
This is true. Actually from my perpective making changes to the "real" config is not correct (and has never been correct) because the "real" config should be for the read-only only use only, e.g. to be used for building, etc. Otherwise there is a rick that, e.g. the "build" job operates with a "real" config while it is being modified by the UI. More than that there is a risk of the ConcurrentModificationException since the buildDefinitions use thread-unsafe structures such as ArrayList, HashMap, etc.
In the 4.0 the "clone" config concept became a core concept rather than the UI one. I.e. instead of calling
IManagedBuildInfo info = ManagedBuildManager.getBuildInfo(project);
IManagedProject mProj = info.getManagedProject();
IConfiguration cfg = mProj.getConfiguration*(...);
which obtains the "real" configuration

One might use
ICProjectDescriptionManager mngr = CoreModel.getDefault().getProjectDescriptionManager();
ICProjectDescription des = mngr.getProjectDescription(project, true /*writable description*/);
ICConfigurationDescription cfgDes = des.getConfiguration*(...);
IConfiguration cfg = ManagedBuildManager.getConfigurationForDescription(cfgDes);
to obtain the "clone" configuration.
and to call mngr.setProjectDescription(project, des); to apply the changes.

Note that modifying the "real" configurations is also supported. I.e. when the ManagedBuildManager.saveBuildInfo() is called, modifications made to the "real" IConfigurations get propagated to the core and saved to the .cproject file.

> Are there any other "change" callbacks, in addition to option value handlers,
> that get called for the temporary config?
There is the ICProjectDescriptionListener call-back that among the other things (depending on the event mask specified on call-back registration) might get fired for the case when the "clone" ICProjectDescription is created and/or in case the "clone" ICProjectDescription gets applied.

Mikhail
Comment 14 Leo Treggiari CLA 2007-09-04 11:37:45 EDT
(In reply to comment #12)
> Hi Leo, Doug,
> As I mentioned in the Comment#3 , historically EVENT_APPLY is not APPLY
> actually :). It is more like ON_CHANGE in that it is being fired when an option
> value is changed, but not applied.

I don't agree with your historical perspective...  ;-)

EVENT_APPLY is fired out of the ManagedBuildManager.setOption methods.  These methods were intended to be the primary public way to change the value of a configuration's option.  The name EVENT_APPLY was probably a poor choice.  It was intended to mean that the value of the option has changed, and not to be specific to the "Apply" button in the UI.  As in Doug's case, there can be code that needs to know that the value of an option has changed regardless of how it gets changed.  For example, in Visual Studio, a lot of option value changes are made by automated scripts, for example, when creating a project.  I assume that changing the default value of options is something that is done quite frequently by code written for the new template engine functionality.  

The BuildOptionsSettingsUI.PerformOk used to be called when OK or Apply was pressed for the Tool Settings UI.  It uses ManagedBuildManager.setOption to apply the changes that were made in the cloned configuration to the real configuration, and so, EVENT_APPLY was fired there also (on the real configuration).

> We could implement posting the APPLY event always when the configuration gets
> applied (no matter whether option values are changed or not), and, as Leo
> mentions, a call-back would be able to check the isTemporary flag to determine
> whether this is the case of applying configuration or the case of an option
> value change, but I tend to think that making a separate CONFIG_APPLY event
> would make more sense since
> 1. on_apply event seem to have quite a different meaning than the on_change one
> 2. two separate event seem to be more intuitive to the users. Having a separate
> CONFIG_APPLY event a tool-integrator would not need to guess that the
> isTemporary should be checked to get the "real" meaning of the APPLY event.
> This would also help integrators to more clearly understand the meaning of the
> APPLY event.
> What do you think?

What I want is a callback to be made when the value of the real configuration's option has changed.  I don't think that I care whether the change was made from the UI or otherwise.

The "special case" is the handling of option value changes while the property page UI is active.  The design was to make handling these value changes as similar as possible to "real" configuration changes.  As Oleg pointed out in a prevous reply, we want the UI to reflect what the state of the configuration will be if OK or Apply gets selected, including any side-effects of value changes such as enabling/disabling other options.

If there are 2 separate events, one needs to signify that the value of an option in a "real" configuration has changed (no matter how), and the other can be specific to "not yet applied" UI changes.
Comment 15 Mikhail Sennikovsky CLA 2007-09-05 07:38:28 EDT
(In reply to comment #14)
Hi Leo,

> EVENT_APPLY is fired out of the ManagedBuildManager.setOption methods.  These
> methods were intended to be the primary public way to change the value of a
> configuration's option.  The name EVENT_APPLY was probably a poor choice.  It
> was intended to mean that the value of the option has changed, and not to be
> specific to the "Apply" button in the UI.  As in Doug's case, there can be code
> that needs to know that the value of an option has changed regardless of how it
> gets changed.  For example, in Visual Studio, a lot of option value changes are
> made by automated scripts, for example, when creating a project.  I assume that
> changing the default value of options is something that is done quite
> frequently by code written for the new template engine functionality.  
That's correct. And actually the option event handling mechanism is not linked to the UI in any way. As you say, the APPLY notification is made from the ManagedBuildManager.setOptoin(), which in the project properties UI case gets called when the APPLY button is pressed (and option values get coppied) and when option UI control gets modified resulting in setOption() for the underlying temporary configuration.
Note that the UI use-cases mentioned above are used just as samples and since UI is using a public settings API there is no difference between programmatic and UI handling of the setOption() and other calls.

> The BuildOptionsSettingsUI.PerformOk used to be called when OK or Apply was
> pressed for the Tool Settings UI.  It uses ManagedBuildManager.setOption to
> apply the changes that were made in the cloned configuration to the real
> configuration, and so, EVENT_APPLY was fired there also (on the real
> configuration).
The reason the APPLY event in not fired when the OK is pressed is because in case of an OK the temporary configuration simply becomes a regular one replacing the previous non-temporary configuration. The two reasons why this is done in this way rather than copying all settings from one configuration to another is performance and settings consistency. Having the configuration simply applied ensures that the configuration settings getting stored are exactly the same as displayed/modified in UI (or programmatically). Otherwise we would encounter problems with setting modification ordering, e.g. in case the configuration tool-chain was modified (e.g. replaced with another one), the "copy settings" mechanism would need to perform the "copy" tool-chain modification before, e.g. copying option values.

So in case of an OK, the "EVENT_APPLY" is called for the "temporary" configuration when the setOption is called for its options as a result of the UI option changes, and when the OK button is pressed, that configuration simply gets stored with no additional setOption() calls.


> What I want is a callback to be made when the value of the real configuration's
> option has changed.  I don't think that I care whether the change was made from
> the UI or otherwise.
> 
> The "special case" is the handling of option value changes while the property
> page UI is active.  The design was to make handling these value changes as
> similar as possible to "real" configuration changes.  As Oleg pointed out in a
> prevous reply, we want the UI to reflect what the state of the configuration
> will be if OK or Apply gets selected, including any side-effects of value
> changes such as enabling/disabling other options.
That's how it works now. And as mentioned above, there is nothing specific about using the option modification mechanism by UI. Handlers do get notified with the APPLY event each time the setOption() is called (no matter whether the change is done via UI or programmatically) and on settings apply, the temporary configuration simply becomes a "reguilar"(non-temporary) one getting stored in the ManagedBuildInfo.
As I mentioned above this works OK in case the changes are made within the configuration concept (the way the handler call-backs are expected to behave), but in Doug's case the handler intends to modify the project global settings outside of the configuration scope, which is not intended to be supported.
 
> If there are 2 separate events, one needs to signify that the value of an
> option in a "real" configuration has changed (no matter how), and the other can
> be specific to "not yet applied" UI changes.
I don't agree with such a discretion completely, because changing settings of the "real" configuration directly does not make those settings to be applied to the entire system. Instead they would be seen by the build system only until the saveBuildInfo() is called. So, e.g. this approach would not work in Doug's case because what I guess he needs is to modify system's settings while the settings get applied. In this sense the "real" configuration will work in a similar way as the "temporary" one in the sense that its settings will not be visible until they are saved.
So this is the additional argument for making the separate CONFIG_APPLY event since, as the above example shows, the "isTemporary" check would not help to distinguish between the case of a direct "real" configuration modifications and the case when the settings get really applied to the entire system.

Mikhail
Comment 16 Leo Treggiari CLA 2007-09-05 12:46:11 EDT
(In reply to comment #15)

...

> As you say, the APPLY notification is made from the
> ManagedBuildManager.setOptoin(), which in the project properties UI case gets
> called when the APPLY button is pressed (and option values get coppied) and
> when option UI control gets modified resulting in setOption() for the
> underlying temporary configuration.

...

> The reason the APPLY event in not fired when the OK is pressed is because in
> case of an OK the temporary configuration simply becomes a regular one
> replacing the previous non-temporary configuration. The two reasons why this is
> done in this way rather than copying all settings from one configuration to
> another is performance and settings consistency. Having the configuration
> simply applied ensures that the configuration settings getting stored are
> exactly the same as displayed/modified in UI (or programmatically). Otherwise
> we would encounter problems with setting modification ordering, e.g. in case
> the configuration tool-chain was modified (e.g. replaced with another one), the
> "copy settings" mechanism would need to perform the "copy" tool-chain
> modification before, e.g. copying option values.
> So in case of an OK, the "EVENT_APPLY" is called for the "temporary"
> configuration when the setOption is called for its options as a result of the
> UI option changes, and when the OK button is pressed, that configuration simply
> gets stored with no additional setOption() calls.

As far as I'm concerned, pressing the OK button should be equivalent to pressing Apply on all of the sub-pages and then closing the dialog box - both from the user's and programmer's perspectives.  Making these different for the programmer simply adds to the complexity of using the functionality.

> > What I want is a callback to be made when the value of the real configuration's
> > option has changed.  I don't think that I care whether the change was made from
> > the UI or otherwise.
> > 
> > The "special case" is the handling of option value changes while the property
> > page UI is active.  The design was to make handling these value changes as
> > similar as possible to "real" configuration changes.  As Oleg pointed out in a
> > prevous reply, we want the UI to reflect what the state of the configuration
> > will be if OK or Apply gets selected, including any side-effects of value
> > changes such as enabling/disabling other options.
> That's how it works now. And as mentioned above, there is nothing specific
> about using the option modification mechanism by UI. Handlers do get notified
> with the APPLY event each time the setOption() is called (no matter whether the
> change is done via UI or programmatically) and on settings apply, the temporary
> configuration simply becomes a "reguilar"(non-temporary) one getting stored in
> the ManagedBuildInfo.
> As I mentioned above this works OK in case the changes are made within the
> configuration concept (the way the handler call-backs are expected to behave),

Expected by this particular implementation?  It was not "expected" prior to CDT 4.0.

> but in Doug's case the handler intends to modify the project global settings
> outside of the configuration scope, which is not intended to be supported.

Data outside of the implementation's view of the configuration scope does not need to be "project global settings".  Other parts of CDT, or ISVs, may want to logically extend the concept of a configuration by storing per-configuration data that is not handled by the current configuration implementation.  Does this current design make that impossible?

> > If there are 2 separate events, one needs to signify that the value of an
> > option in a "real" configuration has changed (no matter how), and the other can
> > be specific to "not yet applied" UI changes.
> I don't agree with such a discretion completely, because changing settings of
> the "real" configuration directly does not make those settings to be applied to
> the entire system. Instead they would be seen by the build system only until
> the saveBuildInfo() is called. So, e.g. this approach would not work in Doug's
> case because what I guess he needs is to modify system's settings while the
> settings get applied. In this sense the "real" configuration will work in a
> similar way as the "temporary" one in the sense that its settings will not be
> visible until they are saved.
> So this is the additional argument for making the separate CONFIG_APPLY event
> since, as the above example shows, the "isTemporary" check would not help to
> distinguish between the case of a direct "real" configuration modifications and
> the case when the settings get really applied to the entire system.

You'll have to specify exactly when the CONFIG_APPLY event would be fired and for which options (only those that have changed?).  If it is fired for all options, then the "performance advantage" of the current desing could easily be lost.

Leo

Comment 17 Mikhail Sennikovsky CLA 2007-09-05 13:51:11 EDT
(In reply to comment #16)
Hi Leo,

> As far as I'm concerned, pressing the OK button should be equivalent to
> pressing Apply on all of the sub-pages and then closing the dialog box - both
> from the user's and programmer's perspectives.  Making these different for the
> programmer simply adds to the complexity of using the functionality.
There is no difference between OK and APPLY from the API perspecfive, i.e. in case of an OK the option values get set by the user in UI, while in case of APPLY those values get set programmatically by the APPLY code in exactly the same fashion. I agree that we have some difference in terms of how/when an ISV is seeing the APPLY and OK, but, as I mentioned above, having OK implemented in the same way as APPLY would result in a performance overhead as well as in the necessity to specify the page APPLY order due to the issues mentioned earlier.

> Expected by this particular implementation?  It was not "expected" prior to CDT 4.0.
I believe it was ;-) The "clone" concept did not change since it first was presented. The value handler always (in the 3.x as well) had to be aware that it might be called both from the "clone" as well as from the "real". As a result, e.g. it was inappropriate to access e.g. the ManagedBuildInfo since this might result in a permanent storing of temporary settings.
We have discussed exactly the same restriction when discussing the "clone" proposal for the value handler mechanism (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=106036#c19 )

Here is a snippet from that conversation:
==snippet start==
>If so, then it is possible, but probably not 
> wise, for other parts of the project to be affected by code invoked from the 
> clone.
I agree that the other parts of the project could be affected from the
tool-integrator call-backs, but I don’t think we have an issue here, more
precisely we don’t have any new issue, since no matter whether we pass a clone,
a “real” configuration or something else, a tool integrator will be always able
to affect other parts of the project (or even workspace) than those passed to
the callback, e.g. currently the IOptionApplicability.isOptionEnabled method is
passed a configuration, an option holder and an option. From this info (namely
from the configuration) a managed project and all its configurations could be
obtained and modified. I suppose that modifying some parts of the project other
than those passed to the callback should be considered as buggy and incorrect
behavior.
==snippet end==

> Data outside of the implementation's view of the configuration scope does not
> need to be "project global settings".  Other parts of CDT, or ISVs, may want to
> logically extend the concept of a configuration by storing per-configuration
> data that is not handled by the current configuration implementation.  Does
> this current design make that impossible?
An ISV can associate any data with the configuration (ICConfigurationDescription). The primary mechanism of doing this is per-configuration session properties that allow maintaining the per-configuration session data and the "storage module" mechanism that allows per-configuration data persistence. So it is possible to maintain any custom data within the configuration. On the other hand in case an ISV wants to modify some data outside of the configuration context, e.g. "project global settings" or the settings of some other configuration, he/she would have to solve the same problem as he would have to in the 3.x: the call-backs could be called from the temporary configs, thus if some config-external data is modified, changes made to that data will still be there after cancel.
So in case the handler is operating within the given configuration (ICConfigurationDescription) context (which still seems to be the expected behavior to me :), the handler should have no problem with the "difference" between the OK and APPLY mechanisms discussed above.

> You'll have to specify exactly when the CONFIG_APPLY event would be fired and
> for which options (only those that have changed?).  If it is fired for all
> options, then the "performance advantage" of the current desing could easily be
> lost.
We could consider making the "CONFIG_APPLY" to be fired for modified options only, however I guess the value handler might be interested in always receiving the notification, independently of whether the option value was changed or not. More than that the 3.x OK/APPLY mechanism notified performed setOption() for all existing options, no matter whether the option value was changed or not thus making all existing handlers notified.

Mikhail