Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Might fix 1981 ("Midi Import crash in master branch") #1984

Merged
merged 2 commits into from
Apr 24, 2015

Conversation

michaelgregorius
Copy link
Contributor

This is a pull request for #1981.

The commit adds checks for conditions that are asserted during calls to
get_atom_value.

It might fix a crash that is described in 1981. Unfortunately no files
have been attached to that issue. However, I was able to crash LMMS
using a local file and this crash is gone with this fix. So hopefully this
change also fixes the crashes described in 1981.

This commit adds checks for conditions that are asserted during calls to
get_atom_value.

It might fix a crash that is described in 1981. Unfortunately no files
have been attached to that issue. However, I was able to crash LMMS
using a local file and this crash is gone with this fix. So hopefully this
change also fixes the crashes described in 1981.
{
QString attr = evt->get_attribute();
if( attr == "tracknames" ) {
if( attr == "tracknames" && evt->get_update_type() == 'a' ) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, better to be safe than sorry. I wonder where a trackname event will hold the track name if it doesn't have an atom to read, though.

Print as much debug info as possible for unhandled data.
@michaelgregorius
Copy link
Contributor Author

Hi all,

I have made the changes as proposed by @curlymorphic. The changed code prints as much debug information as possible.

During my tests I have also noticed that strings written with printf are not flushed immediately and that I had to close LMMS to see the debug information when I used my test file. Wouldn't it make sense to write such information using std::cerr (which is flushed immediately to my knownledge)? I guess this would also be more in the C++ way of doing things while printf is C like.

@curlymorphic
Copy link
Contributor

There appears to be no real consistency in lmms on this. in some places printf is used, in others qDebug. I find the if the printf string is terminated with a \n it flushes immediately.

@Wallacoloo
Copy link
Member

If possible, I think it's better to use the qDebug() family of functions
over printf, as these can easily be hooked, which gives us a LOT more
control down the road (e.g. redirect debug info to a log file,
automatically timestamp log events, tag log statements with the calling
thread's id, etc).
On Apr 20, 2015 11:11 AM, "Dave" notifications@github.com wrote:

There appears to be no real consistency in lmms on this. in some places
printf is used in others qDebug. I find the if the printf string is
terminated with a \n it flushes immediately.


Reply to this email directly or view it on GitHub
#1984 (comment).

@tresf
Copy link
Member

tresf commented Apr 24, 2015

have also noticed that strings written with printf are not flushed immediately and that I had to close LMMS to see the debug information when I used my test file.

Can't we safely call flush as often as we like?

-Tres

@michaelgregorius
Copy link
Contributor Author

Can't we safely call flush as often as we like?

I think in the long run a better solution would be one where the programmer does not have to remember that he might need to flush.

I propose to merge this fix and to create a new issue that's about cleaning up the debugging output for the whole code base and to replace it with something coherent, e.g. qDebug() as proposed by @Wallacoloo. What do you think?

@tresf
Copy link
Member

tresf commented Apr 24, 2015

I completely agree. Merging.

tresf added a commit that referenced this pull request Apr 24, 2015
Might fix 1981 ("Midi Import crash in master branch")
@tresf tresf merged commit 478bbbd into LMMS:master Apr 24, 2015
@michaelgregorius michaelgregorius deleted the 1981-midicrash branch June 27, 2015 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants