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

Bug 330961

Summary: Settings service and co. needs revamp
Product: [Tools] CDT Reporter: Ed Swartz <ed.swartz>
Component: cdt-debug-edcAssignee: Ed Swartz <ed.swartz>
Status: RESOLVED FIXED QA Contact: Ken Ryall <ken.ryall>
Severity: normal    
Priority: P3 CC: cdtdoug, john.cortell, ling.5.wang
Version: 8.0   
Target Milestone: ---   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
changes aiming for compatibility, perhaps pointlessly
none
cleaned-up and updated patch cdtdoug: iplog-

Description Ed Swartz CLA 2010-11-23 14:10:07 EST 
org.eclipse.cdt.debug.edc.tcf.extension.services.ISettings seems to have been incompletely designed and this has been masking stability issues in our debugger.  

-- The "setValues" method has no provision for detecting completion or reporting errors (i.e. no "Done..." callback), which is a pretty fatal problem.  (This is the point where our target-side TCF agent actually initializes itself; this operation can fail and we must sync on the command and catch errors before sending more commands.)

-- Similarly, the AgentSettings wrapper for ISettings for some reason uses the name "sendSettingsToAgent" to ... *get* settings from the agent... and print them out.  Luckily we don't make use of this yet.

-- Less objectionable: the name "ISettings#getSupportedSettings" and its "DoneGetSettingsValues" interface are not named alike.


Obviously, fixing these will involve API changes since both ISettings and AgentSettings are API.  I could either fix them with affordances for backward compatibility (@deprecated tags, etc.) or just take the hit and fix them in a more thorough (breaking) way.  

Is anyone outside Nokia using these classes currently who would have an opinion?
Comment 1 Ed Swartz CLA 2010-11-24 09:36:07 EST 
Created attachment 183761 [details]
changes aiming for compatibility, perhaps pointlessly
Comment 2 Ed Swartz CLA 2010-12-03 11:10:21 EST 
Does anyone have any opinions on this?  John, are you using the settings service in your EDC project?  If not, it's an easy call :)
Comment 3 John Cortell CLA 2010-12-03 11:32:52 EST 
(In reply to comment #2)
> Does anyone have any opinions on this?  John, are you using the settings
> service in your EDC project?  If not, it's an easy call :)

Not yet. Feel free to overhaul as far as I'm concerned.
Comment 4 Ling Wang CLA 2010-12-03 12:16:39 EST 
Right, the service does need revamp. Agree with all the changes, but just @deprecate instead of remove the old "setValues()" so that stop mode debugger won't break, and I'll adapt to any new API later.
Comment 5 Ed Swartz CLA 2010-12-03 16:03:18 EST 
Created attachment 184515 [details]
cleaned-up and updated patch

Oops, I think the getter was renamed incorrectly.  Although no one implements or uses it yet, my renaming of "getSupportedSettings" to "getValues" doesn't jive with the API (which returns a list of setting ids).  

So this will be "#getIds()" now.

Also, the TCF-level "get" and "set" command names don't correspond, which is also confusing.  I propose renaming "get" to "getIds" too.  This allows, in the future, for an actual "getValues" command and API in case we want to retrieve values for settings.  

But the "set" command name will remain the same, both for compatibility with our on-device agents, and because there's only one way "set" would make sense in this service.

----

Ling contends that compatibility and deprecation annotations aren't necessary since we're the only ones who've implemented this, so this patch reflects that.
Comment 6 Ed Swartz CLA 2010-12-15 21:28:25 EST 
Committed to HEAD.