Remove ToggleInstructionStepModeCommand from Run Menu and Main Toolbar.

[raghucssit]

As ToggleInstructionStepModeCommand is contributed in too many places. We can remove it from Main Toolbar and Run Command.

see #1142

[github-actions]

Test Results

   636 files     636 suites   35m 57s ⏱️
11 440 tests 11 295 ✅ 144 💤 1 ❌
11 462 runs  11 317 ✅ 144 💤 1 ❌

For more details on these failures, see this check.

Results for commit 3e4a9d5.

♻️ This comment has been updated with latest results.

[iloveeclipse]

@jonahgraham : a review is welcome.

[raghucssit]

@jonahgraham The motivation for this fix is, " If property tester which decides the command has to the visible or not is not loaded then command is visible by default". Once the Property Tester is loaded by any dependents on the platform then it checks with the property tester and hides it. This is a bug/limitation in the eclipse platform.
Here is the detailed discussion:
eclipse-platform/eclipse.platform.ui#2900
As ToggleInstructionStepModeCommand is contributed to main toolbar, it is by default is visible even without any context.
So I have decided to remove it from there.
And it continues to be available at 'Debug View' toolbar.
Please check if we can remove this from main toolbar.

[jonahgraham]

@jonahgraham : a review is welcome.

😢 I'm sorry that this slipped through the cracks - I was having extended down time when this came in.

[jonahgraham]

@Kummallinen / @Torbjorn-Svensson (or any other CDT committer, especially one that has a CDT based product):

WDYT about removing instruction stepping mode from the main toolbar? Do your products rely/expect this? If so, can we put the onus on the product integrator to add this button back in for their use cases?

[iloveeclipse]

So far no feedback. Can we merge and see if anyone complains? I guess not :-)

[Kummallinen]

Sorry only just saw the notification for this.

For embedded development this is commonly used so would annoy our users if it were removed. While we can put it back for our products users of Embedded CDT would likely miss it & users of multiple plugins sets may end up with duplicates if vendors need to add this back in themselves

[iloveeclipse]

The original issue is that the button is shown in non-CDT context as long as CDT is not loaded. This is a limitation of an extension point because it has to use tester classes to check the button / CDT debugging state, and we don't want CDT to be loaded if it is not used.

What about following options:

1. Declare an activity that is disabled by default & bound that to the button. Whoever needs that can enable activity (either by product customization or via Preferences->Capabilities).
2. Declare an activity that is disabled by default & bound that to the button. Additionally, add code to the corresponding bundle activator that would enable the activity on bundle startup and disable on shutdown.
3. Declare an activity that is disabled by default & bound that to the button. Additionally, enable the activity first time CDT debugger starts (and do not disable it later).
4. Any other ideas ? ...

WDYT?

[ruspl-afed]

What if we add an "id" attribute for these contributions to let interested parties filter them out with E3 activity?

[raghucssit]

contributions

Contribution Element already have an id attribute.
But Menu Manager don't use this id for filtering, Instead it uses actual Command ID.
I am working on supporting this id for activity filtering in Menu Manger.
Please check below image where I added attribute to Menu Contribution.

Is my understanding correct regarding your suggestion ?

[ruspl-afed]

I am working on supporting this id for activity filtering in Menu Manger

Thank you!

Since command id and menu contribution id is a kind of API (for example to filter it out with activity) I would recommend to avoid internal segment in it and find better formulation for it, considering the following:

1. start from contributing bundle symbolic name
2. add segments like menu and command that points to an extension and element
3. end with a key for concrete contribution ("toggle instruction step mode" in our case)

As a result, we may have something like
org.eclipse.cdt.debug.ui.menu.command.toggle_instruction_step_mode
And it's relatively easy to detect and filter such an id for both machines and humans.

We use this approach for our products and recommend it to our customers because it greatly simplifies code maintenance.

[raghucssit]

I am working on supporting this id for activity filtering in Menu Manger

Thank you!

Since command id and menu contribution id is a kind of API (for example to filter it out with activity) I would recommend to avoid internal segment in it and find better formulation for it, considering the following:

1. start from contributing bundle symbolic name
2. add segments like menu and command that points to an extension and element
3. end with a key for concrete contribution ("toggle instruction step mode" in our case)

As a result, we may have something like org.eclipse.cdt.debug.ui.menu.command.toggle_instruction_step_mode And it's relatively easy to detect and filter such an id for both machines and humans.

We use this approach for our products and recommend it to our customers because it greatly simplifies code maintenance.

Please check this PR.
#1221
We can already define an id for a contribution item. And that can be used for activity support.
I have updated PR to remove all the code needed to activate or deactivate in the Bundle Activator.

[iloveeclipse]

@raghucssit : please update this PR, it has merge issues.
@jonahgraham : any chance for a review?

[raghucssit]

@raghucssit : please update this PR, it has merge issues. @jonahgraham : any chance for a review?

@iloveeclipse
May be we can close this PR because we have achieved hiding of this Action using activity support. So by default it is not visible on Main Toolbar and Run Menu.
I have verified this on latest cdt nightly build.

Here is the PR.
#1221

[iloveeclipse]

Ideally users shouldn't need to use "low-level" and not well known features like Capabilities to have a clean and not overloaded UI.

So while one could deactivate the extra buttons/menus via Capabilities, better to have clean UI from beginning. So would be nice if someone from CDT team could review this PR. Maybe @jld01?

[jonahgraham]

I think this is ok - I have added it to the agenda of the CDT call today #1340 to progress this.

I checked the help - and the two references to it in the help only refer to the Debug View anyway:

• https://help.eclipse.org/latest/topic/org.eclipse.cdt.doc.user/tasks/cdt_t_disassembly.htm
• https://help.eclipse.org/latest/topic/org.eclipse.cdt.doc.user/reference/cdt_u_dbg_view.htm

[jonahgraham]

The consensus at the CDT call was to leave it in the main tool bar/Run menu, but for me to spend some time understanding what the issues here are.

My initial understanding of the current implementation contributed in #1221 the buttons won't appear if there is no active debug that the buttons apply to. However #1143 (comment) points out that because of a (unfixable) bug in the Eclipse Platform the icons still appear.

On reviewing the impact of these buttons polluting the UI when CDT isn't in use, and that CDT itself has the instruction step documented as being in the Debug view I would recommend merging this.

During the CDT call @Kummallinen / @Torbjorn-Svensson were concerned about this change (#1221 (comment) ) - and they were involved in the previous discussions in #866 - so I want to wait for them to 👍 or 👎 this change.

[Kummallinen]

Can we move these to a "org.eclipse.cdt.embedded.ui" bundle, or similar? That can be installed by people doing embedded development who will want this function and/or included by vendors using CDT. Could then be used for other similar things in future.

[Kummallinen]

See my previous comment about moving to a new bundle.

(forgot to make that as a proper review comment)

[ruspl-afed]

Can we move these to a "org.eclipse.cdt.embedded.ui" bundle, or similar?

This is interesting direction of discussion, since we already had a few points (re: Core Build vs MBS ) where preferences of embedded developers may be different.

[jonahgraham]

This seems stuck again - moving to a draft until we have a proposal that is widely acceptable by current CDT committers.