Restrict Pin View and Open View to active CPP debug context. #1049

raghucssit · 2025-01-21T14:27:41Z

That is if any CPP application is under debug. Because Pin View is never enabled other than CPP application debug session.

This also includes Action to Command migration.

see #1048

raghucssit · 2025-01-21T14:31:41Z

This is how fix looks like.
@

raghucssit · 2025-01-21T14:32:43Z

FYI, @iloveeclipse , @jonahgraham

github-actions · 2025-01-21T14:34:49Z

Test Results

635 files + 34 635 suites +34 55m 18s ⏱️ + 42m 25s
11 417 tests +1 217 11 206 ✅ +1 029 143 💤 +120 60 ❌ +60 8 🔥 +8
11 455 runs +1 217 11 244 ✅ +1 029 143 💤 +120 60 ❌ +60 8 🔥 +8

For more details on these failures and errors, see this check.

Results for commit cc045d1. ± Comparison against base commit 9e04dc5.

♻️ This comment has been updated with latest results.

raghucssit · 2025-01-21T14:39:58Z

Git diff is strange. I removed old Action delegates and added new Handlers. But it shows as old files are modified.

iloveeclipse · 2025-01-21T14:42:36Z

Git diff is strange. I removed old Action delegates and added new Handlers. But it shows as old files are modified.

Not strange, but clever. It detects a move/rename, because the remaining code similarity is high enough to the old one.

raghucssit · 2025-01-21T14:54:03Z

Oh bad.. There is a compilation problem. I will fix it. I did not have the project which is dependent on OpenNewViewAction so did not find in ws.

jonahgraham · 2025-01-21T15:01:22Z

...g.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/OpenNewViewHandler.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2010, 2014 Texas Instruments and others
+ * Copyright (c) 2025 Advantest Europe GmbH and others.


Git diff is strange. I removed old Action delegates and added new Handlers. But it shows as old files are modified.

Not strange, but clever. It detects a move/rename, because the remaining code similarity is high enough to the old one.

The copyright should probably not be changed here as a result since you have copied at least the function body to the new code, the copyright start date and original author should remain here.

Now I have fixed the copyright.

That is if any CPP application is under debug. Because Pin View is never enabled other than CPP application debug session. This also includes Action to Command migration. see eclipse-cdt#1048

jonahgraham

This looks pretty good, but there is new, unexpected, behaviour when I am debugging both Java and C in the same workspace. The pin context + new view buttons are visible and enabled on the Java context. Prior to this change, they were visible, but only the new view was enabled. See:

The interesting irony, is that with pin command enabled, it is working to pin Java contexts:

I think you should keep enablement condition otherwise pin is enabled on illegal contexts, even within CDT:

I think the enablement should be based on org.eclipse.cdt.debug.internal.ui.pinclone.PinCloneUtils.isPinnable(IWorkbenchPart, ISelection)

jonahgraham · 2025-01-22T19:03:09Z

...e.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/PinCommandEnablementTester.java

+					}
+				}
+			}
+			// Check if any CDT launch is active.


I am concerned that this is too narrow a check. I know of at least one CDT extender that doesn't extend GdbLaunch (not open source). The PDALaunch is supposed to demonstrate this, but that code has bitrotten and no longer works.

The actions used org.eclipse.cdt.debug.internal.ui.pinclone.PinCloneUtils.isPinnable(IWorkbenchPart, ISelection) to determine if the pin should be enabled. You could see if any of the launches have an adapter of type IPinProvider, and if they do, show the button.

org.eclipse.cdt.dsf.gdb.launching.GdbLaunch does not provide an adapter for IPinProvider. I need to find a way to reuse the org.eclipse.cdt.debug.internal.ui.pinclone.PinCloneUtils.isPinnable(IWorkbenchPart, ISelection) . It was bit difficult to achieve that. So I use the approach currently in place.

I wasn't sure by your wording in the last comment. Are you still looking into this? I would like to not break known existing extenders, I don't have access to their source now, but I think they still extend DsfLaunch, so at a minimum that should be required, along with checking if the launch is of that type, rather than exactly GdbLaunch.

The GdbExtendedLaunch example in the CDT source shows what many many extenders do. With the change as currently written, the pin and open new button disappear for those users.

Here is a screenshot that shows the buttons missing, along with how to change the launch type:

jonahgraham · 2025-01-22T20:17:32Z

...g/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/PinViewHandler.java

+	}
+
+	class ViewDisposeListener implements DisposeListener {
+		IViewPart viewPart;
+
+		public ViewDisposeListener(IViewPart vPart) {
+			viewPart = vPart;
+		}
+
+		@Override
+		public void widgetDisposed(DisposeEvent e) {
+			pinned.remove(viewPart);
+		}
+


Unreferenced class?

jonahgraham · 2025-01-22T20:22:23Z

...g/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/PinViewHandler.java

 	private IPartListener2 fPartListener;
-	private DebugContextPinProvider fProvider;
+	private static Set<IViewPart> pinned = new HashSet<>();
+	Image image;


Add access modifier

Suggested change

Image image;

private Image image;

jonahgraham · 2025-01-22T20:30:13Z

...e.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/PinCommandEnablementTester.java

+	@Override
+	public boolean test(Object receiver, String property, Object[] args, Object expectedValue) {
+		if (PIN_VIEW_COMMAND_PROP_TEST_NAME.equals(property)) {
+			// Check if the View was pinned, then button must be there to Unpin.


This code doesn't quite match the comment because the test of if (args[0].equals(vPart.getPartName())) only works for the original view and only works in English because the strings in the plugin.xml are not translated.

To test, open a second view on Variables and pin it. Stop the debug session and the pin disappears. But if you apply the pin to the original view it remains on closing.

In reply to the above note and #1049 (comment)

Note that you can update the comment rather than the code, but they should match. However the language part is a problem regardess.

Even if I plan to contribute a dedicated Command/Handler per view still problem cannot be solved because we can duplicate the View and the duplicated view still use the shared handler.

I would go simpler rather then more complicated. The pins could be left visible on all views if any view is still pinned?

I would go simpler rather then more complicated. The pins could be left visible on all views if any view is still pinned?

This is also not possible. Because PropertyTester has no idea which ToolBarItem from a specific View is being asked to show/hide.
The receiver is simply a workbench selection has no information about from which part that the ToolBarItem being asked to show/hide. So PropertyTester can never know whether that view was pinned.
I have only this information public boolean test(Object receiver, String property, Object[] args, Object expectedValue) and none of then give me the ViewPart or ToolBarItem. I need any of these to decide if I can leave that button visible or hide.

See https://github.com/eclipse-cdt/cdt/pull/1049/files/9a457cbd907698cbb64790b0ed2dc7929716202c#r1927439200

raghucssit · 2025-01-23T14:28:57Z

Migrating Action Contribution to Command/Handler is tedious.
The reason is platform maintains an Instance of IViewActionDelegate for each action contribution. i.e. an instance per view in this case.
As Action is always there(Not show/hide), IViewActionDelegate takes care of enable/disable. And IViewActionDelegate has the complete state of the Action like isChecked, IViewPart where it is added. Based on this controlling enable/disable has every information needed.

On the other hand, Command/Handler, Uses a single instance of Handler and single instance of Property Tester for all the contributions on the workbench. This make it hard to track from which Part/Command enablement request is coming. Command asks Property Tester each time when there is selection change on the workbench. It only supplies the changed selection. From this little information Property Tester cannon decide if it can display/hide the command.
This is the reason I maintained a list of pinned views and passed the view name as an argument from the command where is is requested.

Even if I plan to contribute a dedicated Command/Handler per view still problem cannot be solved because we can duplicate the View and the duplicated view still use the shared handler.

Command/Handler is good for reusing the common state of the Command on all the contributions. And Property Tester never knows what was the state of the Command in case of Toggle if we plan to decide show/hide based on the toggle state.(Currently that is how it is done for Pin Action. i.e If it is pinned(checked) never touch it for enable/disable).

I will evaluate how can I work around this limitation.

jonahgraham · 2025-01-23T18:05:13Z

...e.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/PinCommandEnablementTester.java

+			// Check if the View was pinned, then button must be there to Unpin.
+			Set<IViewPart> pinnedViews = PinViewHandler.getPinnedViews();
+			if (pinnedViews.size() > 0 && args.length == 1) {
+				for (IViewPart viewPart : pinnedViews) {
+					WorkbenchPart vPart = (WorkbenchPart) viewPart;
+					if (args[0].equals(vPart.getPartName())) {
+						return true;
+					}
+				}
+			}


@raghucssit what I was thinking is this, leave command visible everywhere if any pin is in use anywhere. That removes need for argument and removes current bug where it depends on whether it is primary or secondary view that the pin is applied to.

Suggested change

// Check if the View was pinned, then button must be there to Unpin.

Set<IViewPart> pinnedViews = PinViewHandler.getPinnedViews();

if (pinnedViews.size() > 0 && args.length == 1) {

for (IViewPart viewPart : pinnedViews) {

WorkbenchPart vPart = (WorkbenchPart) viewPart;

if (args[0].equals(vPart.getPartName())) {

return true;

}

}

}

// Check if any View was pinned, then leave pin visible on all views

if (!PinViewHandler.getPinnedViews().isEmpty()) {

return true;

}

This will enable Pin Command on all the Views if only one View is pinned. As this is the only instance used by all the Views to check if they can show Command.
Example pin Variable View, then change the selection to Java Debug Launch Config in the Debug View then we can see Pin Command shown for Expressions View. This is because Expressions View asks PropertyTester if it can show Pin Command and tester says Yes.!!

I am working on Non declarative approach. I have registered a IDebugContextListener with Debug Service. Now I get debug changes. Based on the change I will poll the views on the workbench contribute Action dynamically.
Hope this is not an overkill..:-)

Please see #1049 (review) AFAICT the pin command shows when Java process is selected with your version because there is some gdblaunch present pin commands are visible.

This will enable Pin Command on all the Views if only one View is pinned.

Yes - this is intent of my recommendation, make pin commands either visible or not visible globally rather than trying to do it per view. This is a simplification that I assume would resolve the issue. i.e. pin command is only visible if user is actually doing CDT work.

raghucssit · 2025-01-23T19:40:05Z

@jonahgraham I think i fixed the suggestion you gave me. I actually installed IDebugContextListener and reusing the PinCloneUtils.isPinnable(null, debugContext).
Please check my latest commit.
Here is how it works.

fix.mp4

jonahgraham · 2025-01-23T19:48:58Z

debug/org.eclipse.cdt.debug.ui/plugin.xml

+          <visibleWhen
+                checkEnabled="false">
+             <test
+                   args="Variables"


Now that you aren't using the arg, please remove them from the plugin.xml

Suggested change

args="Variables"

jonahgraham · 2025-01-23T19:54:08Z

...e.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/PinCommandEnablementTester.java

+	private ISelection debugContext;
+	private boolean isDclInstalled;


@jonahgraham I think i fixed the suggestion you gave me. I actually installed IDebugContextListener and reusing the PinCloneUtils.isPinnable(null, debugContext).

Sadly this violates the API contract of PropertyTester. See javadoc key part quoted:

So property testers should always be implemented in a stateless fashion.

If you are listening to the active debug context, then I think lots of commands do that already. Have you had a look at how other debug commands are implemented? I am quite short of time now, but I can find you a specific example later if you want.

find you a specific example later if you want.

For example, look at how debugContext variable is used in org.eclipse.debug.examples.ui (source link)

CDT also has reverse commands (E.g. org.eclipse.cdt.debug.ui.command.reverseStepInto) and the property tester ReverseDebuggingPropertyTester and expression definition org.eclipse.cdt.debug.ui.testIsReverseDebuggingEnabled which uses debugContext variable.

The CDT case is interesting, but more complicated because it is a retargettable command, whilst the o.e.debug.examples.ui is simpler.

the visibility of commands. Basically we need to have Debug View for an application under debug. So asking the selection from Debug View and checking it against an active DSF session should give the most accurate visibility condition. Along with this we plan to show the commands in all views if any of the view is pinned.

raghucssit · 2025-01-24T19:22:04Z

@jonahgraham I have fixed all the review comments.
The latest fix for property tester now asks the debug view for selection and then decides if it can show command based on if the selection is an active DSF session. Basically i am reusing existing property tester for that.
In other case, If any View is pinned we leave commands visible in all the views.
If developer not working on both Java and CPP both at the same time then there will not be any issues at all.
Please check if it is good.

If things are good then i will squash and push all the commits. I have made different commits to differentiate the changes to help review.

jonahgraham reviewed Jan 21, 2025

View reviewed changes

raghucssit force-pushed the migrate_pin_view_action branch from 877abc1 to 8e73224 Compare January 21, 2025 16:22

Restrict Pin View and Open View to active CPP debug context.

9a457cb

That is if any CPP application is under debug. Because Pin View is never enabled other than CPP application debug session. This also includes Action to Command migration. see eclipse-cdt#1048

raghucssit force-pushed the migrate_pin_view_action branch from 8e73224 to 9a457cb Compare January 21, 2025 16:24

raghucssit requested a review from jonahgraham January 21, 2025 16:26

jonahgraham requested changes Jan 22, 2025

View reviewed changes

jonahgraham reviewed Jan 23, 2025

View reviewed changes

Improved the Property Tester.

cc045d1

jonahgraham requested changes Jan 23, 2025

View reviewed changes

raghucssit added 2 commits January 24, 2025 20:10

Fix other review comments.

0b90e9b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict Pin View and Open View to active CPP debug context. #1049

Restrict Pin View and Open View to active CPP debug context. #1049

raghucssit commented Jan 21, 2025

raghucssit commented Jan 21, 2025

raghucssit commented Jan 21, 2025

github-actions bot commented Jan 21, 2025 •

edited

Loading

raghucssit commented Jan 21, 2025

iloveeclipse commented Jan 21, 2025

raghucssit commented Jan 21, 2025

jonahgraham Jan 21, 2025

raghucssit Jan 21, 2025

jonahgraham left a comment

jonahgraham Jan 22, 2025

raghucssit Jan 22, 2025

jonahgraham Jan 22, 2025

jonahgraham Jan 22, 2025

jonahgraham Jan 22, 2025

jonahgraham Jan 22, 2025

jonahgraham Jan 23, 2025

raghucssit Jan 23, 2025

jonahgraham Jan 23, 2025

raghucssit commented Jan 23, 2025

jonahgraham Jan 23, 2025

raghucssit Jan 23, 2025

raghucssit Jan 23, 2025

jonahgraham Jan 23, 2025

jonahgraham Jan 23, 2025

raghucssit commented Jan 23, 2025

jonahgraham Jan 23, 2025

jonahgraham Jan 23, 2025

jonahgraham Jan 24, 2025

raghucssit commented Jan 24, 2025 •

edited

Loading

		private ISelection debugContext;
		private boolean isDclInstalled;

Restrict Pin View and Open View to active CPP debug context. #1049

Are you sure you want to change the base?

Restrict Pin View and Open View to active CPP debug context. #1049

Conversation

raghucssit commented Jan 21, 2025

raghucssit commented Jan 21, 2025

raghucssit commented Jan 21, 2025

github-actions bot commented Jan 21, 2025 • edited Loading

Test Results

raghucssit commented Jan 21, 2025

iloveeclipse commented Jan 21, 2025

raghucssit commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghucssit commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghucssit commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghucssit commented Jan 24, 2025 • edited Loading

github-actions bot commented Jan 21, 2025 •

edited

Loading

raghucssit commented Jan 24, 2025 •

edited

Loading