Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Some const-correctness in SaveHandler #5263

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Some const-correctness in SaveHandler #5263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e624070
Some const-correctness in SaveHandler:
bhennion Oct 22, 2022
6fa7567
Fix test unit
bhennion Oct 21, 2023
75ce62b
fixup util for const-correctness
bhennion Oct 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/core/control/Control.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1729,7 +1729,7 @@ auto Control::showSaveDialog() -> bool {
gtk_file_chooser_add_filter(GTK_FILE_CHOOSER(dialog), filterXoj);

this->doc->lock();
auto suggested_folder = this->doc->createSaveFolder(this->settings->getLastSavePath());
auto suggested_folder = this->doc->createSaveFoldername(this->settings->getLastSavePath());
auto suggested_name = this->doc->createSaveFilename(Document::XOPP, this->settings->getDefaultSaveName());
this->doc->unlock();

Expand Down Expand Up @@ -2220,7 +2220,7 @@ void Control::moveSelectionToLayer(size_t layerNo) {
}

auto* oldLayer = currentP->getSelectedLayer();
auto* newLayer = currentP->getLayers()->at(layerNo);
auto* newLayer = currentP->getLayers().at(layerNo);
auto moveSelUndo = std::make_unique<MoveSelectionToLayerUndoAction>(currentP, getLayerController(), oldLayer,
currentP->getSelectedLayerId() - 1, layerNo);
for (auto* e: selection->getElements()) {
Expand Down
2 changes: 1 addition & 1 deletion src/core/control/SearchControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ auto SearchControl::search(const std::string& text, size_t index, size_t* occurr
this->results = this->pdf->findText(text);
}

for (Layer* l: *this->page->getLayers()) {
for (Layer* l: this->page->getLayers()) {
if (!l->isVisible()) {
continue;
}
Expand Down
2 changes: 1 addition & 1 deletion src/core/control/jobs/BaseExportJob.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ auto BaseExportJob::showFilechooser() -> bool {
Settings* settings = control->getSettings();
Document* doc = control->getDocument();
doc->lock();
fs::path folder = doc->createSaveFolder(settings->getLastSavePath());
fs::path folder = doc->createSaveFoldername(settings->getLastSavePath());
fs::path name = doc->createSaveFilename(Document::PDF, settings->getDefaultSaveName(), settings->getDefaultPdfExportName());
doc->unlock();

Expand Down
11 changes: 6 additions & 5 deletions src/core/control/jobs/PreviewJob.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ void PreviewJob::finishPaint() {
}

void PreviewJob::drawPage() {
PageRef page = this->sidebarPreview->page;
ConstPageRef page = this->sidebarPreview->page;
Document* doc = this->sidebarPreview->sidebar->getControl()->getDocument();
DocumentView view;
view.setPdfCache(this->sidebarPreview->sidebar->getCache());
Expand Down Expand Up @@ -96,25 +96,26 @@ void PreviewJob::drawPage() {
if (layer == 0) {
view.drawBackground(xoj::view::BACKGROUND_SHOW_ALL);
} else {
Layer* drawLayer = (*page->getLayers())[layer - 1];
const Layer* drawLayer = page->getLayers()[layer - 1];
xoj::view::LayerView layerView(drawLayer);
layerView.draw(context);
}
view.finializeDrawing();
break;

case RENDER_TYPE_PAGE_LAYERSTACK:
case RENDER_TYPE_PAGE_LAYERSTACK: {
// render all layers up to layer
view.initDrawing(page, cr2, true);
view.drawBackground(xoj::view::BACKGROUND_SHOW_ALL);
const auto& layers = page->getLayers();
for (Layer::Index i = 0; i < layer; i++) {
Layer* drawLayer = (*page->getLayers())[i];
const Layer* drawLayer = layers[i];
xoj::view::LayerView layerView(drawLayer);
layerView.draw(context);
}
view.finializeDrawing();
break;

}
default:
// unknown type
break;
Expand Down
2 changes: 1 addition & 1 deletion src/core/control/layer/LayerController.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ void LayerController::mergeCurrentLayerDown() {
* the background is not in the vector, so layer 1 has index 0 and so on.
*/
const Layer::Index layerBelowIndex = layerBelowID - 1;
Layer* layerBelow = (*page->getLayers())[layerBelowIndex];
Layer* layerBelow = page->getLayers()[layerBelowIndex];

UndoActionPtr undo_redo_action =
std::make_unique<MergeLayerDownUndoAction>(this, page, currentLayer, layerBelow, layerID - 1, pageID);
Expand Down
11 changes: 6 additions & 5 deletions src/core/control/tools/Selection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,21 @@ auto Selection::finalize(PageRef page) -> size_t {
size_t layerId = 0;

if (multiLayer) {
for (int layerNo = page->getLayers()->size() - 1; layerNo >= 0; layerNo--) {
Layer* l = page->getLayers()->at(layerNo);
if (!l->isVisible()) {
const auto& layers = page->getLayers();
size_t layerNo = layers.size();
for (auto l = layers.rbegin(); l != layers.rend(); l = std::next(l), layerNo--) {
if (!(*l)->isVisible()) {
continue;
}
bool selectionOnLayer = false;
for (Element* e: l->getElements()) {
for (Element* e: (*l)->getElements()) {
if (e->isInSelection(this)) {
this->selectedElements.push_back(e);
selectionOnLayer = true;
}
}
if (selectionOnLayer) {
layerId = layerNo + 1;
layerId = layerNo;
break;
}
}
Expand Down
51 changes: 33 additions & 18 deletions src/core/control/xojfile/SaveHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ SaveHandler::SaveHandler() {
this->attachBgId = 1;
}

void SaveHandler::prepareSave(Document* doc) {
void SaveHandler::prepareSave(const Document* doc) {
if (this->root) {
// cleanup old data
backgroundImages.clear();
Expand Down Expand Up @@ -88,20 +88,20 @@ auto SaveHandler::getColorStr(Color c, unsigned char alpha) -> std::string {
return color;
}

void SaveHandler::writeTimestamp(AudioElement* audioElement, XmlAudioNode* xmlAudioNode) {
void SaveHandler::writeTimestamp(XmlAudioNode* xmlAudioNode, const AudioElement* audioElement) {
/** set stroke timestamp value to the XmlPointNode */
xmlAudioNode->setAttrib("ts", audioElement->getTimestamp());
xmlAudioNode->setAttrib("fn", audioElement->getAudioFilename().u8string());
}

void SaveHandler::visitStroke(XmlPointNode* stroke, Stroke* s) {
void SaveHandler::visitStroke(XmlPointNode* stroke, const Stroke* s) {
StrokeTool t = s->getToolType();

unsigned char alpha = 0xff;

if (t == StrokeTool::PEN) {
stroke->setAttrib("tool", "pen");
writeTimestamp(s, stroke);
writeTimestamp(stroke, s);
} else if (t == StrokeTool::ERASER) {
stroke->setAttrib("tool", "eraser");
} else if (t == StrokeTool::HIGHLIGHTER) {
Expand Down Expand Up @@ -134,7 +134,7 @@ void SaveHandler::visitStroke(XmlPointNode* stroke, Stroke* s) {
/**
* Export the fill attributes
*/
void SaveHandler::visitStrokeExtended(XmlPointNode* stroke, Stroke* s) {
void SaveHandler::visitStrokeExtended(XmlPointNode* stroke, const Stroke* s) {
if (s->getFill() != -1) {
stroke->setAttrib("fill", s->getFill());
}
Expand All @@ -156,7 +156,7 @@ void SaveHandler::visitStrokeExtended(XmlPointNode* stroke, Stroke* s) {
}
}

void SaveHandler::visitLayer(XmlNode* page, Layer* l) {
void SaveHandler::visitLayer(XmlNode* page, const Layer* l) {
auto* layer = new XmlNode("layer");
page->addChild(layer);
if (l->hasName()) {
Expand All @@ -182,7 +182,7 @@ void SaveHandler::visitLayer(XmlNode* page, Layer* l) {
text->setAttrib("y", t->getY());
text->setAttrib("color", getColorStr(t->getColor()).c_str());

writeTimestamp(t, text);
writeTimestamp(text, t);
} else if (e->getType() == ELEMENT_IMAGE) {
auto* i = dynamic_cast<Image*>(e);
auto* image = new XmlImageNode("image");
Expand All @@ -208,7 +208,7 @@ void SaveHandler::visitLayer(XmlNode* page, Layer* l) {
}
}

void SaveHandler::visitPage(XmlNode* root, PageRef p, Document* doc, int id) {
void SaveHandler::visitPage(XmlNode* root, ConstPageRef p, const Document* doc, int id) {
auto* page = new XmlNode("page");
root->addChild(page);
page->setAttrib("width", p->getWidth());
Expand Down Expand Up @@ -269,33 +269,46 @@ void SaveHandler::visitPage(XmlNode* root, PageRef p, Document* doc, int id) {
char* filename = g_strdup_printf("bg_%d.png", this->attachBgId++);
background->setAttrib("domain", "attach");
background->setAttrib("filename", filename);
p->getBackgroundImage().setFilepath(filename);

backgroundImages.emplace_back(p->getBackgroundImage());

/*
* Because BackgroundImage is basically a wrapped pointer, the following lines actually modify
* *(p->getBackgroundImage().content) and thus the Document.
* TODO Find a better way
*/
backgroundImages.back().setFilepath(filename);
backgroundImages.back().setCloneId(id);
Comment on lines +275 to +281
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still a bit problematic: the SaveHandler actually modifies a BackgroundImage to link it to a newly saved file. Because BackgroundImage is a simple pointer wrapper, this mean part of the data which constitutes the Document is modified, even though the Document instance is const.
This is fine in terms of c++, but will mess up any shared_mutex plan: we'd like the SaveHandler to only own a shared_lock (as opposed to a unique_lock), so that other read-only tasks (e.g. rendering) can still happen simultaniously.
Any suggestion would be welcome here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear to lock the whole document at once for a single change. This will make the hole application single threaded and will also block most of the UI at each operation. Instead, I would like to have many independent subgroups of the document, which can be locked and modified simultaneously.

Another alternative would be, to remove the locking completely and to replace any changeable subpart by an atomic<shared_ptr> to constant (immutable) Data. The data can then be replaced with a read-modify-write operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear to lock the whole document at once for a single change. This will make the hole application single threaded and will also block most of the UI at each operation. Instead, I would like to have many independent subgroups of the document, which can be locked and modified simultaneously.

I agree with that. I'd say one mutex per page (or maybe per layer), and one for the document's structure (i.e. the pages vector) is a decent balance. Of course, if tiling comes into play at some point, this'll have to be rethought.

Another alternative would be, to remove the locking completely and to replace any changeable subpart by an atomic<shared_ptr> to constant (immutable) Data. The data can then be replaced with a read-modify-write operation.

This would be very expensive. I don't quite see how to clone the content of a layer in an atomic way either.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative would be, to remove the locking completely and to replace any changeable subpart by an atomic<shared_ptr> to constant (immutable) Data. The data can then be replaced with a read-modify-write operation.

This would be very expensive. I don't quite see how to clone the content of a layer in an atomic way either.

It hardly depends on the data structure. Every change must be kept in the Undo Redo Handler, therefore it is copied anyway. When you now store all the document state in a tree of shared_ptr nodes, you would just reference the unit you want to change in the undo-redo handler and then replace the shared_ptr with a copy of it and its data. The replacement of the shared_ptr can be done atomically. We only need a solution for the ABA problem, but that's solvable by introducing a change index.


g_free(filename);
p->getBackgroundImage().setCloneId(id);
} else {
background->setAttrib("domain", "absolute");
background->setAttrib("filename", p->getBackgroundImage().getFilepath().string());
p->getBackgroundImage().setCloneId(id);
BackgroundImage img = p->getBackgroundImage();

/*
* Because BackgroundImage is basically a wrapped pointer, the following line actually modifies
* *(p->getBackgroundImage().content) and thus the Document.
* TODO Find a better way
*/
img.setCloneId(id);
}
} else {
writeSolidBackground(background, p);
}

// no layer, but we need to write one layer, else the old Xournal cannot read the file
if (p->getLayers()->empty()) {
if (p->getLayerCount() == 0) {
auto* layer = new XmlNode("layer");
page->addChild(layer);
}

for (Layer* l: *p->getLayers()) {
for (const Layer* l: p->getLayers()) {
visitLayer(page, l);
}
}

void SaveHandler::writeSolidBackground(XmlNode* background, PageRef p) {
void SaveHandler::writeSolidBackground(XmlNode* background, ConstPageRef p) {
background->setAttrib("type", "solid");
background->setAttrib("color", getColorStr(p->getBackgroundColor()));
background->setAttrib("style", PageTypeHandler::getStringForPageTypeFormat(p->getBackgroundType().format));
Expand All @@ -307,7 +320,7 @@ void SaveHandler::writeSolidBackground(XmlNode* background, PageRef p) {
}
}

void SaveHandler::writeBackgroundName(XmlNode* background, PageRef p) {
void SaveHandler::writeBackgroundName(XmlNode* background, ConstPageRef p) {
if (p->backgroundHasName()) {
background->setAttrib("name", p->getBackgroundName());
}
Expand Down Expand Up @@ -336,9 +349,11 @@ void SaveHandler::saveTo(OutputStream* out, const fs::path& filepath, ProgressLi
out->write("<?xml version=\"1.0\" standalone=\"no\"?>\n");
root->writeOut(out, listener);

for (BackgroundImage const& img: backgroundImages) {
for (const BackgroundImage& img: backgroundImages) {
auto tmpfn = (fs::path(filepath) += ".") += img.getFilepath();
if (!gdk_pixbuf_save(img.getPixbuf(), tmpfn.u8string().c_str(), "png", nullptr, nullptr)) {
// Are we certain that does not modify the GdkPixbuf?
if (!gdk_pixbuf_save(const_cast<GdkPixbuf*>(img.getPixbuf()), tmpfn.u8string().c_str(), "png", nullptr,
nullptr)) {
if (!this->errorMessage.empty()) {
this->errorMessage += "\n";
}
Expand All @@ -348,4 +363,4 @@ void SaveHandler::saveTo(OutputStream* out, const fs::path& filepath, ProgressLi
}
}

auto SaveHandler::getErrorMessage() -> std::string { return this->errorMessage; }
auto SaveHandler::getErrorMessage() -> const std::string& { return this->errorMessage; }
18 changes: 9 additions & 9 deletions src/core/control/xojfile/SaveHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,27 +36,27 @@ class SaveHandler {
SaveHandler();

public:
void prepareSave(Document* doc);
void prepareSave(const Document* doc);
void saveTo(const fs::path& filepath, ProgressListener* listener = nullptr);
void saveTo(OutputStream* out, const fs::path& filepath, ProgressListener* listener = nullptr);
std::string getErrorMessage();
const std::string& getErrorMessage();

protected:
static std::string getColorStr(Color c, unsigned char alpha = 0xff);

virtual void visitPage(XmlNode* root, PageRef p, Document* doc, int id);
virtual void visitLayer(XmlNode* page, Layer* l);
virtual void visitStroke(XmlPointNode* stroke, Stroke* s);
virtual void visitPage(XmlNode* root, ConstPageRef p, const Document* doc, int id);
virtual void visitLayer(XmlNode* page, const Layer* l);
virtual void visitStroke(XmlPointNode* stroke, const Stroke* s);

/**
* Export the fill attributes
*/
virtual void visitStrokeExtended(XmlPointNode* stroke, Stroke* s);
virtual void visitStrokeExtended(XmlPointNode* stroke, const Stroke* s);

virtual void writeHeader();
virtual void writeSolidBackground(XmlNode* background, PageRef p);
virtual void writeTimestamp(AudioElement* audioElement, XmlAudioNode* xmlAudioNode);
virtual void writeBackgroundName(XmlNode* background, PageRef p);
virtual void writeSolidBackground(XmlNode* background, ConstPageRef p);
virtual void writeTimestamp(XmlAudioNode* xmlAudioNode, const AudioElement* audioElement);
virtual void writeBackgroundName(XmlNode* background, ConstPageRef p);

protected:
std::unique_ptr<XmlNode> root{};
Expand Down
8 changes: 4 additions & 4 deletions src/core/control/xojfile/XojExportHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ XojExportHandler::~XojExportHandler() = default;
/**
* Export the fill attributes
*/
void XojExportHandler::visitStrokeExtended(XmlPointNode* stroke, Stroke* s) {
void XojExportHandler::visitStrokeExtended(XmlPointNode* stroke, const Stroke* s) {
// Fill is not exported in .xoj
// Line style is also not supported
}
Expand All @@ -36,7 +36,7 @@ void XojExportHandler::writeHeader() {
new XmlTextNode("title", std::string{"Xournal document (Compatibility) - see "} + PROJECT_URL));
}

void XojExportHandler::writeSolidBackground(XmlNode* background, PageRef p) {
void XojExportHandler::writeSolidBackground(XmlNode* background, ConstPageRef p) {
background->setAttrib("type", "solid");
background->setAttrib("color", getColorStr(p->getBackgroundColor()));

Expand All @@ -52,10 +52,10 @@ void XojExportHandler::writeSolidBackground(XmlNode* background, PageRef p) {
background->setAttrib("style", format);
}

void XojExportHandler::writeTimestamp(AudioElement* audioElement, XmlAudioNode* xmlAudioNode) {
void XojExportHandler::writeTimestamp(XmlAudioNode* xmlAudioNode, const AudioElement* audioElement) {
// Do nothing since timestamp are not supported by Xournal
}

void XojExportHandler::writeBackgroundName(XmlNode* background, PageRef p) {
void XojExportHandler::writeBackgroundName(XmlNode* background, ConstPageRef p) {
// Do nothing since background name is not supported by Xournal
}
8 changes: 4 additions & 4 deletions src/core/control/xojfile/XojExportHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ class XojExportHandler: public SaveHandler {
/**
* Export the fill attributes
*/
void visitStrokeExtended(XmlPointNode* stroke, Stroke* s) override;
void visitStrokeExtended(XmlPointNode* stroke, const Stroke* s) override;
void writeHeader() override;
void writeSolidBackground(XmlNode* background, PageRef p) override;
void writeTimestamp(AudioElement* audioElement, XmlAudioNode* xmlAudioNode) override;
void writeBackgroundName(XmlNode* background, PageRef p) override;
void writeSolidBackground(XmlNode* background, ConstPageRef p) override;
void writeTimestamp(XmlAudioNode* xmlAudioNode, const AudioElement* audioElement) override;
void writeBackgroundName(XmlNode* background, ConstPageRef p) override;

private:
};
12 changes: 6 additions & 6 deletions src/core/gui/PageViewFindObjectHelper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,11 @@ class BaseSelectObject {

if (multiLayer) {
size_t initialLayer = this->view->getPage()->getSelectedLayerId();
for (int layerNo = static_cast<int>(this->view->getPage()->getLayers()->size() - 1); layerNo >= 0;
layerNo--) {
Layer* layer = this->view->getPage()->getLayers()->at(layerNo);
this->view->getXournal()->getControl()->getLayerController()->switchToLay(layerNo + 1);
if (checkLayer(layer)) {
const auto& layers = this->view->getPage()->getLayers();
size_t layerNo = layers.size();
for (auto l = layers.rbegin(); l != layers.rend(); l = std::next(l), layerNo--) {
this->view->getXournal()->getControl()->getLayerController()->switchToLay(layerNo);
if (checkLayer(*l)) {
return true;
}
}
Expand All @@ -56,7 +56,7 @@ class BaseSelectObject {
}

protected:
bool checkLayer(Layer* l) {
bool checkLayer(const Layer* l) {
/* Search for Element whose bounding box center is closest to the place (x,y) where the object is searched for.
* Only those strokes are taken into account that pass the appropriate checkElement test.
*/
Expand Down
5 changes: 3 additions & 2 deletions src/core/model/BackgroundImage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ void BackgroundImage::loadFile(GInputStream* stream, fs::path const& path, GErro
this->img = std::make_shared<Content>(stream, path, error);
}

auto BackgroundImage::getCloneId() -> int { return this->img ? this->img->pageId : -1; }
auto BackgroundImage::getCloneId() const -> int { return this->img ? this->img->pageId : -1; }

void BackgroundImage::setCloneId(int id) {
if (this->img) {
Expand Down Expand Up @@ -88,6 +88,7 @@ void BackgroundImage::setAttach(bool attach) {
this->img->attach = attach;
}

auto BackgroundImage::getPixbuf() const -> GdkPixbuf* { return this->img ? this->img->pixbuf : nullptr; }
auto BackgroundImage::getPixbuf() -> GdkPixbuf* { return this->img ? this->img->pixbuf : nullptr; }
auto BackgroundImage::getPixbuf() const -> const GdkPixbuf* { return this->img ? this->img->pixbuf : nullptr; }

auto BackgroundImage::isEmpty() const -> bool { return !this->img; }
Loading
Loading