Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean-up and simplify Tracing-Options processing #755

Merged
merged 1 commit into from
Oct 21, 2023
Merged

Clean-up and simplify Tracing-Options processing #755

merged 1 commit into from
Oct 21, 2023

Conversation

HannesWell
Copy link
Member

@fedejeanne these are the clean-ups I mentioned in #674 (review).
Since you are probably using tracing more often than I do, do you want to try/review the changes?

@github-actions
Copy link

github-actions bot commented Sep 21, 2023 •
edited
Loading

Test Results

     273 files  ±0       273 suites  ±0   1h 7m 4s ⏱️ + 2m 25s
  3 340 tests ±0    3 310 ✔️ ±0  30 💤 ±0  0 ❌ ±0 
10 317 runs  ±0  10 227 ✔️ ±0  90 💤 ±0  0 ❌ ±0 

Results for commit 3ba23aa. ± Comparison against base commit 9113147.

♻️ This comment has been updated with latest results.

@HannesWell HannesWell force-pushed the tracingOptionsCleanUp branch 2 times, most recently from f773b1d to 1dcf334 Compare September 24, 2023 20:39
@HannesWell
Copy link
Member Author

HannesWell commented Sep 24, 2023 •
edited
Loading

@fedejeanne Thanks for all your remarks. I pushed an enhanced commit that mainly changes the handling of boolean values in TracingPropertySource but also addresses your other remarks where action were required.

If possible, another view is more than welcome, but I would like to to submit this tomorrow evening in order to have at least this in M1.

@fedejeanne
Copy link
Contributor

fedejeanne commented Sep 25, 2023 •
edited
Loading

⚠️ WARNING ⚠️

I just tested the changes and found an error.

Steps to reproduce

  1. Launch a product --> a Run configuration will be created
  2. Open the Run configurations dialog and select the newly created PDE configuration
  3. Switch to the Tracing tab and Enable tracing (check it)
  4. Select a plugin and click Apply
  5. Select another plugin and click Apply again

This dialog comes up and asks you if you want to overwrite the previous configuration:
image

I tried clicking....:

  • Save: the dialog keeps coming up
  • Don't save: I get a SAXParseException (see below) and then my configuration is deleted
  • Cancel: I get the same SAXParseException (see below) and then the dialog pops up again

Here's the exception:

org.eclipse.debug.core.DebugException: org.xml.sax.SAXParseException; lineNumber: 39; columnNumber: 28; Zeichenreferenz "&#0" ist ein ungültiges XML-Zeichen. occurred while reading launch configuration file: D:\dev\eclipse-contributors-experimental\runtime-sdk.product\.metadata\.plugins\org.eclipse.debug.core\.launches\aquintosEE.product.launch.
	at org.eclipse.debug.internal.core.LaunchManager.createDebugException(LaunchManager.java:777)
	at org.eclipse.debug.internal.core.LaunchManager.throwException(LaunchManager.java:2304)
	at org.eclipse.debug.internal.core.LaunchManager.getInfo(LaunchManager.java:1243)
	at org.eclipse.debug.internal.core.LaunchConfiguration.getInfo(LaunchConfiguration.java:487)
	at org.eclipse.debug.internal.core.LaunchConfigurationWorkingCopy.copyFrom(LaunchConfigurationWorkingCopy.java:477)
	at org.eclipse.debug.internal.core.LaunchConfigurationWorkingCopy.<init>(LaunchConfigurationWorkingCopy.java:101)
	at org.eclipse.debug.internal.core.LaunchConfiguration.getWorkingCopy(LaunchConfiguration.java:648)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationTabGroupViewer.handleRevertPressed(LaunchConfigurationTabGroupViewer.java:1545)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationsDialog.handleLaunchConfigurationSelectionChanged(LaunchConfigurationsDialog.java:1042)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationsDialog.lambda$1(LaunchConfigurationsDialog.java:614)
	at org.eclipse.jface.viewers.StructuredViewer$3.run(StructuredViewer.java:827)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:174)
	at org.eclipse.jface.viewers.StructuredViewer.firePostSelectionChanged(StructuredViewer.java:824)
	at org.eclipse.jface.viewers.StructuredViewer.setSelection(StructuredViewer.java:1676)
	at org.eclipse.jface.viewers.TreeViewer.setSelection(TreeViewer.java:1106)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationViewer.preservingSelection(LaunchConfigurationViewer.java:120)
	at org.eclipse.jface.viewers.StructuredViewer.refresh(StructuredViewer.java:1474)
	at org.eclipse.jface.viewers.ColumnViewer.refresh(ColumnViewer.java:523)
	at org.eclipse.jface.viewers.StructuredViewer.refresh(StructuredViewer.java:1428)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationsDialog.refreshLaunchConfigurationView(LaunchConfigurationsDialog.java:1344)
	at org.eclipse.debug.ui.PrototypeTab.postApply(PrototypeTab.java:486)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationsDialog.handleLaunchConfigurationSelectionChanged(LaunchConfigurationsDialog.java:1036)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationsDialog.lambda$1(LaunchConfigurationsDialog.java:614)
	at org.eclipse.jface.viewers.StructuredViewer$3.run(StructuredViewer.java:827)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:174)
	at org.eclipse.jface.viewers.StructuredViewer.firePostSelectionChanged(StructuredViewer.java:824)
	at org.eclipse.jface.viewers.StructuredViewer.setSelection(StructuredViewer.java:1676)
	at org.eclipse.jface.viewers.TreeViewer.setSelection(TreeViewer.java:1106)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationViewer.preservingSelection(LaunchConfigurationViewer.java:120)
	at org.eclipse.jface.viewers.StructuredViewer.refresh(StructuredViewer.java:1474)
	at org.eclipse.jface.viewers.ColumnViewer.refresh(ColumnViewer.java:523)
	at org.eclipse.jface.viewers.StructuredViewer.refresh(StructuredViewer.java:1428)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationsDialog.refreshLaunchConfigurationView(LaunchConfigurationsDialog.java:1344)
	at org.eclipse.debug.ui.PrototypeTab.postApply(PrototypeTab.java:486)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationTabGroupViewer$4.widgetSelected(LaunchConfigurationTabGroupViewer.java:342)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:252)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4274)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1066)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4072)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3660)
	at org.eclipse.jface.window.Window.runEventLoop(Window.java:823)
	at org.eclipse.jface.window.Window.open(Window.java:799)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationsDialog.open(LaunchConfigurationsDialog.java:1240)
	at org.eclipse.debug.ui.DebugUITools.lambda$1(DebugUITools.java:630)
	at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:67)
	at org.eclipse.debug.ui.DebugUITools.openLaunchConfigurationDialogOnGroup(DebugUITools.java:636)
	at org.eclipse.debug.ui.DebugUITools.openLaunchConfigurationDialogOnGroup(DebugUITools.java:574)
	at org.eclipse.debug.ui.actions.OpenLaunchDialogAction.run(OpenLaunchDialogAction.java:85)
	at org.eclipse.jface.action.Action.runWithEvent(Action.java:474)
	at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:580)
	at org.eclipse.jface.action.ActionContributionItem.lambda$4(ActionContributionItem.java:414)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4274)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1066)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4072)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3660)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1155)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:342)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1046)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:645)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:342)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:552)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:171)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:136)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:402)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:651)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:588)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1459)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1432)
Caused by: org.xml.sax.SAXParseException; lineNumber: 39; columnNumber: 28; Zeichenreferenz "&#0" ist ein ungültiges XML-Zeichen.
	at java.xml/com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.createSAXParseException(ErrorHandlerWrapper.java:204)
	at java.xml/com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.fatalError(ErrorHandlerWrapper.java:178)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLErrorReporter.reportError(XMLErrorReporter.java:400)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLErrorReporter.reportError(XMLErrorReporter.java:327)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLScanner.reportFatalError(XMLScanner.java:1465)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLScanner.scanCharReferenceValue(XMLScanner.java:1338)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLScanner.scanAttributeValue(XMLScanner.java:895)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl.scanAttribute(XMLDocumentFragmentScannerImpl.java:1522)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl.scanStartElement(XMLDocumentFragmentScannerImpl.java:1363)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl$FragmentContentDriver.next(XMLDocumentFragmentScannerImpl.java:2726)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl.next(XMLDocumentScannerImpl.java:605)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl.scanDocument(XMLDocumentFragmentScannerImpl.java:542)
	at java.xml/com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:889)
	at java.xml/com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:825)
	at java.xml/com.sun.org.apache.xerces.internal.parsers.XMLParser.parse(XMLParser.java:141)
	at java.xml/com.sun.org.apache.xerces.internal.parsers.DOMParser.parse(DOMParser.java:247)
	at java.xml/com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderImpl.parse(DocumentBuilderImpl.java:342)
	at org.eclipse.debug.internal.core.LaunchManager.createInfoFromXML(LaunchManager.java:827)
	at org.eclipse.debug.internal.core.LaunchManager.getInfo(LaunchManager.java:1236)
	... 78 more

I reverted your changes (back to master) and the error does not occur.

Could you please look into it before merging?

fedejeanne
fedejeanne approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just rebased on master and tested again and the error does not occur anymore. This PR LGTM

@HannesWell HannesWell force-pushed the tracingOptionsCleanUp branch 4 times, most recently from ead1671 to 07026c2 Compare October 21, 2023 15:21
@HannesWell
Copy link
Member Author

I just rebased on master and tested again and the error does not occur anymore. This PR LGTM

Thanks for verifying this again and your reviews in general. :)

After fixing the regression discovered by the tests (thanks Julian that we have them) and applying a few more small clean ups this is now ready.

@HannesWell HannesWell merged commit 474c50c into eclipse-pde:master Oct 21, 2023
16 checks passed
@HannesWell HannesWell deleted the tracingOptionsCleanUp branch October 21, 2023 18:24
@HannesWell HannesWell mentioned this pull request Oct 21, 2023
Merged
@fedejeanne
Copy link
Contributor

... (thanks Julian that we have them) ...

Will do 😄

Thank you too for merging! I'll try to come back to my original issue this week so we can improve the user experience when opening the PDE Run configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedejeanne fedejeanne fedejeanne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@HannesWell @fedejeanne