Opened 17 months ago

Closed 16 months ago

Last modified 5 months ago

#18603 closed defect (fixed)

wxDataViewModel::Cleared will not reload model on Linux GTK

Reported by: moraleda Owned by: Olly Betts <olly@…>
Priority: normal Milestone: 3.0.5
Component: wxGTK Version: 3.0.3
Keywords: Cc: jorge.moraleda@…, greebo@…
Blocked By: Blocking:
Patch: yes

Description

In summary wxDataViewModel::Cleared does not appear to work as expected on Linux under GTK. According to the documentation at https://docs.wxwidgets.org/trunk/classwx_data_view_model.html#ab1ce641ec025c4706f878d569a9516f4 a call to Cleared should trigger a model reload, but it just empties the model.

I remark this same issue has been reported in the past for older versions of wxwidgets and the python bindings. I can reproduce the problem with the latest version of wx/Phoenix using the same sample code of the original reports:

Python

Using wxPython's DVC_DataViewModel demo, add self.model.Cleared() at the
end of the __init__ method of TestPanel:

class TestPanel(wx.Panel):
def __init__(self, parent, log, data=None, model=None):
     ...
     self.model.Cleared()

Save changes and restart demo, the control appears empty under Linux GTK (originally reported at https://trac.wxwidgets.org/ticket/16601

C++

The following self contained sample code (originally given in bug report 10942 at http://wxwidgets.10942.n7.nabble.com/wxDataViewCtrl-problem-on-GTK-with-wxDataViewModel-Cleared-td3435.html) demonstrates the problem in C++: Pressing the Redo button should make new items appear but it doesn't.

#include <wx/wx.h>
#include <wx/dataview.h>
#include <wx/hashset.h>
enum Identifiers {
    IDTREEAPP = wxID_HIGHEST + 1,
    IDDATAVIEW = wxID_HIGHEST + 2,
    IDREDO = wxID_HIGHEST + 3
};
WX_DECLARE_HASH_SET(wxString, wxStringHash, wxStringEqual, StringHash);
StringHash strings;
wxStringCharType* asPointer(const wxString s) {
    if (s.length() < 2) {
        return NULL;
    }
    strings.insert(s);
    StringHash::iterator it = strings.find(s);
    assert(it != strings.end());
    wxStringCharType* res = const_cast<wxStringCharType* >(it->wx_str());
    return res;
}
class TestModel: public wxDataViewModel {
public:
    TestModel(): wxDataViewModel(), topic(0) {}
    ~TestModel() {}
    unsigned int GetColumnCount() const {
        return 1;
    }
    wxString GetColumnType(unsigned int column) const {
        return "string";
    }
    void GetValue(wxVariant& val, const wxDataViewItem& item, unsigned int column) const {
        wxVariant v(_asString(item));
        val = v;
    }
    bool SetValue(const wxVariant& val, const wxDataViewItem& item, unsigned int column) {
        return true;
    }
    wxDataViewItem GetParent(const wxDataViewItem& item) const {
        wxString par = _asString(item);
        if (par.length() != 1) {
            return wxDataViewItem(asPointer(par.Left(par.length() - 1)));
        }
        return wxDataViewItem(NULL);
    }
    bool IsContainer(const wxDataViewItem& item) const {
        wxString par = _asString(item);
        return (par.length() < 3);
    }
    unsigned GetChildren(const wxDataViewItem& item, wxDataViewItemArray& children) const {
        wxString par = _asString(item);
        if (topic == 0 or par.length() == 3) {
            return 0;
        }
        children.Add(wxDataViewItem(asPointer(par + "a")));
        children.Add(wxDataViewItem(asPointer(par + "b")));
        children.Add(wxDataViewItem(asPointer(par + "c")));
        return 3;
    }
    void setTopic(int ptopic) {
        topic = ptopic;
        if (topic > 1) {
            Cleared();
        }
    }
    int topic;
private:
    wxString _asString(const wxDataViewItem& item) const {
        if (item.IsOk()) {
            return static_cast<wxStringCharType*>(item.GetID());
        }
        wxStringCharType ch = 'A' + topic;
        return wxString(ch);
    }
};
class TreeFrame: public wxFrame {
public:
    TreeFrame(const wxString& title, const wxPoint& pos, const wxSize& size):
        wxFrame(NULL, IDTREEAPP, title, pos, size) {
        _sizer = new wxFlexGridSizer(1, 3, 3);
        dataView = new wxDataViewCtrl(this, IDDATAVIEW, wxDefaultPosition, wxSize(300, 300), wxDV_NO_HEADER | wxDV_VARIABLE_LINE_HEIGHT);
        wxDataViewTextRenderer* rend0 = new wxDataViewTextRenderer("string", wxDATAVIEW_CELL_EDITABLE);
        _column0 = new wxDataViewColumn("string", rend0, 0, 100, wxAlignment(wxALIGN_LEFT | wxALIGN_TOP), wxDATAVIEW_COL_RESIZABLE);
        dataView->AppendColumn(_column0);
        dataView->SetExpanderColumn(_column0);
        _sizer->Add(dataView);
        _redoButton = new wxButton(this, IDREDO, "Redo");
        _sizer->Add(_redoButton);
        SetSizerAndFit(_sizer);
    }
    void onRedo(wxCommandEvent& event) {
        testModel->setTopic(testModel->topic + 1);
    }
    TestModel* testModel;
    wxDataViewCtrl* dataView;
private:
    wxFlexGridSizer* _sizer;
    wxDataViewColumn* _column0;
    wxButton* _redoButton;
    DECLARE_EVENT_TABLE()
};
BEGIN_EVENT_TABLE(TreeFrame, wxFrame)
    EVT_BUTTON(IDREDO, TreeFrame::onRedo)
END_EVENT_TABLE()
class TreeApp: public wxApp {
public:
    virtual bool OnInit() {
        if (!wxApp::OnInit()) {
            return false;
        }
        _treeFrame = new TreeFrame("Test tree frame", wxPoint(50, 50), wxSize(300, 300));
        SetTopWindow(_treeFrame);
        _treeFrame->testModel = new TestModel;
        _treeFrame->dataView->AssociateModel(_treeFrame->testModel);
        _treeFrame->testModel->setTopic(1);
        _treeFrame->Show(true);
        return true;
    }
private:
    TreeFrame* _treeFrame;
};
IMPLEMENT_APP(TreeApp)

Change History (11)

comment:1 Changed 17 months ago by moraleda

  • Cc jorge.moraleda@… added
  • Resolution set to fixed
  • Status changed from new to closed

I just became aware of commit https://github.com/wxWidgets/wxWidgets/commit/5403ec4e086c4fbc3d9ee9bf4b38964a48357826#diff-202ad52294648f0a41034aea673ce762 that fixes the bug I just reported. I am closing the item.

comment:2 Changed 17 months ago by ojwb

  • Resolution changed from fixed to port to stable
  • Status changed from closed to portneeded

That fix is only in 3.1.3 though, and hasn't been backported to WX_3_0_BRANCH.

It doesn't cherry-pick cleanly, but looks trivial to apply by hand.

I'll open an MR for it when I get a chance.

comment:4 Changed 17 months ago by vadz

  • Milestone set to 3.0.5
  • Patch set

I'm not sure we want to apply this: it doesn't break the ABI, but it does silently change the behaviour in the stable branch. I.e. the existing code might have special workarounds for wxGTK that would be broken by this change. I'm not sure if it's better or worse than the current situation, so if people really want to have this in 3.0, let's do it, but this change should be at the very least prominently mentioned in the change log.

comment:5 Changed 17 months ago by ojwb

By that logic you'd never fix behavioural bugs in a stable branch though!

The documentation here clearly describes a particular behaviour, and that's what you get on other platforms, so not fixing this leaves a trap for people who start using this method after 3.0.5, or are already using it in an app built for other platforms but then port it to wxGTK.

It also seems likely any workaround would simply involve not calling Cleared() but instead calling ItemsChanged() or similar. This only breaks people who are calling Cleared() and somehow relying on it emptying the model under wxGTK, but not on other platforms.

comment:6 Changed 16 months ago by vadz

In my own code the workaround was to actually delete the model items in different places under MSW and GTK, and while I'm too lazy to retest with the old version, I'm pretty sure that changing GTK behaviour to MSW would have broken it badly (as in made it crash due to dereferencing a freed pointer). And I'm worried that this may happen to the others too.

But, again, if you think it's fine and if nobody else objects to it, let's apply it. Could you please update the "incompatible changes" section of docs/changes.txt to mention it? TIA!

comment:7 Changed 16 months ago by ojwb

But, again, if you think it's fine and if nobody else objects to it, let's apply it.

I wouldn't say "fine", but overall making it behave as documented cross-platform seems a improvement to me. I've not used this API myself though, so I'm not really following how this would end up with a crash, but if you write code that crashes if a feature works as documented, you shouldn't be surprised when your code crashes.

Not fixing this means penalising people who believe the documentation - if this doesn't get backported, I think the documentation needs updating to explain the situation.

Could you please update the "incompatible changes" section of docs/changes.txt to mention it? TIA!

Done.

comment:8 Changed 16 months ago by Olly Betts <olly@…>

  • Owner set to Olly Betts <olly@…>
  • Resolution changed from port to stable to fixed
  • Status changed from portneeded to closed

In d56d17ad1/git-wxWidgets:

Make Cleared() behave consistently with the other ports in wxGTK

Instead of actually deleting all the items from the control, just
refresh it by resetting the model, as this is what Cleared() does in
both the generic and native macOS versions of wxDataViewCtrl, so calling
it there resulted in very different results from doing it under wxGTK,
where instead of refreshing the control contents it raelly cleared it.

The name of this method is unfortunately confusing, but it seems better
to change its behaviour in wxGTK, even if this doesn't match the name,
rather than change it in the other ports to make them do the same thing,
as this could break some currently working code.

Also, this change results in a welcome code simplification.

Fixes #18603.

(cherry picked from commit 5403ec4e086c4fbc3d9ee9bf4b38964a48357826)

comment:9 Changed 5 months ago by codereader

  • Cc greebo@… added

I'm running into a crash after the above change to the wxDataViewModel::Cleared() implementation in wxGTK. Ubuntu 20.10 ships wx3.0.5 by default, and my app is crashing now.

My wxDataViewModel has a Clear() method which looks like this:

void TreeModel::Clear()
{
	_rootNode->values.clear();
	_rootNode->children.clear();
	wxDataViewModel::Cleared(); // <<-- triggers callbacks to GetParent()
}

I'm removing and free'ing all the models except for my root node, and then call Cleared() which will call "return BeforeReset() && AfterReset();" in wxGTK. In "BeforeReset" there is a call to gtk_tree_view_set_model(NULL) which fires some wx/GTK-internal callbacks and then ends up calling my wxDataViewModel::GetParent() passing a wxDataViewItem that refers to a node that has already been freed => segfault.

Am I supposed to do this differently?

It know it didn't crash in wxGTK 3.0.4 and it doesn't crash in wxMSW 3.1.3 (no callbacks happening in 3.1.3).

And apologies if this is the wrong place to mention this.

comment:10 Changed 5 months ago by ojwb

Does your app crash if you use wxGTK 3.1.4?

comment:11 Changed 5 months ago by codereader

I just checked, compiling from the 3.1.4 sources: no, it doesn't crash in wxGTK 3.1.4.

Note: See TracTickets for help on using tickets.