Community
Participate
Working Groups
There have been a couple improvements to AbstractCLaunchDelegate that GdbLaunchDelegate won't pick up for free: bug 178731 and bug 285904. I couldn't recall why GdbLaunchDelegate doesn't use AbstractCLaunchDelegate. Should we do whatever work is required to make AbstractCLaunchDelegate sharable with GdbLaunchDelegate?
I also think it's a good idea. There was a lot of confusion early on about dependencies between various launch components, so the functionality was likely duplicated needlessly.
Just ran into another related problem (bug 285907). I'll start working on better sharing of launch code.
I started working on this and then realized that GdbLaunchDelegate is public and API. I'll look at it again when we get to work on CDT 7.0.
(In reply to comment #3) > I started working on this and then realized that GdbLaunchDelegate is public > and API. I'll look at it again when we get to work on CDT 7.0. What changes would break the API? Making GdbLaunchDelegate extend AbstractCLaunchDelegate doesn't break the API, right? Are there methods you need to remove to make this change worthwhile?
I've been looking at this in more detail and I'm having second thoughts. AbstractCLaunchDelegate has some useful common things but also some things that are not CDI specific but don't add any value to a DSF launch. It should probably be broken up into AbstractCLaunchDelegate and a AbstractCDILaunchDelegate. But given our attempts to deprecate CDI this may not be worth the effort. Perhaps we should instead create an AbstractDSFLaunchDelegate that DSF based debuggers can share and make sure it supports all the stuff in AbstractCLaunchDelegate (mostly the build before run settings).
(In reply to comment #5) > I've been looking at this in more detail and I'm having second thoughts. > AbstractCLaunchDelegate has some useful common things but also some things that > are not CDI specific but don't add any value to a DSF launch. It should > probably be broken up into AbstractCLaunchDelegate and a > AbstractCDILaunchDelegate. > > But given our attempts to deprecate CDI this may not be worth the effort. > Perhaps we should instead create an AbstractDSFLaunchDelegate that DSF based > debuggers can share and make sure it supports all the stuff in > AbstractCLaunchDelegate (mostly the build before run settings). Although it won't help getting new feature of AbstractCLaunchDelegate for free, it would help getting features for free between DSF-based debuggers, so +1 from me.
Never mind, I thought about it over a burrito and now it looks easier than I thought thought to pull the CDI stuff out of AbstractCLaunchDelegate. I'll experiment with that.
OK, I've come full circle: I've created an AbstractCLaunchDelegate2 that combines the common stuff from GdbLaunchDelegate and the new build integration support from AbstractCLaunchDelegate. AbstractCLaunchDelegate is left alone because it has loads of legacy stuff in it that may not make sense or be required any longer but could be relied upon by people building on CDT. Now GdbLaunchDelegate and the EDC launch delegates extend AbstractCLaunchDelegate2 and pick up the new build integration features. These require the new UI in org.eclipse.cdt.launch.ui.CMainTab so the DSF Gdb launch now uses it instead org.eclipse.cdt.dsf.gdb.internal.ui.launching.CMainTab. They are identical except for the new work in org.eclipse.cdt.launch.ui.CMainTab. org.eclipse.cdt.launch.ui.CMainTab has an option for "Connect process input & output to a terminal" that is not in org.eclipse.cdt.dsf.gdb.internal.ui.launching.CMainTab. Does anyone know if this is by design? If so we can hide it in the DSF-GDB launch. My next move is to change the CDI launches to use AbstractCLaunchDelegate2 and see if I have left anything out.
(In reply to comment #8) > org.eclipse.cdt.launch.ui.CMainTab has an option for "Connect process input & > output to a terminal" that is not in > org.eclipse.cdt.dsf.gdb.internal.ui.launching.CMainTab. Does anyone know if > this is by design? If so we can hide it in the DSF-GDB launch. Yep, that was by design. CDI allows the user not to connect the process i/o to a terminal because it fails when using the Restart operation. For DSF-GDB, the Restart action does not break in this case, so we always attach the process i/o to a terminal.
Created attachment 160476 [details] launch delegate refactoring Here is the patch.
Marc, how does this look?
(In reply to comment #11) > Marc, how does this look? Thanks Ken. I'm going over the patch now, but here are some comments about sharing the CMainTab. I think it is a great idea, (DSF-GDB having it's own copy only made us lose features, but this was done when we were not part of CDT). I had made some modifications to the tab for DSF-GDB so we'll need to make the common one fit properly for both DSF-GDB and CDI-gDB. Below is what is missing: - I see that for CDI, the attach launch config type and the core launch config type have the new "Build before launching" UI. I guess it should also be there for those launch config types for DSF-GDB; so dsf.gdb.internal.ui.launching.CMainCoreTAb should be replaced with the one in cdt.launch.ui, and the DSF-GDB one removed; as for dsf.gdb.internal.ui.launching.CMainAttachTab it should be replaced by the one in cdt.launch.ui, but it will need modification to allow to turn off 'checking for program' - There is also a DSF-GDB remote launch that is one of the choices of the Local App launch config type. Should it use the new launch.ui.CMainTab? It still uses the DSF-GDB one in the plugin.xml file. - To get rid of the "Connect process i/o to a terminal" option for DSF-GDB we will need a new class that will inherit from CMainTab but that will call CMainTab(false). You could re-use the filename of gdb.ui.CMainTab if no one else is using it anymore. - In the DSF-GDB main tab we had reversed the order of the input boxes 'binary' and 'project' to make 'project' the one on top. I found it weird that CDI would ask for the 'binary' first but if you clicked on Search Project it would complain that you haven't selected a project yet. But I'm not set on it as I might be missing something. - DSF-GDB supports debugging binaries that are not in an Eclipse project. Therefore, the fBrowseForBinaryButton selectionListener was modified not to require the user to enter a project.
OK, that explains a lot, I'll start working on those changes.
Looking at this again maybe the right thing to do is to have the DSF CMainTab extend the CDT Launch CMainTab. I'll experiment with that.
(In reply to comment #14) > Looking at this again maybe the right thing to do is to have the DSF CMainTab > extend the CDT Launch CMainTab. I'll experiment with that. That sounds like a good idea. We'll have more flexibility in DSF, while not risking breaking the CDI stuff.
One thing to pay attention to: in the plugin.xml, the tabs have a 'placement' field which tell them where to be placed. The id of that field should be the TAB_ID defined _in the class_ and returned by getId(). Currently, it is wrong in dsf.gdb.ui/plugin.xml because the CMainTab used has changed. Depending on what you decide, Ken, just make sure the placement field is properly updated.
The posted patch "launch delegate refactoring" looks good to me. The one problem that I see is that we no longer accept an empty project/binary as required by Bug 244567. But I see that Ken is currently in the process of adding that in. One suggestion is that GdbLaunchDelegate should use verifyCProject() of the new AbstractCLaunchDelegate2 inside of checkBinaryDetails(), instead of the LaunchUtils one.
Created attachment 161396 [details] main tab This patch takes most of the common bits of the two CMainTabs and puts them into a abstract base class. The goal was to leave the CDI CMainTab as unaltered as possible and bring the new build settings into the DSF CMainTab.
(In reply to comment #17) > One suggestion is that GdbLaunchDelegate should use verifyCProject() of the new > AbstractCLaunchDelegate2 inside of checkBinaryDetails(), instead of the > LaunchUtils one. Committed this change as well as the "main tab" patch. There is probably a little more that could move into CAbstractMainTab but I think this will do for now.
(In reply to comment #19) > (In reply to comment #17) > > One suggestion is that GdbLaunchDelegate should use verifyCProject() of the new > > AbstractCLaunchDelegate2 inside of checkBinaryDetails(), instead of the > > LaunchUtils one. > > Committed this change as well as the "main tab" patch. There is probably a > little more that could move into CAbstractMainTab but I think this will do for > now. This looks great! I think it was very important to show a very similar launch between CDI-GDB and DSF-GDB. This was a great bug to have done. One last thing that I think is needed is that the DSF-GDB CMainAttachTab and CMainCoreTab classes should specify the new flag INCLUDE_BUILD_SETTINGS. CDI-GDB shows those settings for both Attach and Core launches. Finally, from the user's perspective, should we have the same order of Project and Binary in the main tab between CDI and DSF? As I said, I felt the order for CDI should have been reversed, but having the two tabs so subtly different may be confusing to the user. Maybe an email to the list is in order? Thanks Ken!
Created attachment 161894 [details] Small fixes for DSF-GDB (In reply to comment #20) This small patch addresses the below issues. > One last thing that I think is needed is that the DSF-GDB CMainAttachTab and > CMainCoreTab classes should specify the new flag INCLUDE_BUILD_SETTINGS. > CDI-GDB shows those settings for both Attach and Core launches. I think this was missing functionality. > Finally, from the user's perspective, should we have the same order of Project > and Binary in the main tab between CDI and DSF? As I said, I felt the order > for CDI should have been reversed, but having the two tabs so subtly different > may be confusing to the user. Maybe an email to the list is in order? I thought it was better to have both launch tab look the same, for user friendliness. I committed to HEAD.
Ken, can you review the minor changes I just made?
I think we can mark this bug as fixed, no?
Done.
