https://bugs.kde.org/show_bug.cgi?id=525066
--- Comment #2 from Piotr Osada <[email protected]> --- For the record, the masking chain itself is fine, so the defect is not in the masking code: AbstractColumnSetMaskedCmd::redo() and undo() both end with m_col->owner()->setDataChanged() (src/backend/core/abstractcolumncommands.cpp:153 and :161), setDataChanged() invalidates the cached statistics before emitting (src/backend/core/AbstractColumn.cpp:569-573), the statistics computation does skip masked rows (src/backend/core/column/ColumnPrivate.cpp:3678), and SpreadsheetModel listens to Column::maskingChanged as well as Column::dataChanged (src/backend/spreadsheet/SpreadsheetModel.cpp:423). AI research at master, commit 61931ced3 The broken link is the connection between the StatisticsSpreadsheet and its parent spreadsheet. StatisticsSpreadsheet subscribes to the *view model* once, in its constructor: // src/backend/spreadsheet/StatisticsSpreadsheet.cpp:92-98 auto* model = m_spreadsheet->model(); connect(model, &SpreadsheetModel::dataChanged, this, &StatisticsSpreadsheet::update); connect(model, &SpreadsheetModel::rowsRemoved, this, &StatisticsSpreadsheet::update); ... but Spreadsheet::model() is a plain getter that never creates the model: // src/backend/spreadsheet/Spreadsheet.cpp:155-157 SpreadsheetModel* Spreadsheet::model() const { return m_model; } The model is instantiated only by the frontend, when the spreadsheet view is opened for the first time: // src/frontend/spreadsheet/SpreadsheetView.cpp:143-150 // the creation of the model is done here since it's only required for the view m_model = m_spreadsheet->model(); if (!m_model) m_model = new SpreadsheetModel(m_spreadsheet); Consequence: when the StatisticsSpreadsheet is created while the parent spreadsheet has no view yet, model() returns nullptr, all six connect() calls are no-ops, and the statistics spreadsheet is never refreshed again in that session. This is exactly what happens when a project is loaded from a file: // src/backend/spreadsheet/Spreadsheet.cpp:1559-1565 (load path, loading = true) d->statisticsSpreadsheet = new StatisticsSpreadsheet(this, true); whereas the interactive path normally works, because the user is looking at the spreadsheet when toggling the feature: // src/backend/spreadsheet/Spreadsheet.cpp:1472 (toggleStatisticsSpreadsheet) d->statisticsSpreadsheet = new StatisticsSpreadsheet(this); The connection is never re-established later: Spreadsheet::setModel() is called only from the SpreadsheetModel constructor (src/backend/spreadsheet/SpreadsheetModel.cpp:77), and StatisticsSpreadsheet::load() does not touch the model. Opening the spreadsheet view after loading creates the model, but the statistics spreadsheet keeps its dead subscription. Scope of the bug This is not specific to masking, and most probably not specific to error bars either: after loading a project, *any* data change in the parent spreadsheet (editing a cell, removing/inserting rows, changing the column mode) leaves the statistics spreadsheet stale, and consequently every plot fed from it (the bar plot and its error bars in this report) keeps showing the old values. Masking is simply where it was noticed. The apparent asymmetry mentioned in the original report ("removing the mask recalculates correctly") also follows from this: with the subscription dead, the statistics keep the values computed for the full data set, so after unmasking they happen to be correct again. Why it correlates with the project structure What decides whether the statistics spreadsheet works is whether the parent spreadsheet's *view* already existed at the moment the StatisticsSpreadsheet object was constructed. Views are not created for the whole project: the dock is created for the aspect the user actually selects, in MainWin::activateSubWindowForAspect() (src/frontend/MainWin.cpp:1435ff), and ContentDockWidget's constructor calls part->view() (src/frontend/core/ContentDockWidget.cpp:27-31); the only other source is the <view> entries of the saved project state (src/frontend/ProjectExplorer.cpp: 1302-1346). On top of that, the default dock visibility policy is folderOnly (src/backend/core/Project.cpp:152), the current folder starts as the project root (src/frontend/MainWin.cpp:521), and MainWin::handleCurrentAspectChanged() (:1414-1433) hides the docks of parts outside the current folder as soon as the selection moves elsewhere. The practical effect is that a spreadsheet in the project root has almost certainly been opened at some point in the session, so its model exists, while one inside a subfolder or a workbook may never have been opened - which matches the observation that masking recalculates in the root folder but not in a subfolder. (For a spreadsheet inside a Workbook, AbstractPart::dockWidget() returns the *workbook's* dock, src/backend/core/AbstractPart.cpp:60-65, so its SpreadsheetView depends on the workbook's tab handling.) How to reproduce v1 * Start labplot from a terminal and load a project containing a statistics spreadsheet - Qt prints "QObject::connect: invalid nullptr parameter" at load time. * With the same project loaded, simply edit an ordinary (unmasked) cell value: the statistics spreadsheet does not update either. * Create the statistics spreadsheet interactively in a fresh session with the spreadsheet view open: masking then updates the statistics and the plot correctly. How to reproduce v2 * Load a project containing a statistics spreadsheet whose parent spreadsheet is not opened automatically. Qt should print "QObject::connect: invalid nullptr parameter" at load time (start labplot from a terminal to see it). * With that project loaded, edit an ordinary, *unmasked* cell: the statistics spreadsheet does not update either - the breakage is not specific to masking. * Decisive test, which is also the workaround: 1. open the spreadsheet's view (double-click it) - this creates the model; 2. mask a cell - still not recalculated, because the connections are only made in the StatisticsSpreadsheet constructor; 3. toggle "Column Statistics" off and on - the object is re-constructed, now with a live model; 4. mask again - it recalculates. Step 2 failing while step 4 works is the signature of this bug. [AI Suggested fix] ==================v1 Minimal: make the getter create the model on demand, so the backend no longer depends on a view having been opened: SpreadsheetModel* Spreadsheet::model() { if (!m_model) m_model = new SpreadsheetModel(this); // its ctor calls setModel(this) return m_model; } ==================v2 // src/frontend/spreadsheet/SpreadsheetView.cpp:170-176 (SpreadsheetView::init) // create a new SpreadsheetModel if not available yet. // the creation of the model is done here since it's only required // for the view but its lifecycle is managed in Spreadsheet, // i.e. the deletion of the model is done in the destructor of Spreadsheet m_model = m_spreadsheet->model(); if (!m_model) m_model = new SpreadsheetModel(m_spreadsheet); return m_model; } Cleaner (removes the backend -> frontend dependency the code comment itself points out): let StatisticsSpreadsheet subscribe directly to the backend signals of the parent spreadsheet and its columns (Column::dataChanged, Column::maskingChanged, aspectAdded/aspectRemoved, rowsInserted/rowsRemoved) instead of to the view model. -- You are receiving this mail because: You are watching all bug changes.
