Community
Participate
Working Groups
The label of a thread is the same as the container, instead of being "Thread [n]". Probably related to the PropertyProvider changes. I'll have a look, I expect it is simple (famous last words :-))
Created attachment 129314 [details] Fix There was in fact a bug in the TreadVMNode which was getting the data of the process instead of the thread. Then, the AbstractThreadVNNode LabelProvider did not yield the same format as what we used to have; it was missing the possibility to have both a threadId and an OSThreadId. This patch fixes both issues and shows GDB Threads as before, i.e., Thread[<threadId>] <OSId> (Suspended : <Reason>) I committed the change. However, the LabelProvider format is part of DSF, so Pawel, let me know if you agree or not, and feel free to make or ask me to make changes.
Pawel, can you review?
This fix technically changes the API by adding two new plublic constants. Also, I changed the format of messages.properties, which could technically break someone's previous usage of it. I saw we still go ahead with it. Do we need votes?
Created attachment 129366 [details] Updated checkProperty() I forgot to update ExecutionContextLabelText#checkProperty() for the new ID2 property. Committed.
I was actually going to re-work the patch so that it's GDB specific, since the view model is explicitly designed to be extendable and multiple id's per thread seem redundant for a general case. I'll have a patch ready soon.
Created attachment 129379 [details] GDB-specific patch. This patch adds the second ID to the GDB threads node only. It also adds a "state change reason known" property which simplifies the label provider logic somewhat. I didn't commit the patch yet, because I want Marc to have a chance to review it first.
BTW, this is technically an API change, even though it's just provisional API. But we're past API freeze so we should ask for review prior to committing.
(In reply to comment #6) > This patch adds the second ID to the GDB threads node only. Thanks! > It also adds a > "state change reason known" property which simplifies the label provider logic > somewhat. That is much nicer. > I didn't commit the patch yet, because I want Marc to have a chance to review > it first. Two comments: 1- In GdbExecutionContextLabelText#getPropertyValue(), don't you have to also check and return the value for IGdbLaunchVMConstants.PROP_OS_ID 2- StackFramesVMNode_No_columns__No_function__text_format in messages.properties had been changed by Ken, but you changed it back. I assume it was an accident, and that Ken was right. The value of that message should be [{1}] at {0} instead of [{2}] at {0} I'll let you fix that in your patch. Besides those two point, everything looks good to me. Go ahead and commit.
(In reply to comment #7) > BTW, this is technically an API change, even though it's just provisional API. > But we're past API freeze so we should ask for review prior to committing. +1 from me.
I just noticed: !ENTRY org.eclipse.osgi 2 1 2009-03-19 14:33:01.655 !MESSAGE NLS missing message: AbstractThreadVMNode_No_columns__No_reason__text_format in: org.eclipse.cdt.dsf.debug.ui.viewmodel.launch.messages !ENTRY org.eclipse.osgi 2 1 2009-03-19 14:33:02.045 !MESSAGE NLS missing message: ThreadVMNode_No_columns__No_reason__text_format in: org.eclipse.cdt.dsf.gdb.internal.ui.viewmodel.launch.messages You should remove ThreadVMNode_No_columns__No_reason__text_format from MessagesForGdbLaunchVM and AbstractThreadVMNode_No_columns__No_reason__text_format from MessagesForLaunchVM
(In reply to comment #8) > 1- In GdbExecutionContextLabelText#getPropertyValue(), don't you have to also > check and return the value for IGdbLaunchVMConstants.PROP_OS_ID No, super.getPropertyValue() will return null, if the property is not available which is fine since the message format checks for it. > 2- StackFramesVMNode_No_columns__No_function__text_format in > messages.properties had been changed by Ken, but you changed it back. I assume > it was an accident, and that Ken was right. The value of that message should > be > [{1}] at {0} > instead of > [{2}] at {0} > I'll let you fix that in your patch. I missed that, thanks. > Besides those two point, everything looks good to me. > Go ahead and commit. I applied the patch. (In reply to comment #10) Fixed, thanks.
