Community
Participate
Working Groups
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?
Created attachment 183761 [details] changes aiming for compatibility, perhaps pointlessly
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 :)
(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.
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.
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.
Committed to HEAD.
*** cdt cvs genie on behalf of eswartz *** Bug 330961: revise and fix bugs in ISettings interface [*] AgentSettings.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/launch/AgentSettings.java?root=Tools_Project&r1=1.1&r2=1.2 [*] ISettings.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc.tcf.extension/src/org/eclipse/cdt/debug/edc/tcf/extension/services/ISettings.java?root=Tools_Project&r1=1.1&r2=1.2 [*] SettingsProxy.java 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc.tcf.extension/src/org/eclipse/cdt/debug/edc/tcf/extension/services/SettingsProxy.java?root=Tools_Project&r1=1.6&r2=1.7
