Skip to content

Commit

Permalink
Refactor EditSelection contructors:
Browse files Browse the repository at this point in the history 
    * Properly lock the document mutex when needed
    * Optimized out 2 out of 3 loops looking for elements in a layer
    * Make SelectionFactory to have explicitly named "contructors"
    * Avoid adding elements to the layer just before removing them for
      selection
  • Loading branch information
@bhennion
bhennion committed Nov 22, 2023
1 parent 773fc8b commit 62ac7b6
Show file tree
Hide file tree
Showing 18 changed files with 544 additions and 362 deletions.
52 changes: 27 additions & 25 deletions src/core/control/Control.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -509,15 +509,21 @@ void Control::selectAllOnPage() {

win->getXournal()->clearSelection();

if (layer->getElements().empty()) {
this->doc->unlock();
return;
}

EditSelection* selection = new EditSelection(this->undoRedo, view, page, layer);
std::vector<Element*> elements = layer->clearNoFree();
this->doc->unlock();

win->getXournal()->setSelection(selection);
if (!elements.empty()) {
InsertionOrder insertOrder;
insertOrder.reserve(elements.size());
Element::Index n = 0;
std::transform(elements.begin(), elements.end(), std::back_inserter(insertOrder),
[&n](Element* e) { return InsertionPosition(e, n++); });
auto [sel, rg] = SelectionFactory::createFromFloatingElements(this, page, layer, view, std::move(insertOrder));

page->fireRangeChanged(rg);

win->getXournal()->setSelection(sel.release());
}
}

void Control::reorderSelection(EditSelection::OrderChange change) {
Expand Down Expand Up @@ -1016,12 +1022,10 @@ void Control::selectTool(ToolType type) {
auto pageNr = getCurrentPageNo();
XojPageView* view = xournal->getViewFor(pageNr);
xoj_assert(view != nullptr);
this->doc->lock();
PageRef page = this->doc->getPage(pageNr);
auto selection = new EditSelection(this->undoRedo, textobj, view, page);
this->doc->unlock();
PageRef page = view->getPage();
auto sel = SelectionFactory::createFromElementOnActiveLayer(this, page, view, textobj);

xournal->setSelection(selection);
xournal->setSelection(sel.release());
}

toolHandler->selectTool(type);
Expand Down Expand Up @@ -2091,6 +2095,8 @@ void Control::clipboardPaste(Element* e) {
this->doc->lock();
PageRef page = this->doc->getPage(pageNr);
Layer* layer = page->getSelectedLayer();
this->doc->unlock();

win->getXournal()->getPasteTarget(x, y);

double width = e->getElementWidth();
Expand All @@ -2101,14 +2107,11 @@ void Control::clipboardPaste(Element* e) {

e->setX(x);
e->setY(y);
layer->addElement(e);

this->doc->unlock();

undoRedo->addUndoAction(std::make_unique<InsertUndoAction>(page, layer, e));
auto* selection = new EditSelection(this->undoRedo, e, view, page);
auto sel = SelectionFactory::createFromFloatingElement(this, page, layer, view, e);

win->getXournal()->setSelection(selection);
win->getXournal()->setSelection(sel.release());
}

void Control::registerPluginToolButtons(ToolMenuHandler* toolMenuHandler) {
Expand All @@ -2132,26 +2135,26 @@ void Control::clipboardPasteXournal(ObjectInputStream& in) {
return;
}

EditSelection* selection = nullptr;
auto selection = std::make_unique<EditSelection>(this, page, page->getSelectedLayer(), view);
this->doc->unlock();

try {
std::unique_ptr<Element> element;
string version = in.readString();
std::string version = in.readString();
if (version != PROJECT_STRING) {
g_warning("Paste from Xournal Version %s to Xournal Version %s", version.c_str(), PROJECT_STRING);
}

selection = new EditSelection(this->undoRedo, page, view);
selection->readSerialized(in);

// document lock not needed anymore, because we don't change the document, we only change the selection
this->doc->unlock();

int count = in.readInt();
auto pasteAddUndoAction = std::make_unique<AddUndoAction>(page, false);
// this will undo a group of elements that are inserted

for (int i = 0; i < count; i++) {
string name = in.getNextObjectName();
std::string name = in.getNextObjectName();
element.reset();

if (name == "Stroke") {
Expand All @@ -2170,7 +2173,7 @@ void Control::clipboardPasteXournal(ObjectInputStream& in) {

pasteAddUndoAction->addElement(layer, element.get(), layer->indexOf(element.get()));
// Todo: unique_ptr
selection->addElement(element.release(), Element::InvalidIndex);
selection->addElement(element.release(), std::numeric_limits<Element::Index>::max());
}
undoRedo->addUndoAction(std::move(pasteAddUndoAction));

Expand All @@ -2190,15 +2193,14 @@ void Control::clipboardPasteXournal(ObjectInputStream& in) {
// update all Elements (same procedure as moving a element selection by hand and releasing the mouse button)
selection->mouseUp();

win->getXournal()->setSelection(selection);
win->getXournal()->setSelection(selection.release());
} catch (const std::exception& e) {
g_warning("could not paste, Exception occurred: %s", e.what());
Stacktrace::printStracktrace();
if (selection) {
for (Element* el: selection->getElements()) {
delete el;
}
delete selection;
}
}
}
Expand Down
12 changes: 4 additions & 8 deletions src/core/control/LatexController.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,8 +277,8 @@ void LatexController::updateStatus() { this->dlg->setCompilationStatus(isValidTe

void LatexController::deleteOldImage() {
if (this->selectedElem) {
EditSelection selection(control->getUndoRedoHandler(), selectedElem, view, page);
this->view->getXournal()->deleteSelection(&selection);
auto sel = SelectionFactory::createFromElementOnActiveLayer(control, page, view, selectedElem);
this->view->getXournal()->deleteSelection(sel.release());
this->selectedElem = nullptr;
}
}
Expand Down Expand Up @@ -332,15 +332,11 @@ void LatexController::insertTexImage() {
this->control->clearSelectionEndText();
this->deleteOldImage();

doc->lock();
layer->addElement(img);
view->rerenderElement(img);
doc->unlock();
control->getUndoRedoHandler()->addUndoAction(std::make_unique<InsertUndoAction>(page, layer, img));

// Select element
auto* selection = new EditSelection(control->getUndoRedoHandler(), img, view, page);
view->getXournal()->setSelection(selection);
auto selection = SelectionFactory::createFromFloatingElement(control, page, layer, view, img);
view->getXournal()->setSelection(selection.release());
}

void LatexController::run(Control* ctrl) {
Expand Down
36 changes: 17 additions & 19 deletions src/core/control/UndoRedoController.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "model/Document.h" // for Document
#include "model/Layer.h" // for Layer
#include "model/PageRef.h" // for PageRef
#include "model/XojPage.h" // for XojPage
#include "undo/UndoRedoHandler.h" // for UndoRedoHandler

#include "Control.h" // for Control
Expand All @@ -18,20 +19,16 @@ class XojPageView;

UndoRedoController::UndoRedoController(Control* control): control(control) {}

UndoRedoController::~UndoRedoController() {
this->control = nullptr;
this->layer = nullptr;
elements.clear();
}
UndoRedoController::~UndoRedoController() = default;

void UndoRedoController::before() {
EditSelection* selection = control->getWindow()->getXournal()->getSelection();
if (selection != nullptr) {
layer = selection->getSourceLayer();
std::copy(selection->getElements().begin(), selection->getElements().end(), std::back_inserter(elements));
elements = selection->getElements();
}

control->clearSelection();
control->clearSelectionEndText();
}

void UndoRedoController::after() {
Expand All @@ -45,35 +42,36 @@ void UndoRedoController::after() {
Document* doc = control->getDocument();

PageRef page = control->getCurrentPage();
std::unique_lock lock(*doc);
size_t pageNo = doc->indexOf(page);
XojPageView* view = control->getWindow()->getXournal()->getViewFor(pageNo);

if (!view || !page) {
// The page may have been undone
return;
}

std::vector<Element*> visibleElements;
InsertionOrder remainingElements;
for (Element* e: elements) {
if (layer->indexOf(e) == -1) {
// Element is gone - so it's not selectable
continue;
// Test is the element has been removed since
if (auto pos = layer->indexOf(e); pos != -1) {
remainingElements.emplace_back(e, pos);
}

visibleElements.push_back(e);
}
if (!visibleElements.empty()) {
auto* selection = new EditSelection(control->getUndoRedoHandler(), visibleElements, view, page);
control->getWindow()->getXournal()->setSelection(selection);
if (!remainingElements.empty()) {
layer->removeElementsAt(remainingElements, false);
lock.unlock(); // Not needed anymore. For all other paths, the lock is released via ~unique_lock()
auto [sel, bounds] =
SelectionFactory::createFromFloatingElements(control, page, layer, view, std::move(remainingElements));
control->getWindow()->getXournal()->setSelection(sel.release());
page->fireRangeChanged(bounds);
}
}

void UndoRedoController::undo(Control* control) {
UndoRedoController handler(control);
handler.before();

// Move out of text mode to allow textboxundo to work
control->clearSelectionEndText();

control->getUndoRedoHandler()->undo();

handler.after();
Expand Down
5 changes: 1 addition & 4 deletions src/core/control/actions/ActionProperties.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,7 @@ struct ActionProperties<Action::UNDO> {
static constexpr const char* accelerators[] = {"<Ctrl>Z", nullptr};
#endif
static bool initiallyEnabled(Control* ctrl) { return ctrl->undoRedo->canUndo(); }
static void callback(GSimpleAction*, GVariant*, Control* ctrl) {
ctrl->clearSelectionEndText();
UndoRedoController::undo(ctrl);
}
static void callback(GSimpleAction*, GVariant*, Control* ctrl) { UndoRedoController::undo(ctrl); }
};
template <>
struct ActionProperties<Action::REDO> {
Expand Down
Loading

0 comments on commit 62ac7b6

Please sign in to comment.