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.

The setter was there, because all the settings were managed by JDIDebugUIPlugin, not by JDIDebugPlugin, partially because managing class/package filter lists are a bit easier with org.eclipse.jface.preference.IPreferenceStore and org.eclipse.jdt.internal.ui.filtertable.FilterManager - those are UI classes, not reachable from the backend.
But I could rewrote it, the base functionality seems to be working - I still have to refactor the settings page and the couple of tests, but that will come in the other PR.

...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

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

jukzi · 2025-01-17T08:21:50Z

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

+	 * @since 3.22
+	 *
+	 */
+	enum Category {


It feels wrong to publish this enum as API as it may be subject to change. Especially "platform" may be usefull but has no clear definition. Also those categories may not be disjunct. code could be synthetic platform test and library at once. One could either use plain Strings as Category or have a CategoryRegistry where new Categories could be added at runtime.

Yes, the categories are not exact and there could be frames that could belong to multiple categories.
However, I don't think that adding new categories is a feasible - and useful feature ever. Main problem is that a single category needs to be handled in multiple places and going for a very general solution would be hard. For example, to have a foreground and background colors we have to declare them in the plugin.xml, otherwise it can't be set in the color editing preference page.
Maybe, we could have Category as a 'record' and have them declared as a fixed constant in the JDIDebugPlugin.

As long as the solution does not make the categories public API, it may be ok.

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

SougandhS · 2025-01-21T01:23:02Z

Hi @gzsombor, pls squash all commits into one

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 · 2025-01-21T08:07:03Z

Hi @gzsombor, pls squash all commits into one

Done and also the build is green now!

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 Outdated Show resolved Hide resolved

gzsombor force-pushed the single-change branch 3 times, most recently from 9eb4199 to 84bc37e Compare January 17, 2025 07:48

jukzi reviewed Jan 17, 2025

View reviewed changes

...se.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/monitors/JavaThreadContentProvider.java Outdated Show resolved Hide resolved

jukzi reviewed Jan 17, 2025

View reviewed changes

...se.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/monitors/JavaThreadContentProvider.java Outdated Show resolved Hide resolved

jukzi reviewed Jan 17, 2025

View reviewed changes

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

gzsombor force-pushed the single-change branch 2 times, most recently from 5154e9e to b62f599 Compare January 20, 2025 17:39

Implement grouping of the stack frames

25f285a

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 b62f599 to 25f285a Compare January 21, 2025 06:50

Turn Category into a record

5fb4500

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

gzsombor Jan 17, 2025

jukzi Jan 15, 2025

gzsombor Jan 15, 2025

jukzi Jan 17, 2025

gzsombor Jan 18, 2025

jukzi Jan 21, 2025

SougandhS commented Jan 21, 2025

gzsombor commented Jan 21, 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

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 21, 2025

gzsombor commented Jan 21, 2025

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