From 1af5c10506c6ef6e68a024d152b95edc99b0e90a Mon Sep 17 00:00:00 2001 From: Collin Kidder Date: Wed, 24 Jun 2015 20:44:38 -0400 Subject: [PATCH] Fix to make access of canbus frames const correct. It's now not really possible to modify the captured canbus frames from outside the model class that holds them. This is safer and more future proof. Boring update but it might save head scratching in the future. --- canframemodel.cpp | 30 +++++++++++++++++++++++------- canframemodel.h | 3 ++- config.h | 2 +- flowviewwindow.cpp | 2 +- flowviewwindow.h | 4 ++-- framefileio.cpp | 10 +++++----- framefileio.h | 10 +++++----- frameinfowindow.cpp | 2 +- frameinfowindow.h | 4 ++-- frameplaybackwindow.cpp | 2 +- frameplaybackwindow.h | 4 ++-- framesenderwindow.cpp | 2 +- framesenderwindow.h | 4 ++-- graphingwindow.cpp | 6 +++--- graphingwindow.h | 4 ++-- mainwindow.cpp | 19 +++++++++++-------- 16 files changed, 64 insertions(+), 44 deletions(-) diff --git a/canframemodel.cpp b/canframemodel.cpp index 04b3f85..71c9b74 100644 --- a/canframemodel.cpp +++ b/canframemodel.cpp @@ -260,13 +260,29 @@ void CANFrameModel::clearFrames() mutex.unlock(); } -//Is this safe? Maybe not but if we don't change it then that's OK -//Is it the best C++ practice? Probably not. This breaks the MVC paradigm -//but, it's for a good cause. -//Implement proper "const"ness for this function. No one should be -//adding frames via this reference. Unfortunately, I've done just that in -//places like the file loading code. -QVector* CANFrameModel::getListReference() +/* + * Since the getListReference function returns readonly + * you can't insert frames with it. Instead this function + * allows for a mass import of frames into the model + */ +void CANFrameModel::insertFrames(const QVector &newFrames) +{ + beginInsertRows(QModelIndex(), frames.count() + 1, frames.count() + newFrames.count()); + for (int i = 0; i < newFrames.count(); i++) + { + frames.append(newFrames[i]); + } + endInsertRows(); +} + +/* + *This used to not be const correct but it is now. So, there's little harm in + * allowing external code to peek at our frames. There's just no touching. + * This ability to get a direct read-only reference speeds up a variety of + * external code that needs to access frames directly and doesn't care about + * this model's normal output mechanism. + */ +const QVector* CANFrameModel::getListReference() const { return &frames; } diff --git a/canframemodel.h b/canframemodel.h index 2fab8ab..5378243 100644 --- a/canframemodel.h +++ b/canframemodel.h @@ -31,7 +31,8 @@ public: void setOverwriteMode(bool); void normalizeTiming(); void recalcOverwrite(); - QVector *getListReference(); + void insertFrames(const QVector &newFrames); + const QVector *getListReference() const; //thou shalt not modify these frames externally! private: diff --git a/config.h b/config.h index 35cc4d5..35d9393 100644 --- a/config.h +++ b/config.h @@ -1,7 +1,7 @@ #ifndef CONFIG #define CONFIG -#define VERSION 114 +#define VERSION 115 #endif // CONFIG diff --git a/flowviewwindow.cpp b/flowviewwindow.cpp index 40e14da..f2879fa 100644 --- a/flowviewwindow.cpp +++ b/flowviewwindow.cpp @@ -6,7 +6,7 @@ const QColor FlowViewWindow::graphColors[8] = {Qt::blue, Qt::green, Qt::black, Q Qt::gray, Qt::yellow, Qt::cyan, Qt::darkMagenta}; //4 5 6 7 -FlowViewWindow::FlowViewWindow(QVector *frames, QWidget *parent) : +FlowViewWindow::FlowViewWindow(const QVector *frames, QWidget *parent) : QDialog(parent), ui(new Ui::FlowViewWindow) { diff --git a/flowviewwindow.h b/flowviewwindow.h index 248a557..b1301f4 100644 --- a/flowviewwindow.h +++ b/flowviewwindow.h @@ -13,7 +13,7 @@ class FlowViewWindow : public QDialog Q_OBJECT public: - explicit FlowViewWindow(QVector *frames, QWidget *parent = 0); + explicit FlowViewWindow(const QVector *frames, QWidget *parent = 0); ~FlowViewWindow(); void showEvent(QShowEvent*); @@ -38,7 +38,7 @@ private: Ui::FlowViewWindow *ui; QList foundID; QList frameCache; - QVector *modelFrames; + const QVector *modelFrames; unsigned char refBytes[8]; unsigned char currBytes[8]; int currentPosition; diff --git a/framefileio.cpp b/framefileio.cpp index ce53d38..771d5fb 100644 --- a/framefileio.cpp +++ b/framefileio.cpp @@ -117,7 +117,7 @@ bool FrameFileIO::loadCRTDFile(QString filename, QVector* frames) return true; } -bool FrameFileIO::saveCRTDFile(QString filename, QVector* frames) +bool FrameFileIO::saveCRTDFile(QString filename, const QVector* frames) { QFile *outFile = new QFile(filename); @@ -197,7 +197,7 @@ bool FrameFileIO::loadNativeCSVFile(QString filename, QVector* frames) return true; } -bool FrameFileIO::saveNativeCSVFile(QString filename, QVector* frames) +bool FrameFileIO::saveNativeCSVFile(QString filename, const QVector* frames) { QFile *outFile = new QFile(filename); @@ -273,7 +273,7 @@ bool FrameFileIO::loadGenericCSVFile(QString filename, QVector* frames return true; } -bool FrameFileIO::saveGenericCSVFile(QString filename, QVector* frames) +bool FrameFileIO::saveGenericCSVFile(QString filename, const QVector* frames) { return false; } @@ -345,7 +345,7 @@ bool FrameFileIO::loadLogFile(QString filename, QVector* frames) return true; } -bool FrameFileIO::saveLogFile(QString filename, QVector* frames) +bool FrameFileIO::saveLogFile(QString filename, const QVector* frames) { return false; } @@ -403,7 +403,7 @@ bool FrameFileIO::loadMicrochipFile(QString filename, QVector* frames) return true; } -bool FrameFileIO::saveMicrochipFile(QString filename, QVector* frames) +bool FrameFileIO::saveMicrochipFile(QString filename, const QVector* frames) { return false; } diff --git a/framefileio.h b/framefileio.h index ef2b918..0dac85e 100644 --- a/framefileio.h +++ b/framefileio.h @@ -26,11 +26,11 @@ public: static bool loadGenericCSVFile(QString, QVector*); static bool loadLogFile(QString, QVector*); static bool loadMicrochipFile(QString, QVector*); - static bool saveCRTDFile(QString, QVector*); - static bool saveNativeCSVFile(QString, QVector*); - static bool saveGenericCSVFile(QString, QVector*); - static bool saveLogFile(QString, QVector*); - static bool saveMicrochipFile(QString, QVector*); + static bool saveCRTDFile(QString, const QVector*); + static bool saveNativeCSVFile(QString, const QVector*); + static bool saveGenericCSVFile(QString, const QVector*); + static bool saveLogFile(QString, const QVector*); + static bool saveMicrochipFile(QString, const QVector*); static QString loadFrameFile(QVector*); }; diff --git a/frameinfowindow.cpp b/frameinfowindow.cpp index e29111f..c7b74ef 100644 --- a/frameinfowindow.cpp +++ b/frameinfowindow.cpp @@ -3,7 +3,7 @@ #include "mainwindow.h" #include -FrameInfoWindow::FrameInfoWindow(QVector *frames, QWidget *parent) : +FrameInfoWindow::FrameInfoWindow(const QVector *frames, QWidget *parent) : QDialog(parent), ui(new Ui::FrameInfoWindow) { diff --git a/frameinfowindow.h b/frameinfowindow.h index e7fb050..1f96d19 100644 --- a/frameinfowindow.h +++ b/frameinfowindow.h @@ -14,7 +14,7 @@ class FrameInfoWindow : public QDialog Q_OBJECT public: - explicit FrameInfoWindow(QVector *frames, QWidget *parent = 0); + explicit FrameInfoWindow(const QVector *frames, QWidget *parent = 0); ~FrameInfoWindow(); void showEvent(QShowEvent*); @@ -27,7 +27,7 @@ private: QList foundID; QList frameCache; - QVector *modelFrames; + const QVector *modelFrames; void refreshIDList(); }; diff --git a/frameplaybackwindow.cpp b/frameplaybackwindow.cpp index f0c7ad4..e047815 100644 --- a/frameplaybackwindow.cpp +++ b/frameplaybackwindow.cpp @@ -16,7 +16,7 @@ * */ -FramePlaybackWindow::FramePlaybackWindow(QVector *frames, SerialWorker *worker, QWidget *parent) : +FramePlaybackWindow::FramePlaybackWindow(const QVector *frames, SerialWorker *worker, QWidget *parent) : QDialog(parent), ui(new Ui::FramePlaybackWindow) { diff --git a/frameplaybackwindow.h b/frameplaybackwindow.h index 7960faa..bc7feaa 100644 --- a/frameplaybackwindow.h +++ b/frameplaybackwindow.h @@ -27,7 +27,7 @@ class FramePlaybackWindow : public QDialog Q_OBJECT public: - explicit FramePlaybackWindow(QVector *frames, SerialWorker *worker, QWidget *parent = 0); + explicit FramePlaybackWindow(const QVector *frames, SerialWorker *worker, QWidget *parent = 0); ~FramePlaybackWindow(); private slots: @@ -57,7 +57,7 @@ private: Ui::FramePlaybackWindow *ui; QList foundID; QList frameCache; - QVector *modelFrames; + const QVector *modelFrames; int currentPosition; QTimer *playbackTimer; SerialWorker *serialWorker; diff --git a/framesenderwindow.cpp b/framesenderwindow.cpp index 77392cb..781ffe6 100644 --- a/framesenderwindow.cpp +++ b/framesenderwindow.cpp @@ -2,7 +2,7 @@ #include "ui_framesenderwindow.h" #include "utility.h" -FrameSenderWindow::FrameSenderWindow(QVector *frames, QWidget *parent) : +FrameSenderWindow::FrameSenderWindow(const QVector *frames, QWidget *parent) : QDialog(parent), ui(new Ui::FrameSenderWindow) { diff --git a/framesenderwindow.h b/framesenderwindow.h index 2360240..ea0db57 100644 --- a/framesenderwindow.h +++ b/framesenderwindow.h @@ -15,7 +15,7 @@ class FrameSenderWindow : public QDialog Q_OBJECT public: - explicit FrameSenderWindow(QVector *frames, QWidget *parent = 0); + explicit FrameSenderWindow(const QVector *frames, QWidget *parent = 0); ~FrameSenderWindow(); private slots: @@ -26,7 +26,7 @@ private: Ui::FrameSenderWindow *ui; QList sendingData; QList frameCache; - QVector *modelFrames; + const QVector *modelFrames; QTimer *intervalTimer; void doModifiers(int); diff --git a/graphingwindow.cpp b/graphingwindow.cpp index d955cde..2ce0c9c 100644 --- a/graphingwindow.cpp +++ b/graphingwindow.cpp @@ -4,7 +4,7 @@ #include "mainwindow.h" #include -GraphingWindow::GraphingWindow(QVector *frames, QWidget *parent) : +GraphingWindow::GraphingWindow(const QVector *frames, QWidget *parent) : QDialog(parent), ui(new Ui::GraphingWindow) { @@ -344,8 +344,8 @@ void GraphingWindow::addNewGraph() void GraphingWindow::createGraph(GraphParams ¶ms, bool createGraphParam) { int tempVal; - float yminval=10000000, ymaxval = -1000000; - float xminval=100000000000, xmaxval = -100000000000; + float yminval=10000000.0, ymaxval = -1000000.0; + float xminval=10000000000.0, xmaxval = -10000000000.0; qDebug() << "New Graph ID: " << params.ID; qDebug() << "Start byte: " << params.startByte; diff --git a/graphingwindow.h b/graphingwindow.h index 5ed42fd..a1502f4 100644 --- a/graphingwindow.h +++ b/graphingwindow.h @@ -29,7 +29,7 @@ class GraphingWindow : public QDialog Q_OBJECT public: - explicit GraphingWindow(QVector *, QWidget *parent = 0); + explicit GraphingWindow(const QVector *, QWidget *parent = 0); ~GraphingWindow(); void showEvent(QShowEvent*); @@ -53,7 +53,7 @@ private slots: private: Ui::GraphingWindow *ui; QList frameCache; - QVector *modelFrames; + const QVector *modelFrames; QList graphParams; QPen selectedPen; bool needScaleSetup; //do we need to set x,y graphing extents?s diff --git a/mainwindow.cpp b/mainwindow.cpp index 2028a45..819e0d1 100644 --- a/mainwindow.cpp +++ b/mainwindow.cpp @@ -307,18 +307,21 @@ void MainWindow::handleLoadFile() ui->canFramesView->scrollToTop(); model->clearFrames(); - if (dialog.selectedNameFilter() == filters[0]) result = FrameFileIO::loadCRTDFile(filename, model->getListReference()); - if (dialog.selectedNameFilter() == filters[1]) result = FrameFileIO::loadNativeCSVFile(filename, model->getListReference()); - if (dialog.selectedNameFilter() == filters[2]) result = FrameFileIO::loadGenericCSVFile(filename, model->getListReference()); - if (dialog.selectedNameFilter() == filters[3]) result = FrameFileIO::loadLogFile(filename, model->getListReference()); - if (dialog.selectedNameFilter() == filters[4]) result = FrameFileIO::loadMicrochipFile(filename, model->getListReference()); + QVector tempFrames; + + if (dialog.selectedNameFilter() == filters[0]) result = FrameFileIO::loadCRTDFile(filename, &tempFrames); + if (dialog.selectedNameFilter() == filters[1]) result = FrameFileIO::loadNativeCSVFile(filename, &tempFrames); + if (dialog.selectedNameFilter() == filters[2]) result = FrameFileIO::loadGenericCSVFile(filename, &tempFrames); + if (dialog.selectedNameFilter() == filters[3]) result = FrameFileIO::loadLogFile(filename, &tempFrames); + if (dialog.selectedNameFilter() == filters[4]) result = FrameFileIO::loadMicrochipFile(filename, &tempFrames); if (result) { + model->insertFrames(tempFrames); + QStringList fileList = filename.split('/'); loadedFileName = fileList[fileList.length() - 1]; model->recalcOverwrite(); - model->sendRefresh(); ui->lbNumFrames->setText(QString::number(model->rowCount())); if (ui->cbAutoScroll->isChecked()) ui->canFramesView->scrollToBottom(); @@ -348,7 +351,7 @@ void MainWindow::handleSaveFile() if (dialog.exec() == QDialog::Accepted) { - QVector *frames = model->getListReference(); + const QVector *frames = model->getListReference(); filename = dialog.selectedFiles()[0]; if (dialog.selectedNameFilter() == filters[0]) result = FrameFileIO::saveCRTDFile(filename, frames); if (dialog.selectedNameFilter() == filters[1]) result = FrameFileIO::saveNativeCSVFile(filename, frames); @@ -433,7 +436,7 @@ void MainWindow::handleSaveDecoded() void MainWindow::saveDecodedTextFile(QString filename) { QFile *outFile = new QFile(filename); - QVector *frames = model->getListReference(); + const QVector *frames = model->getListReference(); if (!outFile->open(QIODevice::WriteOnly | QIODevice::Text)) return;