Skip to content

Conversation

@gzsombor
Copy link
Contributor

@gzsombor gzsombor commented Jun 30, 2022

What it does

Add coloring to the stack frames of the debug thread view and add an action to enable/disable colorization from the context menu.
All of the categorize can be switched on or off
Use separate images for different stack frame categories.

How to test

Start debugging an application - notice, that the stack frames in the debug thread view are colorful, so it is much easier to find the user code, the test methods etc...

Author checklist

@SarikaSinha SarikaSinha self-requested a review July 1, 2022 03:52
@gzsombor gzsombor force-pushed the bug-stack-coloring branch 2 times, most recently from 7a91818 to 077e37a Compare July 3, 2022 12:40
@gzsombor
Copy link
Contributor Author

gzsombor commented Jul 4, 2022

I've improved the patch, after the second commit, not only the stack frames will be color coded, but now, the frames from the library/frameworks can be hidden - so in effect, only frames coming from source code written by the user - will be visible, making the debug experience much more focused on the potentially more interesting parts of the stack.

@iloveeclipse
Copy link
Member

Few impressions without looking into the code.

Bad:

I see some issues with the preference page

  • layout is strange (generic options are split by two "filter" tables)
  • wording is inconsistent to platform ( "mark" is probably "highlight" ?)
  • wrong / inconsistent capitalization (please check how text on other preference pages look like)
  • wording is partly unclear: "differently" to what?

Also by default, because of changed text color most of the stack is unreadable (loss of contrast, instead of black on white we have to read green on white etc). This is for me (and probably others working hours with IDE) a big issue, I have troubles to see the stack without looking closer to display.

Problematic concept

  • "Grouping" of filtered stack is interesting, but how one can inspect the "filtered" frames? That must at least have a way to expand the tree on demand. Also there are exceptions now, see below.

Good:

  • Background color is white by default.
  • You seem to have changed label decorator for frames, so different kind of code has different icon. That is a better approach as to change background & foreground

So I personally would appreciate if by default we only change icons used for different frame types, but not the foreground/background colors.

java.lang.ClassCastException: class org.eclipse.jdt.internal.debug.core.model.GroupedStackFrame cannot be cast to class org.eclipse.debug.core.model.IStackFrame (org.eclipse.jdt.internal.debug.core.model.GroupedStackFrame is in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @3c22db9b; org.eclipse.debug.core.model.IStackFrame is in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @4f6442df)
	at org.eclipse.debug.internal.ui.sourcelookup.SourceLookupFacility.displaySource(SourceLookupFacility.java:930)
	at org.eclipse.debug.internal.ui.elements.adapters.StackFrameSourceDisplayAdapter.displaySource(StackFrameSourceDisplayAdapter.java:27)
	at org.eclipse.debug.internal.ui.sourcelookup.SourceLookupService.displaySource(SourceLookupService.java:148)
	at org.eclipse.debug.internal.ui.sourcelookup.SourceLookupService.displaySource(SourceLookupService.java:132)
	at org.eclipse.debug.internal.ui.sourcelookup.SourceLookupService.debugContextChanged(SourceLookupService.java:62)
	at org.eclipse.debug.internal.ui.contexts.DebugWindowContextService$1.run(DebugWindowContextService.java:228)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.debug.internal.ui.contexts.DebugWindowContextService.notify(DebugWindowContextService.java:225)
	at org.eclipse.debug.internal.ui.contexts.DebugWindowContextService.notify(DebugWindowContextService.java:201)
	at org.eclipse.debug.internal.ui.contexts.DebugWindowContextService.debugContextChanged(DebugWindowContextService.java:410)
	at org.eclipse.debug.ui.contexts.AbstractDebugContextProvider$1.run(AbstractDebugContextProvider.java:76)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.debug.ui.contexts.AbstractDebugContextProvider.fire(AbstractDebugContextProvider.java:73)
	at org.eclipse.debug.internal.ui.views.launch.LaunchView$ContextProviderProxy.debugContextChanged(LaunchView.java:509)
	at org.eclipse.debug.ui.contexts.AbstractDebugContextProvider$1.run(AbstractDebugContextProvider.java:76)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.debug.ui.contexts.AbstractDebugContextProvider.fire(AbstractDebugContextProvider.java:73)
	at org.eclipse.debug.internal.ui.views.launch.LaunchView$TreeViewerContextProvider.activate(LaunchView.java:378)
	at org.eclipse.debug.internal.ui.views.launch.LaunchView.lambda$0(LaunchView.java:471)
	at org.eclipse.jface.viewers.Viewer$1.run(Viewer.java:151)

@gzsombor
Copy link
Contributor Author

gzsombor commented Jul 5, 2022

Thanks for the quick and thorough review!
Visibility and usability is also important to me - as I spend similarly hours debugging, and navigating in deep thread stacks. I'm not a UX expert/designer, but the idea behind this feature is, that it's much simpler to distinguish things based on their color, than to actually reading the labels. And it's useful to lead the user's eye to the important/relevant parts, so it's not accidental, that there are frames, which colored in a less readable way 😉
After using this patch for nearly a year - and using the collapsing feature for nearly a day - in my experience, it's very helpful.
Just compare this 3 images, and try to guess, what's going on, where are we:

  1. without coloring, only the new icons are visible - the full stack is too deep to fit on my screen screenshot-no-colors
  2. with colors:
    screenshot-colors It's getting better, it's easy to find the test code, which called our code.
  3. and after hiding the non relevant frames
    screenshot-collapsed

For me, code - and stack frames - could be categorized into 3 groups:

  1. production code
  2. test code
  3. code which I can't or don't want to change, and most of the time, I don't want to look into. This is somewhat vague category - and this could change from project to project.
    To match this reality, I'm thinking, that the visual distinction between stack frames coming from library code, JVM code and runtime generated code is not that important, so the colors could be more similar to each others... But as these colors are customizable I'm not too concerned.

I've fixed the ClassCastException, and tried to adjust the labels in the preference page, so make it more consistent.
The idea behind the "Highlighting" filter is to mark some code layer in your codebase, so it can be easily distinguished - for example highlight a ServletFilter which handles security, or transaction handling, etc.
I think, I can make this GroupedStackFrame to be expandable - currently, you need to click on the tool menu, to switch off the frame collapsing

@iloveeclipse
Copy link
Member

Note: the "collapsed" thing would also need to be switched off while doing "Copy stack" operation (which is unfortunately uses the tree from UI and not the actual stack behind), otherwise this will be broken.

For the "collapsing/uncollapsing" - would it make sense to put "hidden" part into a (collapsed) subtree?
Not sure how that would look like, but that probably would fix the "Copy stack" issue and missing possibility to intuitively see the hidden part.

Regarding the colors: looking on screenshots above I see my concerns only confirmed. I understand the purpose of the patch, but that heavily affects readability of the stack. All the "fuzzy" colors make stack not readable for me, the contrast is simply gone. I work every day with stack traces, not being easily read them is a big issue for me.

The possibility to change colors is OK, but defaults should be either "black on white" (== no customization) or in color pairs that ensure good contrast. Also note, that this should consider "themes" (Light/Dark/whatever). Might be we simply disable coloring by default an let users adopt that to they needs.

Icons alone are "good enough" for me as it shown in first screenshot.

Note: if we allow to hide/color by some patterns, the preferences that do that should also consider product preferences set via product customization (means, not simply get instance scope but use preferences service with all scopes).

@gzsombor gzsombor force-pushed the bug-stack-coloring branch from d304d31 to 543e52c Compare July 15, 2022 22:52
@gzsombor gzsombor force-pushed the bug-stack-coloring branch from 543e52c to dcbc7c2 Compare August 12, 2022 22:54
@gzsombor gzsombor force-pushed the bug-stack-coloring branch from 81c36cc to 6eab4cd Compare March 5, 2023 16:44
@gzsombor gzsombor force-pushed the bug-stack-coloring branch 3 times, most recently from 0dcf953 to 02924d9 Compare March 14, 2023 07:31
@gzsombor gzsombor force-pushed the bug-stack-coloring branch from 02924d9 to bf6de43 Compare April 30, 2023 23:05
@gzsombor gzsombor force-pushed the bug-stack-coloring branch from bf6de43 to c650189 Compare July 10, 2023 07:37
@gzsombor gzsombor force-pushed the bug-stack-coloring branch 2 times, most recently from 99c0d15 to 06b6655 Compare July 26, 2023 11:48
@gzsombor
Copy link
Contributor Author

Sorry for the comically long silence!

I've added the collapsed frames into the tree, so you can investigate if you really need:
test-Screenshot_20230728_090314

which become this, when you expand it:

expanded

I was not aware, that we can copy the stack trace! I dig into the code, and it doesn't seem to be an easy way to customize the returned lines, but honestly, the current behavior doesn't look too bad. For example, I've got this:

Thread [main] (Suspended (breakpoint at line 69 in UserService))	
	UserService.activateRegistration(String) line: 69	
	12 collapsed frames	
	AccountResource.activateAccount(String) line: 88	
	21 collapsed frames	
	AccountResourceIntTest.testActivateAccountWithWrongKey() line: 461	
	86 collapsed frames	

Definitely, if someone needs that 12+21+86 lines, is not that great, but otherwise, it's much more understandable. And when you open the 'collapsed' part, it will add those lines to the output.


	Thread [main] (Suspended (breakpoint at line 69 in UserService))	
	UserService.activateRegistration(String) line: 69	
	12 collapsed frames	
		UserService$$FastClassBySpringCGLIB$$a39521cd.invoke(int, Object, Object[]) line: not available	
		MethodProxy.invoke(Object, Object[]) line: 218	
		CglibAopProxy$CglibMethodInvocation.invokeJoinpoint() line: 793	
		CglibAopProxy$CglibMethodInvocation(ReflectiveMethodInvocation).proceed() line: 163	
		CglibAopProxy$CglibMethodInvocation.proceed() line: 763	
		TransactionInterceptor$1.proceedWithInvocation() line: 123	
		TransactionInterceptor(TransactionAspectSupport).invokeWithinTransaction(Method, Class<?>, InvocationCallback) line: 388	
		TransactionInterceptor.invoke(MethodInvocation) line: 119	
		CglibAopProxy$CglibMethodInvocation(ReflectiveMethodInvocation).proceed() line: 186	
		CglibAopProxy$CglibMethodInvocation.proceed() line: 763	
		CglibAopProxy$DynamicAdvisedInterceptor.intercept(Object, Method, Object[], MethodProxy) line: 708	
		UserService$$EnhancerBySpringCGLIB$$cddcde66.activateRegistration(String) line: not available	
	AccountResource.activateAccount(String) line: 88	
	NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method]	
	NativeMethodAccessorImpl.invoke(Object, Object[]) line: 62	
	5 collapsed frames	

I've adjusted the colors, hopefully, it's now more readable for everyone, as you can see from the screenshot :)
I have to dig deeper how color customization,dark/light theme handling, and the product customization works...

@akurtakov
Copy link
Contributor

What is the status of this one? I find the amount of changes overwhelming for me to review. If we can get smaller standalone chunks to review it would be easier to make progress.

@eclipse-jdt-bot
Copy link
Contributor

eclipse-jdt-bot commented Nov 27, 2024

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.tests/META-INF/MANIFEST.MF
org.eclipse.jdt.debug.tests/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 8b49ab2213be9619722971a88c955bccc4c7ca9d Mon Sep 17 00:00:00 2001
From: Eclipse JDT Bot <[email protected]>
Date: Wed, 11 Jun 2025 21:25:19 +0000
Subject: [PATCH] Version bump(s) for 4.37 stream


diff --git a/org.eclipse.jdt.debug.tests/META-INF/MANIFEST.MF b/org.eclipse.jdt.debug.tests/META-INF/MANIFEST.MF
index c95cd608d..3da93ba22 100644
--- a/org.eclipse.jdt.debug.tests/META-INF/MANIFEST.MF
+++ b/org.eclipse.jdt.debug.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.jdt.debug.tests; singleton:=true
-Bundle-Version: 3.12.650.qualifier
+Bundle-Version: 3.12.750.qualifier
 Bundle-ClassPath: javadebugtests.jar
 Bundle-Activator: org.eclipse.jdt.debug.testplugin.JavaTestPlugin
 Bundle-Vendor: %providerName
diff --git a/org.eclipse.jdt.debug.tests/pom.xml b/org.eclipse.jdt.debug.tests/pom.xml
index 0c8fbfce3..b7f65e5d6 100644
--- a/org.eclipse.jdt.debug.tests/pom.xml
+++ b/org.eclipse.jdt.debug.tests/pom.xml
@@ -18,7 +18,7 @@
   </parent>
   <groupId>org.eclipse.jdt</groupId>
   <artifactId>org.eclipse.jdt.debug.tests</artifactId>
-  <version>3.12.650-SNAPSHOT</version>
+  <version>3.12.750-SNAPSHOT</version>
   <packaging>eclipse-test-plugin</packaging>
   <properties>
     <testSuite>${project.artifactId}</testSuite>
-- 
2.49.0

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

@jukzi
Copy link
Contributor

jukzi commented Dec 4, 2024

Please get this PR finished or mark as Draft or close.

@gzsombor gzsombor force-pushed the bug-stack-coloring branch 4 times, most recently from bab3def to 42c07aa Compare March 21, 2025 08:58
@gzsombor gzsombor force-pushed the bug-stack-coloring branch from 42c07aa to e1eb85e Compare March 27, 2025 06:43
@gzsombor gzsombor force-pushed the bug-stack-coloring branch from e1eb85e to a8431df Compare April 8, 2025 22:26
@gzsombor gzsombor force-pushed the bug-stack-coloring branch from a8431df to 857e5b9 Compare June 11, 2025 21:23
@gzsombor gzsombor force-pushed the bug-stack-coloring branch from f9c4461 to f695037 Compare July 13, 2025 07:31
@gzsombor gzsombor force-pushed the bug-stack-coloring branch 2 times, most recently from db51a57 to d6c39da Compare August 15, 2025 11:38
and add an action to enable/disable colorization from the context menu.
  All of the categorize can be switched on or off
Add a couple of tests for stack frame grouping.
@SougandhS SougandhS added the enhancement New feature or request label Oct 25, 2025
@SougandhS
Copy link
Member

Preference page looks good 👍
Screenshot 2025-10-25 at 10 41 44 AM

@iloveeclipse iloveeclipse linked an issue Oct 27, 2025 that may be closed by this pull request
Copy link
Member

@SougandhS SougandhS left a comment

Choose a reason for hiding this comment

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

@gzsombor

The highlight stackframe is not working properly and if it is highlighted how can a user disable it ?

My.Movie.mp4

@iloveeclipse
Copy link
Member

@gzsombor : could we separate this PR in two:

  1. "Stack Frames" preference page for configuration of already existing "stack frames" feature => Add preference page for "Collapse" stack frames feature #798
  2. Highlighting/coloring stack frames and addition to "Stack Frames" preference page for it.

I fear that the time we have till M3 is not enough for the 2) but it should be possible to have 1) merged in.

@iloveeclipse
Copy link
Member

@gzsombor : could we separate this PR in two

@gzsombor Ping?

@gzsombor
Copy link
Contributor Author

It's unclear for me, why the build failed for #801 , the reported new 80+ warning is not related to my change

@iloveeclipse
Copy link
Member

It's unclear for me, why the build failed for #801 , the reported new 80+ warning is not related to my change

It's because of compiler changed meanwhile, see eclipse-jdt/eclipse.jdt.core#4572 and especially eclipse-jdt/eclipse.jdt.core#4569. I will try to change baseline, I believe it should be possible somehow.

@gzsombor
Copy link
Contributor Author

It's unclear for me, why the build failed for #801 , the reported new 80+ warning is not related to my change

It's because of compiler changed meanwhile, see eclipse-jdt/eclipse.jdt.core#4572 and especially eclipse-jdt/eclipse.jdt.core#4569. I will try to change baseline, I believe it should be possible somehow.

I see, thanks, makes sense!

Is there any reason not to remove those long deprecated classes? IMHO they are not used outside of some tests, but that's probably would trigger some other, API-incompatibility warning....

@iloveeclipse
Copy link
Member

Is there any reason not to remove those long deprecated classes? IMHO they are not used outside of some tests, but that's probably would trigger some other, API-incompatibility warning....

I haven't looked into details, it is just matter of time and resources. If there is API, one has to annotate it "for deleting" and after two or three releases it can be removed. Non API can be immediately removed, assumed it was deprecated long time ago.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add preference page for "Collapse" stack frames feature

6 participants