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

Fixes for project loading progress display #3672

Merged
merged 6 commits into from Jul 6, 2017

Conversation

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Jun 29, 2017

This PR currently includes two commits: first one fixes #3611, second one is enhancement mentioned in same issue. I set base branch to stable-1.2 because of #3611 (comment) by @tresf.
It is okay to decouple two commits if needed(but I don't think the decoupling is needed).

@liushuyu This PR contains a new string which needs translation. Once it is merged, please add it to i18n source string list.
see this.

Copy link
Member

@zonkmachine zonkmachine left a comment

I can't properly test f76cb6f as I can't reproduce the issue but it works fine. aee0f2b crashes as commented. You have some lines that go way over the recommended 80 lines. ~170 columns will wrap badly so try and make it shorter.

@@ -124,6 +122,9 @@ void TrackContainer::loadSettings( const QDomElement & _this )
if( node.isElement() &&
!node.toElement().attribute( "metadata" ).toInt() )
{
QString trackName = node.toElement().hasAttribute( "name" ) ? node.toElement().attribute( "name" ) :
node.firstChild().toElement().attribute( "name" );

This comment has been minimized.

@zonkmachine

zonkmachine Jul 3, 2017
Member

125 and 126 too long lines. The coding conventions suggestion for ternary operators is something like this:

QString trackName = node.toElement().hasAttribute( "name" )
            ? node.toElement().attribute( "name" )
            : node.firstChild().toElement().attribute( "name" );
@@ -124,6 +122,9 @@ void TrackContainer::loadSettings( const QDomElement & _this )
if( node.isElement() &&
!node.toElement().attribute( "metadata" ).toInt() )
{
QString trackName = node.toElement().hasAttribute( "name" ) ? node.toElement().attribute( "name" ) :
node.firstChild().toElement().attribute( "name" );
pd->setLabelText( tr("Loading Track %1 (%2/Total %3)").arg( trackName ).arg( pd->value() ).arg( Engine::getSong()->getLoadingTrackCount() ) );

This comment has been minimized.

@zonkmachine

zonkmachine Jul 3, 2017
Member

It crashes here on command line render. If you wrap it in an if, something like this:

if( gui )
{
   pd->setLabelText( tr("Loading Track %1 (%2/Total %3)").arg(
   	trackName ).arg( pd->value() ).arg(
   	Engine::getSong()->getLoadingTrackCount() ) )
}

that should fix it.

Also. The line is much too long and wraps badly. Please try and use more lines.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 3, 2017

Backtrace for crash on command line render

(gdb) bt
#0  QProgressDialog::value (this=0x0) at dialogs/qprogressdialog.cpp:649
#1  0x000000000056882b in TrackContainer::loadSettings (this=0x237dfc0, _this=...) at /home/zonkmachine/builds/lmms/lmms/src/core/TrackContainer.cpp:127
#2  0x0000000000552c0a in SerializingObject::restoreState (this=0x237dfe0, element=...)
    at /home/zonkmachine/builds/lmms/lmms/src/core/SerializingObject.cpp:70
@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Jul 3, 2017

@zonkmachine Okay. I'll change the code and test it again.

@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Jul 3, 2017

pd->setValue( start_val + _this.childNodes().count() );(this line) also generated bugs, so I deleted it.
@zonkmachine I fixed code now. Could you test this again?

Copy link
Member

@zonkmachine zonkmachine left a comment

Some more convention...

m_nLoadingTrack=0;
for( int i=0,n=tclist.count(); i<n; ++i ){
QDomNode nd=tclist.at(i).firstChild();
while(!nd.isNull()){

This comment has been minimized.

@zonkmachine

zonkmachine Jul 3, 2017
Member

I think the general idea is to put the braces on the same column/tab although I'm not 100% sure about the convention around this.


( .. )
{
  this
}

( .. ){
  not this
}
{
++m_nLoadingTrack;
if( nd.toElement().attribute("type").toInt() == Track::BBTrack ){
n += nd.toElement().elementsByTagName("bbtrack").at(0).toElement().firstChildElement().childNodes().count();

This comment has been minimized.

@zonkmachine

zonkmachine Jul 3, 2017
Member

A bit too long line here.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 3, 2017

@zonkmachine I fixed code now. Could you test this again?

Crash fixed!

@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Jul 3, 2017

How about conventions?

Copy link
Member

@zonkmachine zonkmachine left a comment

The rest of the braces too.

for( int i=0,n=tclist.count(); i<n; ++i )
{
QDomNode nd=tclist.at(i).firstChild();
while(!nd.isNull()){

This comment has been minimized.

@zonkmachine

zonkmachine Jul 3, 2017
Member

Here also {

if( nd.isElement() && nd.nodeName() == "track" )
{
++m_nLoadingTrack;
if( nd.toElement().attribute("type").toInt() == Track::BBTrack ){

This comment has been minimized.

@zonkmachine

zonkmachine Jul 3, 2017
Member

And here {

@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Jul 3, 2017

@zonkmachine I checked coding conventions, and it seems to be fine now.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 3, 2017

I've tested this and it looks just fine to me. I can also confirm the fix in f76cb6f as I managed to replicate the issue. For me opening the demoproject 'DnB' most clearly demonstrated issue #3611 .

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 6, 2017

Merge?

@tresf
Copy link
Member

@tresf tresf commented Jul 6, 2017

The code look OK. If it's all tested, please merge @zonkmachine. I assume we've tested against a bunch of different project types and that trackName logic won't blow up. :)

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 6, 2017

I assume we've tested against a bunch of different project types and that trackName logic won't blow up. :)

The logic doesn't blow up, but the loading time does however. I thought I had tested for that already... :(
Here's a comparison against loading Skiessi-Onion.mmpz . I believe lmms-1.0.3 might have loaded faster still. @PhysSong As you can see the loading time increases already with your first commit. This is something people have been worrying about already before this PR.
#2029
#3312

Do you see any possibilities to trim it down a bit?

$ time ../stable-1.1.3/bin/lmms ../data/projects/demos/Skiessi/Skiessi-Onion.mmpz 
real	0m3.499s
user	0m2.296s
sys	0m0.200s

$ time ../stable-1.2.0-RC3/bin/lmms ../data/projects/demos/Skiessi/Skiessi-Onion.mmpz 
real	0m5.042s
user	0m3.864s
sys	0m0.224s

$ time ./lmms-aee0f2b ../data/projects/demos/Skiessi/Skiessi-Onion.mmpz 
real	0m8.044s
user	0m6.656s
sys	0m0.288s

$ time ./lmms-097feb0 ../data/projects/demos/Skiessi/Skiessi-Onion.mmpz
real	0m7.291s
user	0m6.584s
sys	0m0.256s
@tresf
Copy link
Member

@tresf tresf commented Jul 6, 2017

Does commenting out the UI call help?

@tresf
Copy link
Member

@tresf tresf commented Jul 6, 2017

I'm also a bit skeptical of iterating mid-for-loop although I haven't placed a debugger on it to see if it can cause problems.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 6, 2017

No. The commit just before this PR is:

real	0m7.294s
user	0m6.724s
sys	0m0.236s

Somethings happened along the way. I'm trying a bisect.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 6, 2017

@PhysSong Sorry, it looks like false alarm. I compiled without -DCMAKE_BUILD_TYPE=DEBUG and now get the same time for stable-1.2 and PhysSong:project-load. I thought all my builds were with DEBUG but the one I compared against was not. Without debug the load time I get is about:

time ./lmms ../data/projects/demos/Skiessi/Skiessi-Onion.mmpz
real	0m4.552s
user	0m3.868s
sys	0m0.228s

This PR is all good.

@zonkmachine zonkmachine merged commit c6c67b3 into LMMS:stable-1.2 Jul 6, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
PhysSong added a commit to PhysSong/lmms that referenced this pull request Jul 8, 2017
Fix project loading progress jumping back.

Show the name of the track currently being loaded.
PhysSong added a commit to PhysSong/lmms that referenced this pull request Jul 8, 2017
Fix project loading progress jumping back.

Show the name of the track currently being loaded.
@PhysSong PhysSong deleted the PhysSong:project-load branch Sep 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.