Debugger: Fix removal of breakpoint marks in disassembler view

Task-number: QTCREATORBUG-14973
Change-Id: I6dfceefe6d8b1404c2a553dbeebd7dccefa1a624
Reviewed-by: Christian Stenger <christian.stenger@theqtcompany.com>

src/plugins/debugger/breakhandler.cpp

@@ -104,8 +104,6 @@ public:
     void changeLineNumberFromMarker(int lineNumber);
     bool isLocatedAt(const QString &fileName, int lineNumber, bool useMarkerPosition) const;
 
-    bool needsChildren() const;
-
     void setMarkerFileAndLine(const QString &fileName, int lineNumber);
 
     void insertSubBreakpoint(const BreakpointResponse &params);
@@ -808,7 +806,8 @@ void Breakpoint::removeAlienBreakpoint()
 
 void Breakpoint::removeBreakpoint() const
 {
-    b->removeBreakpoint();
+    if (b)
+        b->removeBreakpoint();
 }
 
 Breakpoint::Breakpoint(BreakpointItem *b)
@@ -834,11 +833,6 @@ void Breakpoint::setMarkerFileAndLine(const QString &fileName, int lineNumber)
         b->setMarkerFileAndLine(fileName, lineNumber);
 }
 
-bool BreakpointItem::needsChildren() const
-{
-    return m_response.multiple && rowCount() == 0;
-}
-
 void Breakpoint::setTracepoint(bool on)
 {
     if (b->m_params.tracepoint == on)
@@ -971,6 +965,8 @@ void Breakpoint::notifyBreakpointInsertProceeding()
 void Breakpoint::notifyBreakpointInsertOk()
 {
     gotoState(BreakpointInserted, BreakpointInsertProceeding);
+    if (b->m_engine)
+        b->m_engine->updateBreakpointMarker(*this);
 }
 
 void Breakpoint::notifyBreakpointInsertFailed()
@@ -987,6 +983,8 @@ void Breakpoint::notifyBreakpointRemoveOk()
 {
     QTC_ASSERT(b, return);
     QTC_ASSERT(b->m_state == BreakpointRemoveProceeding, qDebug() << b->m_state);
+    if (b->m_engine)
+        b->m_engine->removeBreakpointMarker(*this);
     b->deleteThis();
 }
 
@@ -994,6 +992,8 @@ void Breakpoint::notifyBreakpointRemoveFailed()
 {
     QTC_ASSERT(b, return);
     QTC_ASSERT(b->m_state == BreakpointRemoveProceeding, qDebug() << b->m_state);
+    if (b->m_engine)
+        b->m_engine->removeBreakpointMarker(*this);
     b->deleteThis();
 }
 
@@ -1301,6 +1301,8 @@ void Breakpoint::changeBreakpointData(const BreakpointParameters &params)
     if (params == b->m_params)
         return;
     b->m_params = params;
+    if (b->m_engine)
+        b->m_engine->updateBreakpointMarker(*this);
     b->destroyMarker();
     b->updateMarker();
     b->update();
@@ -1331,8 +1333,6 @@ BreakpointItem::~BreakpointItem()
 
 void BreakpointItem::destroyMarker()
 {
-    if (m_engine)
-        m_engine->updateBreakpointMarkers();
     if (m_marker) {
         BreakpointMarker *m = m_marker;
         m->m_bp = 0;

src/plugins/debugger/debuggerengine.cpp

@@ -1339,9 +1339,14 @@ QString DebuggerEngine::toFileInProject(const QUrl &fileUrl)
     return d->m_fileFinder.findFile(fileUrl);
 }
 
-void DebuggerEngine::updateBreakpointMarkers()
+void DebuggerEngine::removeBreakpointMarker(const Breakpoint &bp)
 {
-    d->m_disassemblerAgent.updateBreakpointMarkers();
+    d->m_disassemblerAgent.removeBreakpointMarker(bp);
+}
+
+void DebuggerEngine::updateBreakpointMarker(const Breakpoint &bp)
+{
+    d->m_disassemblerAgent.updateBreakpointMarker(bp);
 }
 
 bool DebuggerEngine::debuggerActionsEnabled() const
@@ -1638,7 +1643,6 @@ void DebuggerEngine::attemptBreakpointSynchronization()
 
     if (done) {
         showMessage(_("BREAKPOINTS ARE SYNCHRONIZED"));
-        d->m_disassemblerAgent.updateBreakpointMarkers();
     } else {
         showMessage(_("BREAKPOINTS ARE NOT FULLY SYNCHRONIZED"));
     }

src/plugins/debugger/debuggerengine.h

@@ -320,7 +320,8 @@ public:
     virtual void notifyInferiorIll();
 
     QString toFileInProject(const QUrl &fileUrl);
-    void updateBreakpointMarkers();
+    void updateBreakpointMarker(const Breakpoint &bp);
+    void removeBreakpointMarker(const Breakpoint &bp);
 
 signals:
     void stateChanged(Debugger::DebuggerState state);

src/plugins/debugger/disassembleragent.cpp

@@ -61,6 +61,30 @@ using namespace TextEditor;
 namespace Debugger {
 namespace Internal {
 
+///////////////////////////////////////////////////////////////////////
+//
+// DisassemblerBreakpointMarker
+//
+///////////////////////////////////////////////////////////////////////
+
+// The red blob on the left side in the cpp editor.
+class DisassemblerBreakpointMarker : public TextMark
+{
+public:
+    DisassemblerBreakpointMarker(const Breakpoint &bp, int lineNumber)
+        : TextMark(QString(), lineNumber, Constants::TEXT_MARK_CATEGORY_BREAKPOINT), m_bp(bp)
+    {
+        setIcon(bp.icon());
+        setPriority(TextMark::NormalPriority);
+    }
+
+    bool isClickable() const { return true; }
+    void clicked() { m_bp.removeBreakpoint(); }
+
+public:
+    Breakpoint m_bp;
+};
+
 ///////////////////////////////////////////////////////////////////////
 //
 // FrameKey
@@ -102,14 +126,14 @@ public:
     DisassemblerAgentPrivate(DebuggerEngine *engine);
     ~DisassemblerAgentPrivate();
     void configureMimeType();
-    DisassemblerLines contentsAtCurrentLocation() const;
+    int lineForAddress(quint64 address) const;
 
 public:
     QPointer<TextDocument> document;
     Location location;
     QPointer<DebuggerEngine> engine;
     LocationMark locationMark;
-    QList<TextMark *> breakpointMarks;
+    QList<DisassemblerBreakpointMarker *> breakpointMarks;
     QList<CacheEntry> cache;
     QString mimeType;
     bool resetLocationScheduled;
@@ -130,14 +154,14 @@ DisassemblerAgentPrivate::~DisassemblerAgentPrivate()
     qDeleteAll(breakpointMarks);
 }
 
-DisassemblerLines DisassemblerAgentPrivate::contentsAtCurrentLocation() const
+int DisassemblerAgentPrivate::lineForAddress(quint64 address) const
 {
     for (int i = 0, n = cache.size(); i != n; ++i) {
         const CacheEntry &entry = cache.at(i);
         if (entry.first.matches(location))
-            return entry.second;
+            return entry.second.lineForAddress(address);
     }
-    return DisassemblerLines();
+    return 0;
 }
 
 
@@ -310,15 +334,17 @@ void DisassemblerAgent::setContentsToDocument(const DisassemblerLines &contents)
     d->document->setPreferredDisplayName(_("Disassembler (%1)")
         .arg(d->location.functionName()));
 
-    updateBreakpointMarkers();
+    Breakpoints bps = breakHandler()->engineBreakpoints(d->engine);
+    foreach (Breakpoint bp, bps)
+        updateBreakpointMarker(bp);
+
     updateLocationMarker();
 }
 
 void DisassemblerAgent::updateLocationMarker()
 {
     QTC_ASSERT(d->document, return);
-    const DisassemblerLines contents = d->contentsAtCurrentLocation();
-    int lineNumber = contents.lineForAddress(d->location.address());
+    int lineNumber = d->lineForAddress(d->location.address());
     if (d->location.needsMarker()) {
         d->document->removeMark(&d->locationMark);
         d->locationMark.updateLineNumber(lineNumber);
@@ -331,44 +357,45 @@ void DisassemblerAgent::updateLocationMarker()
             textEditor->gotoLine(lineNumber);
 }
 
-void DisassemblerAgent::updateBreakpointMarkers()
+void DisassemblerAgent::removeBreakpointMarker(const Breakpoint &bp)
 {
     if (!d->document)
         return;
 
-    Breakpoints bps = breakHandler()->engineBreakpoints(d->engine);
-    if (bps.isEmpty())
+    BreakpointModelId id = bp.id();
+    foreach (DisassemblerBreakpointMarker *marker, d->breakpointMarks) {
+        if (marker->m_bp.id() == id) {
+            d->breakpointMarks.removeOne(marker);
+            d->document->removeMark(marker);
+            delete marker;
+            return;
+        }
+    }
+}
+
+void DisassemblerAgent::updateBreakpointMarker(const Breakpoint &bp)
+{
+    removeBreakpointMarker(bp);
+    const quint64 address = bp.response().address;
+    if (!address)
         return;
 
-    const DisassemblerLines contents = d->contentsAtCurrentLocation();
-    foreach (TextMark *marker, d->breakpointMarks)
-        d->document->removeMark(marker);
-    qDeleteAll(d->breakpointMarks);
-    d->breakpointMarks.clear();
-    foreach (Breakpoint bp, bps) {
-        const quint64 address = bp.response().address;
-        if (!address)
-            continue;
-        int lineNumber = contents.lineForAddress(address);
-        if (!lineNumber)
-            continue;
-
-        // HACK: If it's a FileAndLine breakpoint, and there's a source line
-        // above, move the marker up there. That allows setting and removing
-        // normal breakpoints from within the disassembler view.
-        if (bp.type() == BreakpointByFileAndLine) {
-            ContextData context = getLocationContext(d->document, lineNumber - 1);
-            if (context.type == LocationByFile)
-                --lineNumber;
-        }
+    int lineNumber = d->lineForAddress(address);
+    if (!lineNumber)
+        return;
 
-        TextMark *marker = new TextMark(QString(), lineNumber,
-                                        Constants::TEXT_MARK_CATEGORY_BREAKPOINT);
-        marker->setIcon(bp.icon());
-        marker->setPriority(TextMark::NormalPriority);
-        d->breakpointMarks.append(marker);
-        d->document->addMark(marker);
+    // HACK: If it's a FileAndLine breakpoint, and there's a source line
+    // above, move the marker up there. That allows setting and removing
+    // normal breakpoints from within the disassembler view.
+    if (bp.type() == BreakpointByFileAndLine) {
+        ContextData context = getLocationContext(d->document, lineNumber - 1);
+        if (context.type == LocationByFile)
+            --lineNumber;
     }
+
+    auto marker = new DisassemblerBreakpointMarker(bp, lineNumber);
+    d->breakpointMarks.append(marker);
+    d->document->addMark(marker);
 }
 
 quint64 DisassemblerAgent::address() const

src/plugins/debugger/disassembleragent.h

@@ -36,6 +36,7 @@
 namespace Debugger {
 namespace Internal {
 
+class Breakpoint;
 class DebuggerEngine;
 class DisassemblerAgentPrivate;
 class DisassemblerLines;
@@ -56,7 +57,8 @@ public:
     void resetLocation();
     void setContents(const DisassemblerLines &contents);
     void updateLocationMarker();
-    void updateBreakpointMarkers();
+    void updateBreakpointMarker(const Breakpoint &bp);
+    void removeBreakpointMarker(const Breakpoint &bp);
 
     // Mimetype: "text/a-asm" or some specialized architecture
     QString mimeType() const;