Clean up BreakPoint

Reviewed-by: Friedemann Kleint
Reviewed-by: hjk

src/plugins/debugger/breakhandler.cpp

@@ -28,6 +28,7 @@
 **************************************************************************/
 
 #include "breakhandler.h"
+#include "breakpointmarker.h"
 
 #include "debuggeractions.h"
 #include "debuggerengine.h"
@@ -52,6 +53,8 @@ static DebuggerPlugin *plugin() { return DebuggerPlugin::instance(); }
 
 
 static Breakpoints m_bp;
+// FIXME this is only static because it might map to bps in the above list
+static QHash<quint64,BreakpointMarker*> m_markers;
 
 //////////////////////////////////////////////////////////////////
 //
@@ -108,6 +111,7 @@ void BreakHandler::removeAt(int index)
     BreakpointData *data = at(index);
     QTC_ASSERT(data, return);
     m_bp.removeAt(index);
+    delete m_markers.take(data->id);
     delete data;
 }
 
@@ -242,10 +246,33 @@ void BreakHandler::loadBreakpoints()
 void BreakHandler::updateMarkers()
 {
     for (int index = 0; index != size(); ++index)
-        at(index)->updateMarker();
+        updateMarker(at(index));
     emit layoutChanged();
 }
 
+void BreakHandler::updateMarker(BreakpointData * bp)
+{
+    BreakpointMarker *marker = m_markers.value(bp->id);
+
+    if (marker && (bp->m_markerFileName != marker->fileName()
+                || bp->m_markerLineNumber != marker->lineNumber())) {
+        removeMarker(bp);
+        marker = 0;
+    }
+
+    if (!marker && !bp->m_markerFileName.isEmpty() && bp->m_markerLineNumber > 0) {
+        marker = new BreakpointMarker(this, bp, bp->m_markerFileName, bp->m_markerLineNumber);
+        m_markers.insert(bp->id, marker);
+    }
+
+    if (marker)
+        marker->setPending(bp->pending);
+}
+void BreakHandler::removeMarker(BreakpointData * bp)
+{
+    delete m_markers.take(bp->id);
+}
+
 QVariant BreakHandler::headerData(int section,
     Qt::Orientation orientation, int role) const
 {
@@ -517,12 +544,14 @@ void BreakHandler::reinsertBreakpoint(BreakpointData *data)
 
 void BreakHandler::append(BreakpointData *data)
 {
-    data->m_handler = this;
     m_bp.append(data);
 }
 
 Breakpoints BreakHandler::takeRemovedBreakpoints()
 {
+    foreach(BreakpointData *bp, m_removed) {
+        removeMarker(bp);
+    }
     Breakpoints result = m_removed;
     m_removed.clear();
     return result;
@@ -547,7 +576,7 @@ void BreakHandler::removeBreakpointHelper(int index)
     BreakpointData *data = at(index);
     QTC_ASSERT(data, return);
     m_bp.removeAt(index);
-    data->removeMarker();
+    removeMarker(data);
     m_removed.append(data);
 }
 
@@ -576,8 +605,8 @@ void BreakHandler::toggleBreakpointEnabled(BreakpointData *data)
         m_enabled.removeAll(data);
         m_disabled.append(data);
     }
-    data->removeMarker(); // Force icon update.
-    data->updateMarker();
+    removeMarker(data); // Force icon update.
+    updateMarker(data);
     emit layoutChanged();
     m_engine->attemptBreakpointSynchronization();
 }
@@ -661,6 +690,9 @@ void BreakHandler::saveSessionData()
 void BreakHandler::loadSessionData()
 {
     QTC_ASSERT(m_engine->isSessionEngine(), return);
+    foreach(BreakpointData *bp, m_bp) {
+        delete m_markers.take(bp->id);
+    }
     qDeleteAll(m_bp);
     m_bp.clear();
     loadBreakpoints();
@@ -696,7 +728,9 @@ void BreakHandler::initializeFromTemplate(BreakHandler *other)
     m_inserted.clear();
     foreach(BreakpointData *data, m_bp) {
         if (m_engine->acceptsBreakpoint(data)) {
-            data->m_handler = this;
+            BreakpointMarker *marker = m_markers.value(data->id);
+            if (marker)
+                marker->m_handler = this;
             m_inserted.append(data);
         }
     }
@@ -705,7 +739,9 @@ void BreakHandler::initializeFromTemplate(BreakHandler *other)
 void BreakHandler::storeToTemplate(BreakHandler *other)
 {
     foreach (BreakpointData *data, m_bp) {
-        data->m_handler = other;
+        BreakpointMarker *marker = m_markers.value(data->id);
+        if (marker)
+            marker->m_handler = other;
         data->clear();
     }
     other->saveSessionData();

src/plugins/debugger/breakhandler.h

@@ -75,6 +75,8 @@ public:
     int findWatchPointIndexByAddress(quint64 address) const;
     bool watchPointAt(quint64 address) const;
     void updateMarkers();
+    void updateMarker(BreakpointData *);
+    void removeMarker(BreakpointData *);
     bool isActive() const;
 
     Breakpoints takeRemovedBreakpoints(); // Owned.

src/plugins/debugger/breakpoint.cpp

@@ -27,13 +27,9 @@
 **
 **************************************************************************/
 
+#include "breakpoint.h"
 #include "breakhandler.h"
 
-#include "debuggeractions.h"
-#include "debuggerengine.h"
-#include "debuggerstringutils.h"
-#include "threadshandler.h"
-#include "stackhandler.h"
 #include "stackframe.h"
 
 #include <texteditor/basetextmark.h>
@@ -45,111 +41,9 @@
 #include <QtCore/QFileInfo>
 #include <QtCore/QDir>
 
-//////////////////////////////////////////////////////////////////
-//
-// BreakpointMarker
-//
-//////////////////////////////////////////////////////////////////
-
-// Compare file names case insensitively on Windows.
-static inline bool fileNameMatch(const QString &f1, const QString &f2)
-{
-#ifdef Q_OS_WIN
-    return f1.compare(f2, Qt::CaseInsensitive) == 0;
-#else
-    return f1 == f2;
-#endif
-}
-
 namespace Debugger {
 namespace Internal {
 
-// The red blob on the left side in the cpp editor.
-class BreakpointMarker : public TextEditor::BaseTextMark
-{
-    Q_OBJECT
-public:
-    BreakpointMarker(BreakpointData *data, const QString &fileName, int lineNumber)
-      : BaseTextMark(fileName, lineNumber)
-    {
-        m_data = data;
-        m_pending = true;
-        //qDebug() << "CREATE MARKER " << fileName << lineNumber;
-    }
-
-    ~BreakpointMarker()
-    {
-        //qDebug() << "REMOVE MARKER ";
-        m_data = 0;
-    }
-
-    QIcon icon() const
-    {
-        const BreakHandler *handler = m_data->handler();
-        if (!m_data->enabled)
-            return handler->disabledBreakpointIcon();
-        if (!handler->isActive())
-            return handler->emptyIcon();
-        return m_pending ? handler->pendingBreakPointIcon() : handler->breakpointIcon();
-    }
-
-    void setPending(bool pending)
-    {
-        if (pending == m_pending)
-            return;
-        m_pending = pending;
-        updateMarker();
-    }
-
-    void updateBlock(const QTextBlock &)
-    {
-        //qDebug() << "BREAKPOINT MARKER UPDATE BLOCK";
-    }
-
-    void removedFromEditor()
-    {
-        if (!m_data)
-            return;
-
-        BreakHandler *handler = m_data->handler();
-        handler->removeBreakpoint(m_data);
-        //handler->saveBreakpoints();
-        handler->updateMarkers();
-    }
-
-    void updateLineNumber(int lineNumber)
-    {
-        if (!m_data)
-            return;
-        //if (m_data->markerLineNumber == lineNumber)
-        //    return;
-        if (m_data->markerLineNumber() != lineNumber) {
-            m_data->setMarkerLineNumber(lineNumber);
-            // FIXME: Should we tell gdb about the change?
-            // Ignore it for now, as we would require re-compilation
-            // and debugger re-start anyway.
-            if (0 && m_data->bpLineNumber) {
-                if (!m_data->bpNumber.trimmed().isEmpty()) {
-                    m_data->pending = true;
-                }
-            }
-        }
-        // Ignore updates to the "real" line number while the debugger is
-        // running, as this can be triggered by moving the breakpoint to
-        // the next line that generated code.
-        // FIXME: Do we need yet another data member?
-        if (m_data->bpNumber.trimmed().isEmpty()) {
-            m_data->lineNumber = lineNumber;
-            m_data->handler()->updateMarkers();
-        }
-    }
-
-private:
-    BreakpointData *m_data;
-    bool m_pending;
-};
-
-
 //////////////////////////////////////////////////////////////////
 //
 // BreakpointData
@@ -159,22 +53,30 @@ private:
 const char *BreakpointData::throwFunction = "throw";
 const char *BreakpointData::catchFunction = "catch";
 
+static quint64 nextBPId() {
+    /* ok to be not thread-safe
+       the order does not matter and only the gui
+       produces authoritative ids. well, for now...
+    */
+    static quint64 i=1;
+    return ++i;
+}
+
 BreakpointData::BreakpointData() :
-    m_handler(0), enabled(true),
+    id(nextBPId()), enabled(true),
     pending(true), type(BreakpointType),
     ignoreCount(0), lineNumber(0), address(0),
     useFullPath(false),
     bpIgnoreCount(0), bpLineNumber(0),
     bpCorrectedLineNumber(0), bpAddress(0),
     bpMultiple(false), bpEnabled(true),
-    m_markerLineNumber(0), marker(0)
+    m_markerLineNumber(0)
 {
 }
 
 BreakpointData *BreakpointData::clone() const
 {
     BreakpointData *data = new BreakpointData();
-    data->m_handler = m_handler;
     data->enabled = enabled;
     data->type = type;
     data->fileName = fileName;
@@ -199,7 +101,6 @@ BreakpointData *BreakpointData::clone() const
 
 BreakpointData::~BreakpointData()
 {
-    removeMarker();
 }
 
 void BreakpointData::clear()
@@ -220,27 +121,6 @@ void BreakpointData::clear()
     bpState.clear();
     m_markerFileName = fileName;
     m_markerLineNumber = lineNumber;
-    updateMarker();
-}
-
-void BreakpointData::removeMarker()
-{
-    BreakpointMarker *m = marker;
-    marker = 0;
-    delete m;
-}
-
-void BreakpointData::updateMarker()
-{
-    if (marker && (m_markerFileName != marker->fileName()
-            || m_markerLineNumber != marker->lineNumber()))
-        removeMarker();
-
-    if (!marker && !m_markerFileName.isEmpty() && m_markerLineNumber > 0)
-        marker = new BreakpointMarker(this, m_markerFileName, m_markerLineNumber);
-
-    if (marker)
-        marker->setPending(pending);
 }
 
 void BreakpointData::setMarkerFileName(const QString &fileName)
@@ -268,44 +148,44 @@ QString BreakpointData::toToolTip() const
     QString rc;
     QTextStream str(&rc);
     str << "<html><body><table>"
-        << "<tr><td>" << BreakHandler::tr("Marker File:")
+        << "<tr><td>" << tr("Marker File:")
         << "</td><td>" << QDir::toNativeSeparators(m_markerFileName) << "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("Marker Line:")
+        << "<tr><td>" << tr("Marker Line:")
         << "</td><td>" << m_markerLineNumber << "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("Breakpoint Number:")
+        << "<tr><td>" << tr("Breakpoint Number:")
         << "</td><td>" << bpNumber << "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("Breakpoint Type:")
+        << "<tr><td>" << tr("Breakpoint Type:")
         << "</td><td>"
-        << (type == BreakpointType ? BreakHandler::tr("Breakpoint")
-            : type == WatchpointType ? BreakHandler::tr("Watchpoint")
-            : BreakHandler::tr("Unknown breakpoint type"))
+        << (type == BreakpointType ? tr("Breakpoint")
+            : type == WatchpointType ? tr("Watchpoint")
+            : tr("Unknown breakpoint type"))
         << "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("State:")
+        << "<tr><td>" << tr("State:")
         << "</td><td>" << bpState << "</td></tr>"
         << "</table><br><hr><table>"
-        << "<tr><th>" << BreakHandler::tr("Property")
-        << "</th><th>" << BreakHandler::tr("Requested")
-        << "</th><th>" << BreakHandler::tr("Obtained") << "</th></tr>"
-        << "<tr><td>" << BreakHandler::tr("Internal Number:")
+        << "<tr><th>" << tr("Property")
+        << "</th><th>" << tr("Requested")
+        << "</th><th>" << tr("Obtained") << "</th></tr>"
+        << "<tr><td>" << tr("Internal Number:")
         << "</td><td>&mdash;</td><td>" << bpNumber << "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("File Name:")
+        << "<tr><td>" << tr("File Name:")
         << "</td><td>" << QDir::toNativeSeparators(fileName) << "</td><td>" << QDir::toNativeSeparators(bpFileName) << "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("Function Name:")
+        << "<tr><td>" << tr("Function Name:")
         << "</td><td>" << funcName << "</td><td>" << bpFuncName << "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("Line Number:") << "</td><td>";
+        << "<tr><td>" << tr("Line Number:") << "</td><td>";
     if (lineNumber)
         str << lineNumber;
     str << "</td><td>";
     if (bpLineNumber)
         str << bpLineNumber;
     str << "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("Breakpoint Address:")
+        << "<tr><td>" << tr("Breakpoint Address:")
         << "</td><td>";
     formatAddress(str, address);
     str << "</td><td>";
     formatAddress(str, bpAddress);
     str <<  "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("Corrected Line Number:")
+        << "<tr><td>" << tr("Corrected Line Number:")
         << "</td><td>-</td><td>";
     if (bpCorrectedLineNumber > 0) {
         str << bpCorrectedLineNumber;
@@ -313,21 +193,31 @@ QString BreakpointData::toToolTip() const
         str << '-';
     }
     str << "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("Condition:")
+        << "<tr><td>" << tr("Condition:")
         << "</td><td>" << condition << "</td><td>" << bpCondition << "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("Ignore Count:") << "</td><td>";
+        << "<tr><td>" << tr("Ignore Count:") << "</td><td>";
     if (ignoreCount)
         str << ignoreCount;
     str << "</td><td>";
     if (bpIgnoreCount)
         str << bpIgnoreCount;
     str << "</td></tr>"
-        << "<tr><td>" << BreakHandler::tr("Thread Specification:")
+        << "<tr><td>" << tr("Thread Specification:")
         << "</td><td>" << threadSpec << "</td><td>" << bpThreadSpec << "</td></tr>"
         << "</table></body></html>";
     return rc;
 }
 
+// Compare file names case insensitively on Windows.
+static inline bool fileNameMatch(const QString &f1, const QString &f2)
+{
+#ifdef Q_OS_WIN
+    return f1.compare(f2, Qt::CaseInsensitive) == 0;
+#else
+    return f1 == f2;
+#endif
+}
+
 bool BreakpointData::isLocatedAt(const QString &fileName_, int lineNumber_, 
     bool useMarkerPosition) const
 {
@@ -339,6 +229,9 @@ bool BreakpointData::isSimilarTo(const BreakpointData *needle) const
 {
     //qDebug() << "COMPARING " << toString() << " WITH " << needle->toString();
 
+    if (id == needle->id && id != 0)
+        return true;
+
     // Clear hit.
     if (bpNumber == needle->bpNumber
             && !bpNumber.isEmpty()
@@ -385,13 +278,6 @@ bool BreakpointData::conditionsMatch() const
     return s1 == s2;
 }
 
-void BreakpointData::reinsertBreakpoint()
-{
-    QTC_ASSERT(m_handler, return);
-    m_handler->reinsertBreakpoint(this);
-}
-
 } // namespace Internal
 } // namespace Debugger
 
-#include "breakpoint.moc"

src/plugins/debugger/breakpoint.h

@@ -33,6 +33,7 @@
 #include <QtCore/QMetaType>
 #include <QtCore/QList>
 #include <QtCore/QString>
+#include <QtCore/QCoreApplication>
 
 namespace Debugger {
 namespace Internal {
@@ -52,11 +53,7 @@ public:
     BreakpointData();
     ~BreakpointData();
 
-    void removeMarker();
-    void updateMarker();
     QString toToolTip() const;
-    BreakHandler *handler() { return m_handler; }
-    void reinsertBreakpoint();
     void clear(); // Delete all generated data.
 
     bool isLocatedAt(const QString &fileName, int lineNumber,
@@ -77,13 +74,13 @@ private:
     BreakpointData(const BreakpointData &);
     void operator=(const BreakpointData &);
 
-    // Our owner
-    BreakHandler *m_handler; // Not owned.
     friend class BreakHandler;
 
 public:
     enum Type { BreakpointType, WatchpointType };
 
+    quint64 id;
+
     bool enabled;            // Should we talk to the debugger engine?
     bool pending;            // Does the debugger engine know about us already?
     Type type;               // Type of breakpoint.
@@ -124,13 +121,12 @@ public:
     bool isSetByFunction() const { return !funcName.isEmpty(); }
     bool isSetByFileAndLine() const { return !fileName.isEmpty(); }
 
-private:
+    Q_DECLARE_TR_FUNCTIONS(BreakHandler)
+
+    // TODO: move those to breakhandler
     // Taken from either user input or gdb responses.
     QString m_markerFileName; // Used to locate the marker.
     int m_markerLineNumber;
-
-    // Our red blob in the editor.
-    BreakpointMarker *marker;
 };
 
 typedef QList<BreakpointData *> Breakpoints;

src/plugins/debugger/breakpointmarker.cpp

+/**************************************************************************
+**
+** This file is part of Qt Creator
+**
+** Copyright (c) 2010 Nokia Corporation and/or its subsidiary(-ies).
+**
+** Contact: Nokia Corporation (qt-info@nokia.com)
+**
+** Commercial Usage
+**
+** Licensees holding valid Qt Commercial licenses may use this file in
+** accordance with the Qt Commercial License Agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and Nokia.
+**
+** GNU Lesser General Public License Usage
+**
+** Alternatively, this file may be used under the terms of the GNU Lesser
+** General Public License version 2.1 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPL included in the
+** packaging of this file.  Please review the following information to
+** ensure the GNU Lesser General Public License version 2.1 requirements
+** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
+**
+** If you are unsure which license is appropriate for your use, please
+** contact the sales department at http://qt.nokia.com/contact.
+**
+**************************************************************************/
+
+#include "breakpointmarker.h"
+#include "breakhandler.h"
+
+#include "stackframe.h"
+
+#include <texteditor/basetextmark.h>
+#include <utils/qtcassert.h>
+
+#include <QtCore/QByteArray>
+#include <QtCore/QDebug>
+#include <QtCore/QTextStream>
+#include <QtCore/QFileInfo>
+#include <QtCore/QDir>
+
+//////////////////////////////////////////////////////////////////
+//
+// BreakpointMarker
+//
+//////////////////////////////////////////////////////////////////
+
+
+namespace Debugger {
+namespace Internal {
+
+BreakpointMarker::BreakpointMarker(BreakHandler *handler, BreakpointData *data, 
+        const QString &fileName, int lineNumber)
+    : BaseTextMark(fileName, lineNumber)
+    , m_handler(handler)
+    , m_data(data)
+    , m_pending(true)
+{
+    //qDebug() << "CREATE MARKER " << fileName << lineNumber;
+}
+
+BreakpointMarker::~BreakpointMarker()
+{
+    //qDebug() << "REMOVE MARKER ";
+    m_data = 0;
+}
+
+QIcon BreakpointMarker::icon() const
+{
+    if (!m_data->enabled)
+        return m_handler->disabledBreakpointIcon();
+    if (!m_handler->isActive())
+        return m_handler->emptyIcon();
+    return m_pending ? m_handler->pendingBreakPointIcon() : m_handler->breakpointIcon();
+}
+
+void BreakpointMarker::setPending(bool pending)
+{
+    if (pending == m_pending)
+        return;
+    m_pending = pending;
+    updateMarker();
+}
+
+void BreakpointMarker::updateBlock(const QTextBlock &)
+{
+    //qDebug() << "BREAKPOINT MARKER UPDATE BLOCK";
+}
+
+void BreakpointMarker::removedFromEditor()
+{
+    if (!m_data)
+        return;
+    m_handler->removeBreakpoint(m_data);
+    //handler->saveBreakpoints();
+    m_handler->updateMarkers();
+}
+
+void BreakpointMarker::updateLineNumber(int lineNumber)
+{
+    if (!m_data)
+        return;
+    //if (m_data->markerLineNumber == lineNumber)
+    //    return;
+    if (m_data->markerLineNumber() != lineNumber) {
+        m_data->setMarkerLineNumber(lineNumber);
+        // FIXME: Should we tell gdb about the change?
+        // Ignore it for now, as we would require re-compilation
+        // and debugger re-start anyway.
+        if (0 && m_data->bpLineNumber) {
+            if (!m_data->bpNumber.trimmed().isEmpty()) {