From 65df52c26b16885c4e3305ed4068c3a089458acd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Gs=C3=A4nger?= Date: Tue, 8 Oct 2024 21:55:01 +0200 Subject: [PATCH] Fix atomlist, copy buf implementations --- gui/qt/glwidget.cpp | 12 ++----- gui/qt/mainwidgets/molwidget_aux/atomlist.cpp | 15 ++------ gui/qt/mainwidgets/molwidget_aux/atomlist.h | 1 - .../mainwidgets/molwidget_aux/atommodel.cpp | 10 +++--- gui/qt/mainwindow.cpp | 35 +++++++++++++------ gui/qt/vipsterapplication.cpp | 10 ++++-- gui/qt/vipsterapplication.h | 9 ++--- vipster/step.h | 16 --------- 8 files changed, 49 insertions(+), 59 deletions(-) diff --git a/gui/qt/glwidget.cpp b/gui/qt/glwidget.cpp index 9753deea..f2b9a455 100644 --- a/gui/qt/glwidget.cpp +++ b/gui/qt/glwidget.cpp @@ -259,11 +259,8 @@ void GLWidget::mousePressEvent(QMouseEvent *e) break; case MouseMode::Select: if(e->button() == Qt::MouseButton::RightButton){ - // TODO: this assumes viewport is active -> use viewport over vApp? - vApp.invokeOnSel([](Step::selection &sel){ - // TODO: sort out const-correctness - sel = const_cast(vApp.curStep()).select(SelectionFilter{}); - }); + // clear selection + vApp.updateSelection({}); } break; case MouseMode::Modify: @@ -378,10 +375,7 @@ void GLWidget::mouseReleaseEvent(QMouseEvent *e) filter.indices = std::move(pick); // create new selection from filter // TODO: this assumes viewport is active -> use viewport over vApp? - vApp.invokeOnSel([](Step::selection &sel, const SelectionFilter &filter){ - // TODO: sort out const-correctness - sel = const_cast(vApp.curStep()).select(filter); - }, filter); + vApp.updateSelection(filter); rectPos = mousePos; } break; diff --git a/gui/qt/mainwidgets/molwidget_aux/atomlist.cpp b/gui/qt/mainwidgets/molwidget_aux/atomlist.cpp index 9dccbcbc..87b00310 100644 --- a/gui/qt/mainwidgets/molwidget_aux/atomlist.cpp +++ b/gui/qt/mainwidgets/molwidget_aux/atomlist.cpp @@ -123,16 +123,12 @@ void AtomList::selectionChanged() for(const auto& i: idx){ filter.indices.emplace_back(static_cast(i.row()), SizeVec{}); } - vApp.invokeOnSel([](Step::selection &sel, const SelectionFilter &filter){ - // TODO: sort out const-correctness - sel = const_cast(vApp.curStep()).select(filter); - }, filter); + vApp.updateSelection(filter); } void AtomList::fmtSelectionHandler(int index) { - // TODO: sort out const-correctness - atomModel.setStep(const_cast(vApp.curStep()).asFmt(static_cast(index-2))); + atomModel.setStep(vApp.curStep().asFmt(static_cast(index-2))); } void AtomList::fmtButtonHandler() @@ -149,15 +145,10 @@ void AtomList::fmtButtonHandler() // modify actual Step auto fmt = static_cast(ifmt-2); vApp.invokeOnStep(&Step::setFmt, fmt, true); - // TODO: sort out const-correctness - ownStep = const_cast(vApp.curStep()).asFmt(fmt); // reset selection SelectionFilter filter{}; filter.mode = SelectionFilter::Mode::Index; filter.indices = vApp.curSel().getAtoms().indices; - vApp.invokeOnSel([](Step::selection &sel, const SelectionFilter &filter){ - // TODO: sort out const-correctness - sel = const_cast(vApp.curStep()).select(filter); - }, filter); + vApp.updateSelection(filter); } diff --git a/gui/qt/mainwidgets/molwidget_aux/atomlist.h b/gui/qt/mainwidgets/molwidget_aux/atomlist.h index 8c6b2e4b..0a00fea6 100644 --- a/gui/qt/mainwidgets/molwidget_aux/atomlist.h +++ b/gui/qt/mainwidgets/molwidget_aux/atomlist.h @@ -32,7 +32,6 @@ private slots: Ui::AtomList *ui; - std::optional ownStep{}; AtomModel atomModel{}; QList headerActions; }; diff --git a/gui/qt/mainwidgets/molwidget_aux/atommodel.cpp b/gui/qt/mainwidgets/molwidget_aux/atommodel.cpp index e451d95c..a78b3358 100644 --- a/gui/qt/mainwidgets/molwidget_aux/atommodel.cpp +++ b/gui/qt/mainwidgets/molwidget_aux/atommodel.cpp @@ -138,9 +138,9 @@ bool AtomModel::setData(const QModelIndex &index, const QVariant &value, int rol case c_x: case c_y: case c_z: - vApp.invokeOnStep([](Step &s, int i, int c, double d){ - s.at(i).coord[c] = d; - }, index.row(), col-c_x, value.toDouble()); + vApp.invokeOnStep([](Step &s, AtomFmt fmt, int i, int c, double d){ + s.asFmt(fmt).at(i).coord[c] = d; + }, curStep->getFmt(), index.row(), col-c_x, value.toDouble()); break; case c_charge: vApp.invokeOnStep([](Step &s, int i, double charge){ @@ -150,8 +150,8 @@ bool AtomModel::setData(const QModelIndex &index, const QVariant &value, int rol case c_fx: case c_fy: case c_fz: - vApp.invokeOnStep([](Step &s, int i, int dir, double charge){ - s.at(i).properties->forces[dir] = charge; + vApp.invokeOnStep([](Step &s, int i, int dir, double force){ + s.at(i).properties->forces[dir] = force; }, index.row(), col-c_fx, value.toDouble()); break; } diff --git a/gui/qt/mainwindow.cpp b/gui/qt/mainwindow.cpp index 8caf7d0b..afc56ce9 100644 --- a/gui/qt/mainwindow.cpp +++ b/gui/qt/mainwindow.cpp @@ -217,21 +217,32 @@ void MainWindow::setupEditMenu() Qt::Key_N); // Delete selected Atom(s) + auto delAtoms = [](Step &s, const Step::const_selection &sel) + { + std::set indices{}; + for(const auto [idx, _]: sel.getAtoms().indices){ + indices.insert(idx); + } + for(auto it = indices.rbegin(); it != indices.rend(); ++it) + { + if(*it < s.getNat()){ + s.delAtom(*it); + } + } + }; auto *delAction = editMenu.addAction("&Delete atom(s)", - [](){ - // TODO: figure out const-correctness - // BUG: this clears selection but does not trigger signal - vApp.invokeOnStep(&Step::delAtoms, const_cast(vApp.curSel())); + [&](){ + vApp.invokeOnStep(delAtoms, vApp.curSel()); + vApp.updateSelection({}); }, Qt::Key_Delete); // Cut atoms auto *cutAction = editMenu.addAction("C&ut atom(s)", - [](){ + [&](){ vApp.selectionToCopy(); - // TODO: figure out const-correctness - // BUG: this clears selection but does not trigger signal - vApp.invokeOnStep(&Step::delAtoms, const_cast(vApp.curSel())); + vApp.invokeOnStep(delAtoms, vApp.curSel()); + vApp.updateSelection({}); }, QKeySequence::Cut); cutAction->setEnabled(false); @@ -251,12 +262,16 @@ void MainWindow::setupEditMenu() // Paste atoms auto *pasteAction = editMenu.addAction("&Paste atom(s)", [](){ - vApp.invokeOnStep(static_cast(&Step::newAtoms), *vApp.copyBuf); + vApp.invokeOnStep([](Step &s){ + if (vApp.copyBuf.getNat() > 0) { + s.newAtoms(vApp.copyBuf); + } + }); }, QKeySequence::Paste); pasteAction->setEnabled(false); connect(&vApp, &Application::copyBufChanged, - pasteAction, [=](const Step::selection &buf){pasteAction->setEnabled(buf.getNat() > 0);}); + pasteAction, [=](const std::optional &buf){pasteAction->setEnabled(buf->getNat() > 0);}); // Separator editMenu.addSeparator(); diff --git a/gui/qt/vipsterapplication.cpp b/gui/qt/vipsterapplication.cpp index 0391a89d..3a382200 100644 --- a/gui/qt/vipsterapplication.cpp +++ b/gui/qt/vipsterapplication.cpp @@ -50,6 +50,12 @@ void Application::setActiveStep(Step &step, Step::selection &sel) emit activeStepChanged(step, sel); } +void Application::updateSelection(SelectionFilter filter) +{ + *pCurSel = pCurStep->select(filter); + emit selChanged(*pCurSel); +} + Application::StepState& Application::getState(const Step &step) { // initialize state if required @@ -102,6 +108,6 @@ void Application::newIOData(IOTuple &&t){ } void Application::selectionToCopy(){ - copyBuf = std::make_unique(*pCurSel); - emit copyBufChanged(*copyBuf); + copyBuf = *pCurSel; + emit copyBufChanged(copyBuf); } diff --git a/gui/qt/vipsterapplication.h b/gui/qt/vipsterapplication.h index 9e838bbf..375c856d 100644 --- a/gui/qt/vipsterapplication.h +++ b/gui/qt/vipsterapplication.h @@ -151,7 +151,7 @@ class Application: public QObject // TODO: integrate with OS-specific copy-paste functionality instead void selectionToCopy(); - std::unique_ptr copyBuf{}; // TODO: are lifetime semantics of selection sufficient?? convert to optional instead + Step copyBuf{}; template auto invokeOnStep(F &&f, Args &&...args) @@ -170,18 +170,19 @@ class Application: public QObject { if constexpr (!std::is_void_v(f), std::forward(args)...))>) { auto tmp = invokeImpl(*pCurSel, std::forward(f), std::forward(args)...); - emit selChanged(*pCurSel); + emit stepChanged(*pCurStep); return tmp; } else { invokeImpl(*pCurSel, std::forward(f), std::forward(args)...); - emit selChanged(*pCurSel); + emit stepChanged(*pCurStep); } } + void updateSelection(SelectionFilter filter); signals: void activeStepChanged(const Vipster::Step &step, const Vipster::Step::selection &sel); void stepChanged(const Vipster::Step &step); void selChanged(const Vipster::Step::selection &sel); - void copyBufChanged(const Vipster::Step::selection &buf); + void copyBufChanged(const Vipster::Step &buf); public: // GUI-Data related to a loaded Step diff --git a/vipster/step.h b/vipster/step.h index bb892e16..97560ef2 100644 --- a/vipster/step.h +++ b/vipster/step.h @@ -347,22 +347,6 @@ class Step: public StepMutable } } void delAtom(size_t i); - // TODO: this is a weird interface - either ensure that it is a selection object refering to own instance, or create a more abstract i.e. index based interface - template - void delAtoms(StepConst>& s) - { - std::set indices{}; - for(const auto [idx, _]: s.getAtoms().indices){ - indices.insert(idx); - } - for(auto it = indices.rbegin(); it != indices.rend(); ++it) - { - if(*it < getNat()){ - delAtom(*it); - } - } - s = select({}); - } // Cell void enableCell(bool val) noexcept;