Community
Participate
Working Groups
1) Create a breakpoint 2) Open breakpoints view 3) Select the breakpoint and delete it from the view 4) Create another breakpoint in the same location 5) In the view, Ctrl-Z to undo Result, there are two duplicate breakpoints. When adding breakpoints back with undo, we can try to remove duplicates the same way we do with refactor/import.
recreatable with 4.3M6
Created attachment 245378 [details] Handle Duplicate Breakpoints This patch adds a check to make sure that there is no matching breakpoint marker at the same line before adding a new one
(In reply to Sarika Sinha from comment #2) > Created attachment 245378 [details] > Handle Duplicate Breakpoints > > This patch adds a check to make sure that there is no matching breakpoint > marker at the same line before adding a new one I think to be a bit more sure we have a dupe, we would need to compare more than just the line number and resource - we should probably compare elements from the bp marker attribute mapping as well. While testing it seems a dead breakpoint is (still?) put back: 1. new workspace / java project / class 2. create class load BP and method BP, open bp view 3. select + delete the method bp -> undo 4. notice the method bp is put back, but without entry / exit, etc attributes. Also note you cannot toggle the bp from the editor, and it does not appear again in the bp view.
(In reply to Michael Rennie from comment #3) > (In reply to Sarika Sinha from comment #2) > > Created attachment 245378 [details] > > Handle Duplicate Breakpoints > > > > This patch adds a check to make sure that there is no matching breakpoint > > marker at the same line before adding a new one > > I think to be a bit more sure we have a dupe, we would need to compare more > than just the line number and resource - we should probably compare elements > from the bp marker attribute mapping as well. > > While testing it seems a dead breakpoint is (still?) put back: > > 1. new workspace / java project / class > 2. create class load BP and method BP, open bp view > 3. select + delete the method bp -> undo > 4. notice the method bp is put back, but without entry / exit, etc > attributes. Also note you cannot toggle the bp from the editor, and it does > not appear again in the bp view. Step 4 . I was able to toggle and it was added in the bp view if we just foloow the above steps. But yes there are some other undo cases where the bp is shown only in the editor and not the bp view. But that is another bug where undo is not working properly. Also If I compare existing.getAttributes().equal(newMarker.getAttributes()) It fails. We can compare the CharStart and Char End along with line no and resource check.
It fixes exactly the reported steps but as soon as I e.g. - delete from view - Ctrl+Z - Ctrl+Y (redo) - Ctrl+Z ==> breakpoint comes back in the editor but not in the Breakpoints view. Note that this is not due to your patch, but it would be good to have a fix that covers the undo/redo scenario as a whole.
(In reply to Dani Megert from comment #5) > It fixes exactly the reported steps but as soon as I e.g. > > - delete from view > - Ctrl+Z > - Ctrl+Y (redo) > - Ctrl+Z > > ==> breakpoint comes back in the editor but not in the Breakpoints view. > > Note that this is not due to your patch, but it would be good to have a fix > that covers the undo/redo scenario as a whole. Yes there are issues with redo and we have another bug for it and the solutions are totally independent so I think we should go ahead and fix this at least. Mostly the other solution will be in JDT Debug component.
(In reply to Sarika Sinha from comment #6) > (In reply to Dani Megert from comment #5) > > It fixes exactly the reported steps but as soon as I e.g. > > > > - delete from view > > - Ctrl+Z > > - Ctrl+Y (redo) > > - Ctrl+Z > > > > ==> breakpoint comes back in the editor but not in the Breakpoints view. > > > > Note that this is not due to your patch, but it would be good to have a fix > > that covers the undo/redo scenario as a whole. > > Yes there are issues with redo and we have another bug for it Do you mean bug 424561? > and the solutions are totally independent so I think we should go ahead and > fix this at least. OK, let's move on with this bug here, given we have a bug for the other scenario.
Comment on attachment 245378 [details] Handle Duplicate Breakpoints The fix works as already outlined in my previous comment. Some minor issues that need to be addressed: - breakpoint.getMarker() can return 'null' and hence a cause a potential NPE - the new method can return 'null', hence this must be specified in the Javadoc - I would rename the method to findMatchingBreakpoint - write English sentences, e.g. "line number" and not "line no" ;-) - use a traditional comment if the comment is more than one line
Created attachment 247750 [details] Handle Duplicate Breakpoints Updated the patch with review comments. Bug 424561 and Bug 404990 describes the similar undo scenario.
(In reply to Dani Megert from comment #8) > - write English sentences, e.g. "line number" and not "line no" ;-) This wasn't done in the new methods Javadoc. Fixed this. Also, please put primitive values inside a code tag, e.g. <code>null</code>. Done that too. Committed your patch with http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=8a1df72b32d13765ec6e719f211a79d27a86667c
Oops, I missed another potential NPE in this code: (Integer) bpMarker.getAttribute(IMarker.LINE_NUMBER) == (line == null ? -1 : line.intValue()) If the attribute is null it will end up in (Integer)null == -1 which will fail. Fixed with http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=564d3bfae3140780ca5879d4cbbe3601cf6916d2
Verified using Eclipse SDK Version: Mars (4.5) Build id: I20141014-0800
