Bugzilla – Full Text Bug Listing
| Summary: | [variables] Allow for editable value different from the current formatted value. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | David Dubrow <david.dubrow> | ||||||
| Component: | cdt-debug-dsf | Assignee: | David Dubrow <david.dubrow> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Pawel Piech <pawel.1.piech> | ||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | cdtdoug, david.dubrow, ed.swartz, ken.ryall, pawel.1.piech | ||||||
| Version: | 7.0 | Flags: | pawel.1.piech:
review+
|
||||||
| Target Milestone: | 7.0 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
David Dubrow
I like this idea. Would it be enough to simply create a new format ID (.e.g. EDITABLE), and the UI would use this format, if available, to populate the editor? I don't think we have to rush it for M6 because of the API freeze. We can still make API changes beyond M6 we just need to go through a little more process. (In reply to comment #1) > I like this idea. Would it be enough to simply create a new format ID (.e.g. > EDITABLE), and the UI would use this format, if available, to populate the > editor? It seems that this is just a little less flexible than a first class API because it wouldn't allow for different editable values that respected the current format ID. > I don't think we have to rush it for M6 because of the API freeze. We can > still make API changes beyond M6 we just need to go through a little more > process. Ok. I just wanted to make sure we got it in time for Helios. :) (In reply to comment #2) > It seems that this is just a little less flexible than a first class API > because it wouldn't allow for different editable values that respected the > current format ID. Good point. It seems that I missed something though, did you propose a new API already? (In reply to comment #3) > (In reply to comment #2) > > It seems that this is just a little less flexible than a first class API > > because it wouldn't allow for different editable values that respected the > > current format ID. > > Good point. It seems that I missed something though, did you propose a new API > already? I proposed adding a new method to IFormattedValues.FormattedValueDMData getEditableValue() (In reply to comment #4) > I proposed adding a new method to IFormattedValues.FormattedValueDMData > > getEditableValue() Ah, thank you that wasn't obvious to me. In that case go for it ;-) Committed to HEAD Doug, I just committed this change this morning. Is it in time for M6? David, thank you for the fix. For future reference please attach the fix patch to the bug and request a review from one of the other committers as per our bug policy (http://wiki.eclipse.org/CDT/Bugs#Reviewing). For this specific fix, I have a couple of requests: 1) Add an API backward compatible constructor for GetFormattedValueValueQuery. 2) For consistency, remove FormattedValueDMData.setEditableValue() and add a constructor which takes it as a parameter instead. 3) Make SyncVariableDataAccess.getValue() private since its redundant. Created attachment 162070 [details] additional changes (In reply to comment #8) > David, thank you for the fix. For future reference please attach the fix patch > to the bug and request a review from one of the other committers as per our bug > policy (http://wiki.eclipse.org/CDT/Bugs#Reviewing). I was going to post a patch to this bugzilla, and then I thought I would miss the M6 deadline (which I think I may have missed anyway). Not trying to excuse myself, but I kept trying to work on this and was getting pulled away with other tasks. Sorry! :( > > For this specific fix, I have a couple of requests: > > 1) Add an API backward compatible constructor for GetFormattedValueValueQuery. Ah, yes. It is also API. > > 2) For consistency, remove FormattedValueDMData.setEditableValue() and add a > constructor which takes it as a parameter instead. I guess I was trying to allow for the editable value to be set after the object was created. I guess I can add the new constructor and leave the setter? > > 3) Make SyncVariableDataAccess.getValue() private since its redundant. No problem. (In reply to comment #9) > I was going to post a patch to this bugzilla, and then I thought I would miss > the M6 deadline (which I think I may have missed anyway). Not trying to excuse > myself, but I kept trying to work on this and was getting pulled away with > other tasks. Sorry! :( No problem, I just wanted to make sure you're aware of the process. > I guess I was trying to allow for the editable value to be set after the object > was created. I guess I can add the new constructor and leave the setter? The down side of this is that it open the object to possible race conditions. The DM data object is meant to be thread safe, so if you make it so it's not immutable, you'll also need to add synchronization. Created attachment 162089 [details] new patch (In reply to comment #10) > The down side of this is that it open the object to possible race conditions. > The DM data object is meant to be thread safe, so if you make it so it's not > immutable, you'll also need to add synchronization. Ok. I;ll take out the setter. No sense adding more complexity to this, especially that I don't actually have a use-case that requires the setter. ;) Let me know if I can commit this. I think I've addressed all your concerns. :) (In reply to comment #11) > Let me know if I can commit this. I think I've addressed all your concerns. :) Looks good to me. Thanks! |