Bug 197707

Created attachment 114057 [details]
The API change which is necessary to implement this.

Please review the attached simple API change that adds the ability to enable/disable breakpoints when one is hit. An implementation usage of this API follows.
I'll add a test as well.

[JG01] enableDisableDependentBreakpoints does not do null checks, and it is not clear what null means in this case. Probably breakpointsToEnable and breakpointsToDisable and the corresponding getters and setters should be @NonNull, with Collections.emptySet() as the default value.


[JG02] Seems the new methods should be added to org.netbeans.api.debugger.Breakpoint, as nothing about them seems specific to JPDA and they might be useful for non-JVM debuggers as well.

[JG01] O.K. I'll make the methods @NonNull, it will be safer to use them.

[JG02] The functionality behind these methods rely on the implementation. I was thinking to add them to the Breakpoint class, but using these methods will have no effect until the appropriate logic is written into the implementation. This is why I've added them to JPDABreakpoint only, where we can guarantee the functionality.
We can move it into Breakpoint class and add a method like canHaveDependentBreakpoints() returning false by default and make the methods to throw IllegalStateException when canHaveDependentBreakpoints() is false.
Originally I thought that it's best when every Breakpoint implementation class adds this functionality as appropriate, but perhaps having it directly on the Breakpoint class allows to hide the implementation classes, which might be better and more universal.

(In reply to comment #4)
> I was
> thinking to add them to the Breakpoint class, but using these methods will have
> no effect until the appropriate logic is written into the implementation.

Of course; but the Javadoc could simply say that a given debugger implementation may or may not honor dependent breakpoints.

Created attachment 114105 [details]
A modified API change.

I've modified the patch by adding the methods directly to the Breakpoint class.
Also I've introduced a method canHaveDependentBreakpoints() which is a clean way how to determine if the new methods actually change the behavior or not.

[JG03] Then api.debugger.jpda and debugger.jpda should require 1.34 spec version of api.debugger.


BTW "Can not have..." should generally be "Cannot have...". (The latter means "it is impossible to have..." which is what you want; the former is ambiguous in writing, possibly meaning "it is possible to not have..." if "not" is stressed in speech or italicized in writing.)

Y01 I don't understand why you need "setBreakpointsToEnable" and "setBreakpointsToDisable"! The only use is in JPDA breakpoint class and it is restricted only to BreakpointsFromGroup. It will be much easier if the implementation just sets the name of the group to the JPDA breakpoint and the breakpoint, when asked for getBreakpointsToE/D would read the breakpoints from the group. Remove the setters from where they don't belong.

Y02 As you don't need setters, you can probably also remove "canHaveDependentBreakpoints" method.

Created attachment 114219 [details]
The updated API change with usage from UI module.

[JG03] Thanks, I've updated the version dependency.
       Also thanks for the language correction.

[Y01] I did not provide the associated change in the UI module as I've been working on them. The setter methods are called from the debugger.jpda.ui module, where the user specifies the breakpoint group. Also, I've designed the API to be more generic intentionally. The original idea was to use just a group name, but I think it's valuable to make use of the other grouping we have (by files, projects, breakpoint types, etc.) This is why the API uses just Set<Breakpoint> to be generic enough.

I'm attaching the API change with all associated UI changes.

Thanks for the review, I plan to integrate the change today COB.

Pushed as changeset:   209146:b22274554ce8
http://hg.netbeans.org/main/rev/b22274554ce8

Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/b22274554ce8
User: mentlicher@netbeans.org
Log: #197707: Dependent breakpoints introduced. Methods to get/set breakpoints to enable/disable others were added to the Breakpoint class. It's used by JPDA breakpoints.