Implement grouping of the stack frames #583

gzsombor · 2025-01-04T10:54:07Z

The rules how the frames are categorized are not editable as of now, just a single action is visible to switch on-off the behaviour

What it does

Groups - and hides - the supposedly not relevant stack frames from the debug views.

After

Expanding the group:

How to test

Start debugging, and notice that instead of having hundreds of frames only a couple are visible

Author checklist

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

gzsombor · 2025-01-14T08:00:37Z

@jukzi : Could you please have a look on this when you have time and check what needs to be done to be merged?

jukzi · 2025-01-14T08:06:34Z

unfortunately i am very limited in time. @SougandhS can you test/review this PR?

SougandhS · 2025-01-14T09:22:43Z

unfortunately i am very limited in time. @SougandhS can you test/review this PR?

will check this

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

...clipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/StackFramePresentationProvider.java

...se.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/monitors/JavaThreadContentProvider.java

SougandhS · 2025-01-14T09:45:34Z

...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/monitors/MonitorsAdapterFactory.java

@@ -40,7 +41,7 @@ public class MonitorsAdapterFactory implements IAdapterFactory {
 	public <T> T getAdapter(Object adaptableObject, Class<T> adapterType) {

    	if (IElementContentProvider.class.equals(adapterType)) {
-    		if (adaptableObject instanceof IJavaThread) {
+			if (adaptableObject instanceof IJavaThread || adaptableObject instanceof GroupedStackFrame) {


Some additional space was added here

Yeah, sorry. Files with mixed tabs and spaces are very confusing and it is easy to mess up them even more!

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

SougandhS · 2025-01-14T10:03:22Z

...lipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/JDIDebugUIPreferenceInitializer.java

+		store.setDefault(IJDIPreferencesConstants.PREF_ACTIVE_CUSTOM_FRAME_FILTER_LIST, ""); //$NON-NLS-1$
+		store.setDefault(IJDIPreferencesConstants.PREF_INACTIVE_CUSTOM_FRAME_FILTER_LIST, ""); //$NON-NLS-1$
+		store.setDefault(IJDIPreferencesConstants.PREF_COLLAPSE_STACK_FRAMES, true);
+


Try using org.eclipse.jface.util.Util.ZERO_LENGTH_STRING for empty string declarations, so you can avoid NON-NLS comments

Thanks, I didn't know about this class!

SougandhS · 2025-01-14T10:18:18Z

Stack icons looks bit odd when code stops in .java files

Icons are same as of .Class files

reference . On master -

SougandhS · 2025-01-14T10:29:37Z

if the breakpoint is in .class file then original stack (master) icons are shown

both

Feature looks awsm!

Would be nice if the icons are consistent

gzsombor · 2025-01-14T22:37:26Z

if the breakpoint is in .class file then original stack (master) icons are shown

both

Feature looks awsm! Would be nice if the icons are consistent

I'm happy that you liked the feature!
It is a long time, since I've written most of the code - I even forgot, that I've modified that part which returns images for the stack frames. I've modified again a bit, so the icon will be the same old, for all the prod/test codes, and have the 'jar' icon for classes coming from the libraries and 'platform'.

SougandhS · 2025-01-15T04:48:14Z

have the 'jar' icon for classes coming from the libraries and 'platform'.

Icons looks good now thanks!

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

SougandhS · 2025-01-15T04:57:01Z

Hi @gzsombor , could pls take the latest pull and squash everything to one commit only and push, so we can get rid of the build errors

SougandhS

LGTM 👍

jukzi · 2025-01-15T07:21:25Z

@gzsombor can you please update the pr description to include before/after screenshot to fatsly see what to expect?

jukzi · 2025-01-15T07:26:34Z

...clipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/StackFramePresentationProvider.java

+		} catch (DebugException e) {
+			JDIDebugUIPlugin.log(e);
+		}
+		frame.setCategory(result);


why does a getter getCategory set something? that is unexpected

I add more description to the javadoc, but essentially, we don't want to always re-evaluate the rules, so we store the calculated category in that frame.

so if setCategory is only used as cache please move the caching implementation into the getter to avoid the setter. However it feels like the implementation is wrong in means of it will not update the cached value if the options change.

...clipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/StackFramePresentationProvider.java

jukzi · 2025-01-15T07:28:54Z

org.eclipse.jdt.debug/model/org/eclipse/jdt/debug/core/IJavaStackFrame.java

+	 * @param category
+	 *            the new category of the stack frame.
+	 */
+	public void setCategory(Category category);


can it somehow avoided to have such setter? it is not obvious to me if such setter should fireChangeEvent(DebugEvent.STATE);

Somehow we need to store the information how that frame was categorized - even if the process is not slow, it needs to lookup the source, and check which classpath entry contains that, etc.
In theory, we could have a map from IJavaStackFrame -> Category in some singleton, but it's unclear how we can detect, if that stack frame was not accessible anymore, so maybe a WeakHashMap could work, but it felt too complicated and error prone.
This setter is only called once - when we first want to display the frame, so firing a change event should not needed - at least it seems to work fine without it.

...clipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/StackFramePresentationProvider.java

The rules how the frames are categorized are not editable as of now, just a single action is visible to switch on-off the behaviour, with enough javadocs

gzsombor force-pushed the single-change branch from c4172ac to a1db2c3 Compare January 4, 2025 11:27

gzsombor mentioned this pull request Jan 4, 2025

Add coloring to the stack frames of the debug thread view #81

Open

SougandhS suggested changes Jan 14, 2025

View reviewed changes

SougandhS reviewed Jan 14, 2025

View reviewed changes

SougandhS suggested changes Jan 15, 2025

View reviewed changes

org.eclipse.jdt.debug.ui/plugin.xml Outdated Show resolved Hide resolved

gzsombor force-pushed the single-change branch from 7d7e88a to 3eed15a Compare January 15, 2025 06:46

SougandhS approved these changes Jan 15, 2025

View reviewed changes

jukzi added the enhancement New feature or request label Jan 15, 2025

jukzi reviewed Jan 15, 2025

View reviewed changes

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

jukzi reviewed Jan 15, 2025

View reviewed changes

...clipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/StackFramePresentationProvider.java Show resolved Hide resolved

Implement grouping of the stack frames

2fcfe4c

The rules how the frames are categorized are not editable as of now, just a single action is visible to switch on-off the behaviour, with enough javadocs

gzsombor force-pushed the single-change branch from 3eed15a to 2fcfe4c Compare January 15, 2025 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement grouping of the stack frames #583

Implement grouping of the stack frames #583

gzsombor commented Jan 4, 2025 •

edited

Loading

gzsombor commented Jan 14, 2025

jukzi commented Jan 14, 2025

SougandhS commented Jan 14, 2025

SougandhS Jan 14, 2025

gzsombor Jan 14, 2025

SougandhS Jan 14, 2025

gzsombor Jan 14, 2025

SougandhS commented Jan 14, 2025 •

edited

Loading

SougandhS commented Jan 14, 2025 •

edited

Loading

gzsombor commented Jan 14, 2025

SougandhS commented Jan 15, 2025

SougandhS commented Jan 15, 2025 •

edited

Loading

SougandhS left a comment

jukzi commented Jan 15, 2025

jukzi Jan 15, 2025

gzsombor Jan 15, 2025

jukzi Jan 15, 2025

jukzi Jan 15, 2025

gzsombor Jan 15, 2025

Implement grouping of the stack frames #583

Are you sure you want to change the base?

Implement grouping of the stack frames #583

Conversation

gzsombor commented Jan 4, 2025 • edited Loading

What it does

How to test

Author checklist

gzsombor commented Jan 14, 2025

jukzi commented Jan 14, 2025

SougandhS commented Jan 14, 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

SougandhS commented Jan 14, 2025 • edited Loading

SougandhS commented Jan 14, 2025 • edited Loading

gzsombor commented Jan 14, 2025

SougandhS commented Jan 15, 2025

SougandhS commented Jan 15, 2025 • edited Loading

SougandhS left a comment

Choose a reason for hiding this comment

jukzi commented Jan 15, 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

gzsombor commented Jan 4, 2025 •

edited

Loading

SougandhS commented Jan 14, 2025 •

edited

Loading

SougandhS commented Jan 14, 2025 •

edited

Loading

SougandhS commented Jan 15, 2025 •

edited

Loading