Migrate ToggleInstructionStepModeAction to ToggleInstructionStepModeCommand.

[raghucssit]

All the contributions of the the action ToggleInstructionStepModeAction has been replaced ToggleInstructionStepModeCommand.
Enabled when introduced. Which enabled the command only when C/CPP application is under debug in Debug View.

see #865

[raghucssit]

@iloveeclipse FYI

[raghucssit]

This command is contributed in 4 places. I think it is too redundent.
I belive we can restrict it to Debug View toolbar. Current implementation has as it is in old Action.

[Torbjorn-Svensson]

I've just had a quick glance at the changes and some minor remarks.

[raghucssit]

@Torbjorn-Svensson This command is currently contributed in 4 locations. IMHO it is too much.
My suggestion is, We can just keep it on DebugView Toolbar and remove it from rest of the locations.
WDYT ?

[Torbjorn-Svensson]

@Torbjorn-Svensson This command is currently contributed in 4 locations. IMHO it is too much. My suggestion is, We can just keep it on DebugView Toolbar and remove it from rest of the locations. WDYT ?

I'm not so sure that it's a good idea to remove all other locations. The one next to the run control widget in the "main" toolbar is the one I usually use/check to see what mode I'm currently in.
I'd say that they were added to the various locations for a reason and there should be a strong case for not keeping them there.
@jonahgraham / @jld01 Do you any opinion on what locations to keep and what to remove?

[raghucssit]

Fixed all the review comments.

[iloveeclipse]

@jonahgraham : could you please check? Would be nice to get it into next release.

[ruspl-afed]

Perhaps you may want to avoid API removal if you want this change to be integrated faster @raghucssit @iloveeclipse
In any case the version of org.eclipse.cdt.debug.core bundle should be incremented according to the scope of changes.

[iloveeclipse]

Perhaps you may want to avoid API removal

Which one?

Are the test failures known or they are result of this change?

[ruspl-afed]

Which one

debug/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/model/ITargetProperties.java

[iloveeclipse]

Which one

debug/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/model/ITargetProperties.java

OK, I've overlooked that.
@raghucssit : why is this done at all, I don't see any comment asking for that? I see the change in DsfSteppingModeTarget but I don't see the reason why is is needed!

[Torbjorn-Svensson]

Are the test failures known or they are result of this change?

I would say that they are due to changes in this PR as other PRs are green.

[raghucssit]

Perhaps you may want to avoid API removal if you want this change to be integrated faster @raghucssit @iloveeclipse In any case the version of org.eclipse.cdt.debug.core bundle should be incremented according to the scope of changes.

incremented version number.

[raghucssit]

@Torbjorn-Svensson Fixed the issues mentioned.

[raghucssit]

Which one

debug/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/model/ITargetProperties.java

OK, I've overlooked that. @raghucssit : why is this done at all, I don't see any comment asking for that? I see the change in DsfSteppingModeTarget but I don't see the reason why is is needed!

My bad. I checked for the client in code base. There was none jus removed it. I forgot somewhere/someone in the world may be using it. Now i have added it back.

[raghucssit]

@Torbjorn-Svensson org.eclipse.cdt.tests.dsf.gdb tests are not executed in others branch except mine. Is it because others don't have changes in this plug-in ? Or something wrong ?

2024-07-14T21:06:20.2316535Z [INFO] --- tycho-surefire:4.0.6:test (default-test) @ org.eclipse.cdt.tests.dsf.gdb ---
2024-07-14T21:06:20.2349398Z [INFO] Skipping tests
2024-07-14T21:06:20.2349847Z [INFO] 
2024-07-14T21:06:20.2350927Z [INFO] --- tycho-p2:4.0.6:p2-metadata (baselinereplace-p2-metadata) @ org.eclipse.cdt.tests.dsf.gdb ---
2024-07-14T21:06:20.2444000Z [INFO] 
2024-07-14T21:06:20.2445520Z [INFO] --- tycho-p2-extras:4.0.6:compare-version-with-baselines (compare-attached-artifacts-with-release) @ org.eclipse.cdt.tests.dsf.gdb ---
2024-07-14T21:06:20.2448646Z [INFO] Execution was skipped
2024-07-14T21:06:20.2449144Z [INFO] 

I have done lot of analysis but Not able to find why do they fail ? All the tests in this plugin passes fine in m local build. Maven as well as inside eclipse debug session.
I will just try to create some dummy PR by making some simple change and increasing the version number and check if these tests passes there.

[raghucssit]

@Torbjorn-Svensson Thank you so much for triggering my test pr build.
My assumptions are correct. Those tests simply fails. My changes are not reason for them.
https://github.com/eclipse-cdt/cdt/runs/28768126974

Now I am not sure how to proceed with this.
gdb commands are not working as expected according the failures. Example Step Into Selection does not work from test but it works UI. I have tested manually with the same test org.eclipse.cdt.tests.dsf.gdb/data/launch/src/StepIntoSelectionTestApp.cc

[raghucssit]

@ruspl-afed As i have mentioned alreday. org.eclipse.cdt.tests.dsf.gdb tests are not executed on main branch itself.
main
SO the issues reported on my branch were not discovered by anybody else so far. I mean those issues alreday exist and are not because of my change.
Here is the sample PR which shows those tests simply fails with simple change.
Now I am not sure we can merge this PR or not. Those tests don't fail in my local. I tried alot to make them fail to find out the root cause but no success. Even from the log there is no information.

Please help me is there anything I am missing here ?

[jonahgraham]

Here are the comments touching all aspects of the change. Hopefully my quotes make it obvious enough what I am replying to.

@jonahgraham / @jld01 Do you any opinion on what locations to keep and what to remove?

Please keep them.

@jonahgraham : could you please check? Would be nice to get it into next release.

I am all for this in CDT 12 - current main branch is working towards CDT 12.

@Torbjorn-Svensson Thank you so much for triggering my test pr build.

BTW @raghucssit if you enable GitHub actions on your fork you will be able to run all the builds without needing to ping anyone.

How will you be able to identify if this is needed or not?

We are targetting CDT 12. Go ahead with the removal of the interface as it seems to have low value and while possibly used the fact it continues to pull in deprecated Platform API is a problem anyway. I am ok to simplify. Please add a an entry in https://github.com/eclipse-cdt/cdt/blob/main/NewAndNoteworthy/CHANGELOG-API.md for removed public API.

As i have mentioned alreday. org.eclipse.cdt.tests.dsf.gdb tests are not executed on main branch itself.

These tests are known to fail - sorry you lost time to these, see #816

---

A minor error is that supportsInstructionStepping is no longer called. This means that instruction stepping button is visible and enabled in post mortem (core) debug sessions. This is a minor issue, but we should make it work, or remove the dead code.

---

In summary this change looks good. I hope that my fellow reviewers have had a chance to run it in their specific environments if they are concerned.

PS You have taken on some legacy code. The old code was written as it was (including seemingly underused interfaces) because it existed to handle the transition from now deleted CDI debugger to the DSF one. This action, along with all the infrastructure for instruction stepping, was written so it would work with both backends. Because of that this code also works with the DAP (Debug Adapter Protocol) back end.

[raghucssit]

A minor error is that supportsInstructionStepping is no longer called. This means that instruction stepping button is visible and enabled in post mortem (core) debug sessions. This is a minor issue, but we should make it work, or remove the dead code.

Fixed this in the org.eclipse.cdt.debug.internal.ui.actions.CDTDebugPropertyTester.

We are targetting CDT 12. Go ahead with the removal of the interface

org.eclipse.cdt.debug.core.model.ITargetProperties interface allowing us to maintain unique instruction_stepping_mode per target. If we remove this all the targets in Debug View will share common instruction_stepping_mode. Can we keep it to serve only this functionality ? Other option is the migrate only deprecated preferences. Which I played around and works fine. I initially removed ITargetProperties and that also worked fine but all the targets sharing the same instruction_stepping_mode For now I have not fixed in the PR. I will take call based on the feedback.

[jonahgraham]

This LGTM

I will take call based on the feedback.

I am fine with current solution, and your info makes it sound like it is better this way.

I will merge this in the coming days assuming no changes are requested by fellow reviewers.

[jonahgraham]

The 8 errors, 60 failed tests of latest commit are #816 and do not hold up acceptance of this PR.

[ruspl-afed]

@jonahgraham I asked to reduce API changes to a minimum in order to simplify the potential integration of this PR for the 11.x stream.

Why did I care about 11.x here? Because @iloveeclipse mentioned the next release and our default next release 12.0 is currently discussed to be in November 13, 2024 , that is after SimRel 2024-09 and this may be later than expected by default.

LGTM

[iloveeclipse]

Thanks everyone, I just wanted to have the change available in some next release, not necessarily in 11.x. 12.x is fine too.

[raghucssit]

This LGTM

I will take call based on the feedback.

I am fine with current solution, and your info makes it sound like it is better this way.

I will merge this in the coming days assuming no changes are requested by fellow reviewers.

I will plan to work on Deprecated Preferences later in a separate PR. For now this PR has no changes pending that were requested by reviewers.
Also I will plan to improve few others Actions like Pin to Debug Context action in VariablesView and ExpressionsView. They also should be visible only CDT debug context.

[Torbjorn-Svensson]

The only important remark I've got is that we are registering more listeners than we remove. This could lead to the GC would not remove object that could otherwise be discarded.

[Torbjorn-Svensson]

These are the last minor remarks I have.
If my fellow committers are happy with this change as-is, I will not block it.

Sorry that I missed these before :S

[jonahgraham]

@Torbjorn-Svensson can you please merge once you have re-reviewed.

[Torbjorn-Svensson]

LGTM
I just renamed the members of ToggleInstructionStepHandler to be prefix with f so that it aligns with other code in the same plugin. Will merge this as soon as the build is complete.

[Torbjorn-Svensson]

Thank you @raghucssit for your contribution!