Q_ASSERT(!isActive());
This line of code appears several times in mscore/svggenerator.cpp. AFAIK it has been there since the inception of v2.0. What does it do?
Do you still have an unanswered question? Please log in first to post your question.
This line of code appears several times in mscore/svggenerator.cpp. AFAIK it has been there since the inception of v2.0. What does it do?
Do you still have an unanswered question? Please log in first to post your question.
Comments
It crashes when the condition is met
In reply to It crashes when the… by Jojo-Schmitz
Yes, that much I understand, but what is isActive() in this context? Qt doesn't let me go to the declaration.
In reply to Yes, that much I understand,… by sideways
http://doc.qt.io/qt-5/qtimer.html#isActive
In reply to http://doc.qt.io/qt-5/qtimer… by Jojo-Schmitz
What timer? I don't see any timer on this class.
I think this can be removed from these places in this file. This file, svggenerator.cpp, was originally generated by Qt, or so I believe.
Should I leave these Q_ASSERTs intact, or is it OK to delete them? I have deleted them in my working branch and see no issues with it. If they are not necessary I'll remove them in a future PR.
Thanks.
In reply to What timer? I don't see any… by sideways
OK, then see http://doc.qt.io/qt-5/qpaintengine.html#isActive
Returns true if the paint engine is actively drawing; otherwise returns false
In reply to OK, then see http://doc.qt… by Jojo-Schmitz
OK, but why should the program crash in this case? And why would the QPaintEngine be actively drawing something else at the same time? It seems like a distant possibility that the same QPaintEngine instance would be drawing something else at the same time. That would be a programming error. Maybe that's what the QASSERT is designed to catch, but it's not necessary because of the way the code that calls svggenerator.cpp's Paint Engine is designed.
In reply to OK, but why should the… by sideways
And only in DEBUG mode, in RELEASE mode this collapses to a NOP anyway.
In reply to OK, but why should the… by sideways
Catching programming errors is indeed the whole point of assert statements - when you think things have been designed in such a way that something could never happen, but something is making you less than 100% sure. Adding an assert causes an error message to be printed to the console if it ever happens, so you don't even need to be running under the debugger to know exactly what happened, thus allowing anyone running that build to help you determine if such cases exist, whether they know the code or are looking for this error or not. As mentioned, the abort happens only for debug builds, so in real life, no crash happens. But since the code is presumably not designed to handle that case gracefully, chances are often good that a crash will ensue at some point soon anyhow.
In reply to Catching programming errors… by Marc Sabatella
I understand. In this case I question the need for it. I don't see the non-fatal error that might be caused. This assert seems to testing to see if the same instance of the paint engine object is actively printing something else right now. If you are running in the most async, multi-threaded situation possible, the code could crash because it tries to stream into the same buffer from two sources at once. But this is only preventing you from setting paint engine page-level properties, not actually printing. I have never seen any issue with this over the course of hundreds of SVG Exports I've run in the past few years. AFAIK there is zero possibility of two separate SVG Export processes for separate pages running at the same time. Each page completes before the next begins. It is a separate instance of the paint engine for each page:
https://github.com/musescore/MuseScore/blob/dd82ee5124084af6233b7df42f5…
I'm going to leave it alone in the master, but I believe it can be removed. I am removing it in my code, in the several places that it appears. IMO it's misleading and confusing to leave it there for future coders, just a waste of their time trying to figure out what it does. I just invented that wheel. There should be no need to reinvent it.
In reply to I understand. In this case… by sideways
https://github.com/musescore/MuseScore/commit/a9897a2286fd2c0775a552634…
That's the link to the PR where printing by page was added. Check with @lasconic and see if he thinks it is possible for these asserts to trigger. Prior to this PR, it printed one big page in one big file. It only created one instance of the paint engine object for each synchronous user export action. Both versions of the code only set these page-level properties once per page. Unless my understanding of isActive() is way off, it is literally impossible for these asserts to trigger the way the code in the caller, file.cpp, is written.
In reply to https://github.com/musescore… by sideways
One more thing to remember: svggenerator.cpp was a template provided by Qt, or something like that. This is legacy Qt code that imagines the possibility of any number of callers accessing the paint engine in any number of ways. MuseScore doesn't do that with SVG. It synchronously exports each page from a single block of code.