Ignore HCR Failures for current session #347 #558

SougandhS · 2024-11-11T11:38:53Z

Enhancement : #347

Adds a checkbox to the HCR error alert box that allows users to ignore HCR failures only for the current debug session, rather than applying it to all future debug sessions.

Fixes : #347

What it does

How to test

Author checklist

I have thoroughly tested my changes
The change is following the coding conventions
I have signed the Eclipse Contributor Agreement (ECA)

SougandhS · 2024-11-11T12:08:54Z

Hi @jukzi ,
Could u please check this too when you are free.

org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/JavaHotCodeReplaceListener.java

org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/model/JDIDebugTarget.java

org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/ErrorDialogWithToggle.java

jukzi · 2024-11-11T12:14:21Z

As this seems to be new feature can it wait for M1?

SougandhS · 2024-11-11T12:17:16Z

As this seems to be new feature can it wait for M1?

Yeah sure, no problem 👍

eclipse-jdt-bot · 2024-11-26T14:05:26Z

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

org.eclipse.jdt.debug.ui/META-INF/MANIFEST.MF
org.eclipse.jdt.debug.ui/pom.xml

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From 0349b257fde13d14d686530066272b272ced6329 Mon Sep 17 00:00:00 2001
From: Eclipse JDT Bot <[email protected]>
Date: Wed, 27 Nov 2024 11:45:24 +0000
Subject: [PATCH] Version bump(s) for 4.35 stream


diff --git a/org.eclipse.jdt.debug.ui/META-INF/MANIFEST.MF b/org.eclipse.jdt.debug.ui/META-INF/MANIFEST.MF
index e651afa8a..06269ace2 100644
--- a/org.eclipse.jdt.debug.ui/META-INF/MANIFEST.MF
+++ b/org.eclipse.jdt.debug.ui/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.jdt.debug.ui; singleton:=true
-Bundle-Version: 3.13.600.qualifier
+Bundle-Version: 3.13.700.qualifier
 Bundle-Activator: org.eclipse.jdt.internal.debug.ui.JDIDebugUIPlugin
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
diff --git a/org.eclipse.jdt.debug.ui/pom.xml b/org.eclipse.jdt.debug.ui/pom.xml
index dceebf96d..597b96d81 100644
--- a/org.eclipse.jdt.debug.ui/pom.xml
+++ b/org.eclipse.jdt.debug.ui/pom.xml
@@ -18,7 +18,7 @@
   </parent>
   <groupId>org.eclipse.jdt</groupId>
   <artifactId>org.eclipse.jdt.debug.ui</artifactId>
-  <version>3.13.600-SNAPSHOT</version>
+  <version>3.13.700-SNAPSHOT</version>
   <packaging>eclipse-plugin</packaging>
   <properties>
   	<defaultSigning-excludeInnerJars>true</defaultSigning-excludeInnerJars>
-- 
2.47.0

Further information are available in Common Build Issues - Missing version increments.

SougandhS · 2024-11-27T06:22:14Z

Hi @jukzi , can you try committing again ?

iloveeclipse · 2024-11-27T06:26:06Z

org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/model/JDIDebugTarget.java

+	/**
+	 * Preference storage for active debug session.
+	 */
+	public static final Map<String, Object> debugSessionVariables = new ConcurrentHashMap<>();


Wjy is this a static map and why it is a map at all? If with the the "session" the whole Eclipse session meant, no map is needed, a Boolean flag will be enough. If with the "session" a specific debug session meant, the code is plain wrong as it would affect all other debug sessions runnijg in parallel or after the current one.

This can be used to store pref values only for current debug session. other pref values can also be added.

other pref values in the sense other features preferences value only for current debug session in future if it requires.

This can be used to store pref values only for current debug session

Then a static map is wrong. Try to start two sessions in parallel.

other pref values in the sense other features preferences value only for current debug session in future if it requires.

If future requires, the code can be refactored.

will remove static then.

other pref values in the sense other features preferences value only for current debug session in future if it requires.

If future requires, the code can be refactored.

will replace it with boolean flag as you suggested since its more appropriate.

I wasnt able to check without static but now I have modified it further to make it more debug sessions specific.

Tested with parallel debug launches

iloveeclipse · 2024-12-03T16:07:19Z

org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/ErrorDialogWithToggle.java

@@ -43,25 +47,37 @@ public class ErrorDialogWithToggle extends ErrorDialog {
 	/**
 	 * The message displayed to the user, with the toggle button
 	 */
-	private String fToggleMessage= null;
+	private String fToggleMessage1 = null;


Style: please don't initialize fields to default values, java does it automatically, but during debugging you will jump from field to field just because of this meaningless instructions here.

will remove default initialization

iloveeclipse · 2024-12-03T16:11:15Z

org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/ErrorDialogWithToggle.java

 		fStore.setValue(fPreferenceKey, !getToggleButton().getSelection());
+		if (fToggleButton2 != null) {
+			try {
+				JDIDebugTarget.hcrDebugSessionErrors.put(target.getName(), fToggleButton2.getSelection());


This is inappropriate place / dependency. The ErrorDialogWithToggle has no relationship to JDIDebugTarget and shouldn't have.

The HotCodeReplaceErrorDialog class that extends this dialog is the right place for HCR related code.

Yes, but I couldn't find any ways to access same from JavaHotCodeReplaceListener class, which checks the boolean value.

iloveeclipse · 2024-12-03T16:12:56Z

org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/ErrorDialogWithToggle.java

 	/**
 	 * The preference store which will be affected by the toggle button
 	 */
 	IPreferenceStore fStore= null;

+	public ErrorDialogWithToggle(Shell parentShell, String dialogTitle, String message, IStatus status, String preferenceKey, String toggleMessage1, String toggleMessage2, IPreferenceStore store) {


toggleMessage2 should be documented to be optional, but ideally it should be in HotCodeReplaceErrorDialog

Will provide documentation like you suggested

iloveeclipse · 2024-12-03T16:22:26Z

org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/model/JDIDebugTarget.java

+	/**
+	 * HCR failure alert pref for current debug session.
+	 */
+	public static final Map<String, Boolean> hcrDebugSessionErrors = new ConcurrentHashMap<>();


Still can't unerstand why do you use static map here, if you want restrict errors dialog for the specific current session. But same target can be started multiple times!
Why not add a simple not static boolean field & getter on JDIDebugTarget and instead of doing lookup on a non-inuque name only apply the new code for the known instances of the debug targets.
So NOT
failurePopUp = JDIDebugTarget.hcrDebugSessionErrors.getOrDefault(target.getName(), false);
but

if(target instanceof JDIDebugTarget jdiTarget){ failurePopUp = jdiTarget.shouldReportHcrErrors(); }

Thanks, will do the same you suggested

Adds a checkbox to the HCR error alert box that allows users to ignore HCR failures only for the current debug session, rather than applying it to all future debug sessions. Fixes : eclipse-jdt#347

jukzi reviewed Nov 11, 2024

View reviewed changes

org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/JavaHotCodeReplaceListener.java Outdated Show resolved Hide resolved

jukzi reviewed Nov 11, 2024

View reviewed changes

org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/model/JDIDebugTarget.java Outdated Show resolved Hide resolved

jukzi reviewed Nov 11, 2024

View reviewed changes

org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/ErrorDialogWithToggle.java Outdated Show resolved Hide resolved

SougandhS force-pushed the HCR_Ignore_this_session branch 2 times, most recently from d780561 to 1a41681 Compare November 11, 2024 12:37

jukzi force-pushed the HCR_Ignore_this_session branch from 1a41681 to 34717d0 Compare November 26, 2024 14:03

iloveeclipse requested changes Nov 27, 2024

View reviewed changes

SougandhS force-pushed the HCR_Ignore_this_session branch 4 times, most recently from 024d81c to fe1273b Compare November 27, 2024 11:43

SougandhS requested a review from iloveeclipse November 27, 2024 12:40

SougandhS force-pushed the HCR_Ignore_this_session branch from ba0149c to 42d462f Compare December 3, 2024 06:27

iloveeclipse reviewed Dec 3, 2024

View reviewed changes

iloveeclipse requested changes Dec 3, 2024

View reviewed changes

SougandhS force-pushed the HCR_Ignore_this_session branch 2 times, most recently from 47e421d to c40c922 Compare December 5, 2024 10:00

SougandhS requested a review from iloveeclipse December 5, 2024 11:38

Ignore HCR Failures for current session eclipse-jdt#347

ce76980

Adds a checkbox to the HCR error alert box that allows users to ignore HCR failures only for the current debug session, rather than applying it to all future debug sessions. Fixes : eclipse-jdt#347

SougandhS force-pushed the HCR_Ignore_this_session branch from c40c922 to ce76980 Compare January 1, 2025 02:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore HCR Failures for current session #347 #558

Ignore HCR Failures for current session #347 #558

SougandhS commented Nov 11, 2024 •

edited

Loading

SougandhS commented Nov 11, 2024

jukzi commented Nov 11, 2024

SougandhS commented Nov 11, 2024

eclipse-jdt-bot commented Nov 26, 2024 •

edited

Loading

SougandhS commented Nov 27, 2024

iloveeclipse Nov 27, 2024

SougandhS Nov 27, 2024

SougandhS Nov 27, 2024

iloveeclipse Nov 27, 2024

iloveeclipse Nov 27, 2024

SougandhS Nov 27, 2024

SougandhS Nov 27, 2024

SougandhS Nov 27, 2024

iloveeclipse Dec 3, 2024

SougandhS Dec 5, 2024

iloveeclipse Dec 3, 2024

SougandhS Dec 5, 2024

iloveeclipse Dec 3, 2024

SougandhS Dec 5, 2024

iloveeclipse Dec 3, 2024

SougandhS Dec 5, 2024

Ignore HCR Failures for current session #347 #558

Are you sure you want to change the base?

Ignore HCR Failures for current session #347 #558

Conversation

SougandhS commented Nov 11, 2024 • edited Loading

What it does

How to test

Author checklist

SougandhS commented Nov 11, 2024

jukzi commented Nov 11, 2024

SougandhS commented Nov 11, 2024

eclipse-jdt-bot commented Nov 26, 2024 • edited Loading

SougandhS commented Nov 27, 2024

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

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

SougandhS commented Nov 11, 2024 •

edited

Loading

eclipse-jdt-bot commented Nov 26, 2024 •

edited

Loading