Deleting curves without disabling tracing first crashes KmPlot.
BUG: 325627
Details
Details
- Reviewers
aacid - Group Reviewers
KDE Edu - Commits
- R334:c57ae7614b29: Disable trace mode in KmPlot before deleting tracing curve from the plot
- Add a Cartesian curve on the plot ( e.g., f(x) = x^3).
- Left-click on it to start tracing.
- Push the "Delete" button under the "Functions" list.
- KmPlot should not crash anymore, the curve should disappear with the corresponding labels on the status bar.
Diff Detail
Diff Detail
- Repository
- R334 KmPlot
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| kmplot/parser.cpp | ||
|---|---|---|
| 660 ↗ | (On Diff #43941) | I've no idea about kmplot so excuse me if what i say makes no sense :D My guess is here you would actually want to compare m_currentPlot.functionId with something from "Function *item"? I mean if you're removing a function that is not the plotted one does the view care? |
| kmplot/parser.cpp | ||
|---|---|---|
| 660 ↗ | (On Diff #43941) | Yes. That would be even better. Thanks. |
Comment Actions
I think it would make more sense if the logic of the code was on View itself and not parser, i.e. parser doesn't care about views, but views care about function it's viewing.
We already have the
emit functionRemoved( id );
in that function so doing something like
connect(XParser::self(), &Parser::functionRemoved, this, &View::functionRemoved)
and then make the functionRemoved function do something similar to the new code you added.
At the end "it's the same", but i think it makes more sense from an architectural point of view.
What do you think?
Comment Actions
I think that
- I cannot find the place for this connect. There is already
connect(XParser::self(), &XParser::functionRemoved, this, &FunctionEditor::functionsChanged);
in functioneditor.cpp.
- If it will be placed in View then View should have to be acknowledged of what to remove and we again should add some public variables to do so even if we do not use them in the case when nothing's going to be removed.
Any ideas? Can we avoid complete rewrite of KmPlot to fix this tiny bug?
Comment Actions
Please disregard the above questions. It takes some time to understand the things as they should be. And thanks for the suggestions.
| kmplot/functioneditor.cpp | ||
|---|---|---|
| 144 ↗ | (On Diff #44701) | Can you try moving this to View() constructor and see if it works? obviously replacing View::self with this (but you knew that) If that works i think it makes more sense there than in functioneditor. It's the View saying "I'm interested in signals from when the parser" than the functioneditor, i think that the view is interested in signals from the parser. Other than that, thanks for accepting my reviews that sometimes are a bit annoying :) |