Bug 245918 - [dstore] Customization of connection timeout does not work
Summary: [dstore] Customization of connection timeout does not work
Status: VERIFIED FIXED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 3.0   Edit
Hardware: PC Windows XP
: P3 major (vote)
Target Milestone: 3.0.1   Edit
Assignee: David McKnight CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-01 22:53 EDT by Masao Nishimoto CLA
Modified: 2008-09-30 00:31 EDT (History)
2 users (show)

See Also:
mober.at+eclipse: pmc_approved+
kjdoyle: review+
dmcknigh: review+
xuanchen: review+


Attachments
here's a patch for this (4.12 KB, patch)
2008-09-18 10:58 EDT, David McKnight CLA
no flags Details | Diff
updated patch with hack to trigger intializer (4.88 KB, patch)
2008-09-18 11:28 EDT, David McKnight CLA
no flags Details | Diff
Patch fixing the problem (7.62 KB, patch)
2008-09-18 13:13 EDT, Martin Oberhuber CLA
no flags Details | Diff
Improved patch (7.14 KB, patch)
2008-09-18 13:31 EDT, Martin Oberhuber CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Masao Nishimoto CLA 2008-09-01 22:53:27 EDT
To increase the default time out value, I put the following line in plugin_customization.ini file.

org.eclipse.rse.ui/org.eclipse.rse.connectorservice.dstore.ui.preferences.sockettimeout=30000

The setting does not work, because Activator.initializeDefaultPreferences() overrides the value.

getInt() and getDefaultInt() for the preference returns 30000 before the method, and 5000 after the method.
Comment 1 Martin Oberhuber CLA 2008-09-02 09:00:05 EDT
Are you absolutely sure that this is an issue?

My experience (with the SHOW_EMPTY_LISTS Preference) is that the plugin_customization.ini method works properly even if store.setDefault() is executed. 

I'm not exactly sure why this is the case (maybe because SHOW_EMPTY_LISTS is not initialized from the Activator.start() method but from SystemPreferenceInitializer.initializeDefaultPreferences() which implements the org.eclipse.core.runtime.preferences extension point).

Perhaps that extension is executed *before* the plugin_customization.ini Preferences are applied such that plugin_customization.ini can override the hardcoded default-defaults.
Comment 2 Masao Nishimoto CLA 2008-09-03 01:31:22 EDT
I also have the following line in the plugin_customization.ini, and it works.

org.eclipse.rse.ui/org.eclipse.rse.preferences.shownewconnection=true

I confirmed that the setting in the plugin_customization.ini is in effect, not when SystemPreferenceInitializer.initializeDefaultPreferences() is executed, but when Activator.initializeDefaultPreferences() is executed.
Comment 3 Martin Oberhuber CLA 2008-09-18 09:56:58 EDT
See also bug 245714
Comment 4 David McKnight CLA 2008-09-18 10:58:13 EDT
Created attachment 112903 [details]
here's a patch for this 

I've tried this but it doesn't seem to work for me.  Martin, am I missing something in this patch?
Comment 5 Martin Oberhuber CLA 2008-09-18 11:04:04 EDT
This is a major issue since I cannot think of a workaround. Being able to customize products is very important.
Comment 6 David McKnight CLA 2008-09-18 11:28:19 EDT
Created attachment 112910 [details]
updated patch with hack to trigger intializer
Comment 7 David McKnight CLA 2008-09-18 12:00:49 EDT
Comment on attachment 112910 [details]
updated patch with hack to trigger intializer

this won't work with plugin_customization.ini because we're still using the RSE UI preferences
Comment 8 Martin Oberhuber CLA 2008-09-18 13:13:21 EDT
Created attachment 112920 [details]
Patch fixing the problem

The proposed patch does not fix the problem: The core.preferences extension can not be used here because the Preferences in question are actually stored in a different plugin. As a result, the following can happen:

  1. RSEUIPlugin Preferences get accessed
  2. plugin_customization Preferences are applied to RSEUIPlugin
  3. Connectorservice.dstore gets loaded
  4. Coded Defaults from there override the customized defaults

The correct order of initialization can be seen in class DefaultPreferences:
 private void loadDefaults() {
  applyRuntimeDefaults();
  applyBundleDefaults();
  applyProductDefaults();
  applyCommandLineDefaults();
 }

Since our code here is potentially loaded *after* defaults have been applied to RSEUIPlugin already, we must take care not to override any defaults that have been set already. The problem here is, that the IPreferenceStore API does not provide a method for checkign whether a default has been set already; the newer IPreferencesService = Platform.getPreferencesService() API must be used here.

I tested the patch and it works fine for me, and I propose re-building 3.0.1 to include this patch. I consider it low-risk since the normal path of operation is not changed in the normal case (default preference==null).
Comment 9 Martin Oberhuber CLA 2008-09-18 13:14:34 EDT
I'm approving my own patch for 3.0.1 rebuild. Dave and Kevin, what do you think?
Comment 10 Martin Oberhuber CLA 2008-09-18 13:16:17 EDT
Patch committed for 3.1M2 -- Xuan what do you think about releasing this into a 3.0.1 rebuild?
Comment 11 Martin Oberhuber CLA 2008-09-18 13:31:42 EDT
Created attachment 112928 [details]
Improved patch

Here is actually a better patch, that's less code and works without extending ScopedPreferenceStore. ScopedPreferenceStore is not marked @noextend, but extending an API class still bears some risk: In case a "hasDefault()" method would be added to ScopedPreferenceStore in the future, our code might break.

It's therefore safer to just call existing API methods rather than extending the class. Other than that, functionality is the same as the other patch (tested to work).
Comment 12 Xuan Chen CLA 2008-09-18 14:44:28 EDT
I am fine with putting this fix into 3.0.1.  Thanks.
Comment 13 Martin Oberhuber CLA 2008-09-18 17:17:35 EDT
The fix has been rolled into a TM 3.0.1 Rebuild:

http://download.eclipse.org/dsdp/tm/downloads/drops/R-3.0.1-200809181400/
http://download.eclipse.org/dsdp/tm/updates/3.0.1milestones/

Please test and verify.
Comment 14 Masao Nishimoto CLA 2008-09-30 00:31:39 EDT
Verified