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

Bug 375131

Summary: [launch] Cancelling an attach or postmortem launch no longer cancels
Product: [Tools] CDT Reporter: Marc Khouzam <marc.khouzam>
Component: cdt-debug-dsf-gdbAssignee: Marc Khouzam <marc.khouzam>
Status: RESOLVED FIXED QA Contact: Marc Khouzam <marc.khouzam>
Severity: normal    
Priority: P3 CC: cdtdoug, nobody, pawel.1.piech
Version: 8.1.0Flags: nobody: review+
Target Milestone: 8.1.0   
Hardware: PC   
OS: Linux   
Whiteboard:
Attachments:
Description Flags
Fix to cancel the new RM marc.khouzam: iplog-

Description Marc Khouzam CLA 2012-03-22 22:03:43 EDT 
If you launch a local attach session and press cancel in the prompt that pops up, the launch is no longer cancelled.

The reason is that the change to bug 373845 required to create a new requestMonitor, but we forgot to handle the cancel case.
Comment 1 Marc Khouzam CLA 2012-03-22 22:08:04 EDT 
Created attachment 213086 [details]
Fix to cancel the new RM

This patch simply cancels the new RM we are using, when the original one was cancelled.

This is something we should do all the time when we use a new RM.  I personally am not used to it because we didn't used to deal with cancellations, but now that we do, it should be dealt with when we write a handleCompleted() method.
Comment 2 Marc Khouzam CLA 2012-03-22 22:09:17 EDT 
Committed to master.

I also re-tested the cases of bug 373845 and things still work as expected.

Mikhail, can you review?
Comment 3 Nobody - feel free to take it CLA 2012-03-22 22:13:01 EDT 
Looks good. Should we add JUnit tests for cancelling?
Comment 4 Marc Khouzam CLA 2012-03-23 09:00:37 EDT 
(In reply to comment #3)
> Looks good. Should we add JUnit tests for cancelling?

Absolutely.  But I don't know how.  These cancellations are triggered by cancelling a Dialog and we don't have JUnit tests that handle the UI.  This is actually something that is a big lack in our JUnit coverage: there is a bunch of logic that we don't test because it is triggered from the UI.

I've noticed that some other CDT JUnit tests trigger the UI.  I haven't had time to investigate, but I would really like for DSF to have such tests.
Comment 5 Nobody - feel free to take it CLA 2012-03-23 10:56:04 EDT 
Marc, did you check in this patch? I don't see the changes.
Comment 6 Marc Khouzam CLA 2012-03-23 11:26:58 EDT 
(In reply to comment #5)
> Marc, did you check in this patch? I don't see the changes.

Yes, I committed it.  I fetched it back and I also see it from the web interface at: 
http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=ccf74d51d0917607433997778b66dec882c14f79
Comment 7 Nobody - feel free to take it CLA 2012-03-23 11:35:56 EDT 
(In reply to comment #6)
> (In reply to comment #5)
> > Marc, did you check in this patch? I don't see the changes.
> 
> Yes, I committed it.  I fetched it back and I also see it from the web
> interface at: 
> http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=ccf74d51d0917607433997778b66dec882c14f79

The last patch I see is Anna's patch. But it seems that I did something wrong, can you please check this change:
http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=9e7c69f28952f708bfc1997c318043eee112a4c3
It appeared when I committed the patch for the test application name. I hope I didn't break anything.
Comment 8 Marc Khouzam CLA 2012-03-23 12:44:19 EDT 
(In reply to comment #7)

> The last patch I see is Anna's patch. But it seems that I did something wrong,
> can you please check this change:
> http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=9e7c69f28952f708bfc1997c318043eee112a4c3
> It appeared when I committed the patch for the test application name. I hope I
> didn't break anything.

As confirmed on the mailing list, nothing got broken.

But are you able to fetch the latest commits?
Comment 9 Nobody - feel free to take it CLA 2012-03-23 12:48:20 EDT 
(In reply to comment #8)
> (In reply to comment #7)
> 
> > The last patch I see is Anna's patch. But it seems that I did something wrong,
> > can you please check this change:
> > http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=9e7c69f28952f708bfc1997c318043eee112a4c3
> > It appeared when I committed the patch for the test application name. I hope I
> > didn't break anything.
> 
> As confirmed on the mailing list, nothing got broken.
> 
> But are you able to fetch the latest commits?

Yes, after I made the changes suggested by Pawel and Andrew.
Comment 10 Nobody - feel free to take it CLA 2012-03-23 13:46:02 EDT 
(In reply to comment #4)
> (In reply to comment #3)
> > Looks good. Should we add JUnit tests for cancelling?
> 
> Absolutely.  But I don't know how.  These cancellations are triggered by
> cancelling a Dialog and we don't have JUnit tests that handle the UI.  This is
> actually something that is a big lack in our JUnit coverage: there is a bunch
> of logic that we don't test because it is triggered from the UI.
> 
> I've noticed that some other CDT JUnit tests trigger the UI.  I haven't had
> time to investigate, but I would really like for DSF to have such tests.

I created https://bugs.eclipse.org/bugs/show_bug.cgi?id=375203 to track this proposal.
Comment 11 CDT Genie CLA 2012-03-23 14:59:11 EDT 
*** cdt git genie on behalf of Marc Khouzam ***

    Bug 375131: [launch] Cancelling an attach or postmortem launch no longer cancels

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=ccf74d51d0917607433997778b66dec882c14f79