Skip to content

Commit

Permalink
Put clipboard data on the system stack (#617)
Browse files Browse the repository at this point in the history
The clipboard data structures are created by the first document on the
heap and then are subsequently overridden by other documents.

It can be a better design to put the clipboard data into one central place
on the stack of `System` as it is available during the lifetime of the
program.
  • Loading branch information
tobiolo authored Mar 29, 2024
1 parent fa7c018 commit c438af6
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 22 deletions.
30 changes: 9 additions & 21 deletions src/document.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,6 @@ struct Document {
wxString filename;
long lastmodsinceautosave, undolistsizeatfullsave, lastsave;
bool modified, tmpsavesuccess;
wxDataObjectComposite *dataobjc;
wxTextDataObject *dataobjt;
wxBitmapDataObject *dataobji;
wxFileDataObject *dataobjf;
//wxHTMLDataObject *dataobjh;
//wxRichTextBufferDataObject *dataobjr;

struct MyPrintout : wxPrintout {
Document *doc;
Expand Down Expand Up @@ -118,12 +112,6 @@ struct Document {
currentviewscale(1),
searchfilter(false),
editfilter(0) {
dataobjc = new wxDataObjectComposite(); // deleted by DropTarget
dataobjc->Add(dataobji = new wxBitmapDataObject());
dataobjc->Add(dataobjt = new wxTextDataObject());
dataobjc->Add(dataobjf = new wxFileDataObject());
//dataobjc->Add(dataobjh = new wxHTMLDataObject(), true); // Prefer HTML over text, doesn't seem to work.
//dataobjc->Add(dataobjr = new wxRichTextBufferDataObject());
ResetFont();
pageSetupData = printData;
pageSetupData.SetMarginTopLeft(wxPoint(15, 15));
Expand Down Expand Up @@ -1582,7 +1570,7 @@ struct Document {
case A_PASTE:
if (!(c = selected.ThinExpand(this))) return OneCell();
if (wxTheClipboard->Open()) {
wxTheClipboard->GetData(*dataobjc);
wxTheClipboard->GetData(sys->dataobjc);
PasteOrDrop();
wxTheClipboard->Close();
} else if (sys->cellclipboard) {
Expand Down Expand Up @@ -2099,9 +2087,9 @@ struct Document {
Cell *c = selected.ThinExpand(this);
if (!c) return;
wxBusyCursor wait;
switch (dataobjc->GetReceivedFormat().GetType()) {
switch (sys->dataobjc.GetReceivedFormat().GetType()) {
case wxDF_FILENAME: {
const wxArrayString &as = dataobjf->GetFilenames();
const wxArrayString &as = sys->dataobjf.GetFilenames();
if (as.size()) {
if (as.size() > 1) sw->Status(_(L"Cannot drag & drop more than 1 file."));
c->AddUndo(this);
Expand All @@ -2116,12 +2104,12 @@ struct Document {
#ifdef __WXMSW__
case wxDF_PNG:
#endif
if (dataobji->GetBitmap().GetRefData() != wxNullBitmap.GetRefData()) {
if (sys->dataobji.GetBitmap().GetRefData() != wxNullBitmap.GetRefData()) {
c->AddUndo(this);
wxImage im = dataobji->GetBitmap().ConvertToImage();
wxImage im = sys->dataobji.GetBitmap().ConvertToImage();
vector<uint8_t> idv = ConvertWxImageToBuffer(im, wxBITMAP_TYPE_PNG);
SetImageBM(c, std::move(idv), sys->frame->csf);
dataobji->SetBitmap(wxNullBitmap);
sys->dataobji.SetBitmap(wxNullBitmap);
c->Reset();
Refresh();
}
Expand All @@ -2138,8 +2126,8 @@ struct Document {
}
*/
default: // several text formats
if (dataobjt->GetText() != wxEmptyString) {
wxString s = dataobjt->GetText();
if (sys->dataobjt.GetText() != wxEmptyString) {
wxString s = sys->dataobjt.GetText();
if ((sys->clipboardcopy == s) && sys->cellclipboard) {
c->Paste(this, sys->cellclipboard.get(), selected);
Refresh();
Expand All @@ -2161,7 +2149,7 @@ struct Document {
Refresh();
}
}
dataobjt->SetText(wxEmptyString);
sys->dataobjt.SetText(wxEmptyString);
}
break;
}
Expand Down
2 changes: 1 addition & 1 deletion src/myframe.h
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,7 @@ struct MyFrame : wxFrame {
nb->AddPage(sw, _(L"<unnamed>"), true, wxNullBitmap);
else
nb->InsertPage(0, sw, _(L"<unnamed>"), true, wxNullBitmap);
sw->SetDropTarget(new DropTarget(doc->dataobjc));
sw->SetDropTarget(new DropTarget(&sys->dataobjc));
sw->SetFocus();
return sw;
}
Expand Down
12 changes: 12 additions & 0 deletions src/system.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ struct System {
uint lastcellcolor = 0xFFFFFF;
uint lasttextcolor = 0;
uint lastbordcolor = 0xA0A0A0;
wxDataObjectComposite dataobjc;
wxTextDataObject dataobjt;
wxBitmapDataObject dataobji;
wxFileDataObject dataobjf;
//wxHTMLDataObject dataobjh;
//wxRichTextBufferDataObject dataobjr;

System(bool portable)
: defaultfont(
Expand Down Expand Up @@ -141,6 +147,12 @@ struct System {

// fsw.Connect(wxID_ANY, wxID_ANY, wxEVT_FSWATCHER,
// wxFileSystemWatcherEventHandler(System::OnFileChanged));

dataobjc.Add(&dataobji);
dataobjc.Add(&dataobjt);
dataobjc.Add(&dataobjf);
//dataobjc.Add(dataobjh, true); // Prefer HTML over text, doesn't seem to work.
//dataobjc.Add(dataobjr);
}

~System() {
Expand Down

8 comments on commit c438af6

@georgeraraujo
Copy link
Contributor

Choose a reason for hiding this comment

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

Ever since this commit (Release 8482864821), TreeSheets crashes upon exit and writes a crash dump in %HOMEPATH%\AppData\Local\CrashDumps. The issue does not happen with Release 8473991279.

My computer is running Windows 10 x64.

@tobiolo
Copy link
Collaborator Author

@tobiolo tobiolo commented on c438af6 Apr 5, 2024 via email

Choose a reason for hiding this comment

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

@georgeraraujo
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @t2b3 ,
Thanks for the feedback!
I should have been more clear: I tested ALL releases since this commit, including Release 8566882814 from earlier today, and the issue happens with all of them. I suppose it has to do with the changes in the code that handles clipboard data.

@tobiolo
Copy link
Collaborator Author

@tobiolo tobiolo commented on c438af6 Apr 5, 2024 via email

Choose a reason for hiding this comment

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

@georgeraraujo
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it always crashes, even if I just open and the close the application.

One thing I noticed: it happens while closing a file. I have 4 files that I always keep open. In my last test, instead of quitting with CTRL+Q, I tried closing each file with CTRL+W, and TreeSheets crashed on one of the them (I did this a few times and it happened on different files). When I ran TreeSheets again, the file that I had tried to close when the crash happened was reloaded, as if I hadn't explicitly closed it.

@tobiolo
Copy link
Collaborator Author

@tobiolo tobiolo commented on c438af6 Apr 5, 2024

Choose a reason for hiding this comment

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

Thank you very much for discovering the issue!
I could reproduce the issue and indeed now pushed a fix (hopefully for it).

(gdb) bt
#0  0x00005e304b075966 in wxDropTargetBase::~wxDropTargetBase (this=0x5e304cb822a0, __in_chrg=<optimized out>) at /home/t2b3/development/treesheets/lib/wxWidgets/include/wx/dnd.h:140
#1  0x00005e304b0ad076 in wxDropTarget::~wxDropTarget (this=0x5e304cb822a0, __in_chrg=<optimized out>) at /home/t2b3/development/treesheets/lib/wxWidgets/include/wx/gtk/dnd.h:28
#2  0x00005e304b0f2cc4 in treesheets::DropTarget::~DropTarget (this=0x5e304cb822a0, __in_chrg=<optimized out>) at /home/t2b3/development/treesheets/src/mywxtools.h:11
#3  0x00005e304b0f2ce0 in treesheets::DropTarget::~DropTarget (this=0x5e304cb822a0, __in_chrg=<optimized out>) at /home/t2b3/development/treesheets/src/mywxtools.h:11
#4  0x00005e304b24ae7f in wxWindowBase::~wxWindowBase (this=0x5e304d2f20a0, __in_chrg=<optimized out>) at /home/t2b3/development/treesheets/lib/wxWidgets/src/common/wincmn.cpp:515
#5  0x00005e304b3071c2 in wxWindow::~wxWindow (this=0x5e304d2f20a0, __in_chrg=<optimized out>) at /home/t2b3/development/treesheets/lib/wxWidgets/src/gtk/window.cpp:3026
#6  0x00005e304b0aecbc in wxScrolled<wxWindow>::~wxScrolled (this=0x5e304d2f20a0, __in_chrg=<optimized out>) at /home/t2b3/development/treesheets/lib/wxWidgets/include/wx/scrolwin.h:415
#7  0x00005e304b0aef48 in treesheets::TSCanvas::~TSCanvas (this=0x5e304d2f20a0, __in_chrg=<optimized out>) at /home/t2b3/development/treesheets/src/mycanvas.h:28
#8  0x00005e304b0aef74 in treesheets::TSCanvas::~TSCanvas (this=0x5e304d2f20a0, __in_chrg=<optimized out>) at /home/t2b3/development/treesheets/src/mycanvas.h:28
#9  0x00005e304b24b1ab in wxWindowBase::Destroy (this=0x5e304d2f20a0) at /home/t2b3/development/treesheets/lib/wxWidgets/src/common/wincmn.cpp:570
#10 0x00005e304b12d442 in wxAuiNotebook::DeletePage (this=0x5e304d24b760, page_idx=0) at /home/t2b3/development/treesheets/lib/wxWidgets/src/aui/auibook.cpp:2087
#11 0x00005e304b0cd065 in treesheets::MyFrame::OnClosing (this=0x5e304ccd8530, ce=...) at /home/t2b3/development/treesheets/src/myframe.h:1191

The problem is that
https://docs.wxwidgets.org/latest/classwx_drop_target.html#ac7af5b2047050278248dc9ab41625892
the destructor of wxDropTarget frees the data per page destruction, and if we have multiple pages closing, it tries to free the data structure multiple times leading to the crash.

@georgeraraujo
Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested Release 8575186124 and the issue is fixed. Great work! Thanks a lot!

@tobiolo
Copy link
Collaborator Author

@tobiolo tobiolo commented on c438af6 Apr 5, 2024

Choose a reason for hiding this comment

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

I just tested Release 8575186124 and the issue is fixed. Great work! Thanks a lot!

You are welcome! :-) And thanks to you for discovering the issue! 👍

Please sign in to comment.