Fix opening of project files on macOS #3219

Merged
merged 1 commit into from Mar 18, 2017

Conversation

Projects
None yet
2 participants
@tresf
Member

tresf commented Jan 8, 2017

This is intended to fix the problem where LMMS won't open a project that has been double-clicked on macOS.

Closes #665

Although this fix is Mac-specific, it should have no negative impact on Windows or Linux.

@jasp00

This comment has been minimized.

Show comment
Hide comment
@jasp00

jasp00 Jan 8, 2017

Member
  • What is the best approach to prevent crash?

It looks like you are trying to load the project before the engine is initialized. If Engine::getSong() is null, then delay the loading; you should edit fileToLoad in main.cpp somehow.

  • How is best to incorporate feature into codebase?

You should use a separate file for the class. LmmsApplication or MainApplication would be better names since this the QApplication derivative.

Member

jasp00 commented Jan 8, 2017

  • What is the best approach to prevent crash?

It looks like you are trying to load the project before the engine is initialized. If Engine::getSong() is null, then delay the loading; you should edit fileToLoad in main.cpp somehow.

  • How is best to incorporate feature into codebase?

You should use a separate file for the class. LmmsApplication or MainApplication would be better names since this the QApplication derivative.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jan 8, 2017

Member

It looks like you are trying to load the project before the engine is initialized.

That makes sense.

If Engine::getSong() is null, then delay the loading; you should edit fileToLoad in main.cpp somehow.

I'm not sure the second part of that statement, but it appears we could be missing quite a bit of logic here. Shouldn't this be decoupled from main.cpp and placed into some GUI init function?

lmms/src/core/main.cpp

Lines 714 to 899 in 1f39474

else // otherwise, start the GUI
{
new GuiApplication();
// re-intialize RNG - shared libraries might have srand() or
// srandom() calls in their init procedure
srand( getpid() + time( 0 ) );
// recover a file?
QString recoveryFile = ConfigManager::inst()->recoveryFile();
bool recoveryFilePresent = QFileInfo( recoveryFile ).exists() &&
QFileInfo( recoveryFile ).isFile();
bool autoSaveEnabled =
ConfigManager::inst()->value( "ui", "enableautosave" ).toInt();
if( recoveryFilePresent )
{
QMessageBox mb;
mb.setWindowTitle( MainWindow::tr( "Project recovery" ) );
mb.setText( QString(
"<html>"
"<p style=\"margin-left:6\">%1</p>"
"<table cellpadding=\"3\">"
" <tr>"
" <td><b>%2</b></td>"
" <td>%3</td>"
" </tr>"
" <tr>"
" <td><b>%4</b></td>"
" <td>%5</td>"
" </tr>"
" <tr>"
" <td><b>%6</b></td>"
" <td>%7</td>"
" </tr>"
"</table>"
"</html>" ).arg(
MainWindow::tr( "There is a recovery file present. "
"It looks like the last session did not end "
"properly or another instance of LMMS is "
"already running. Do you want to recover the "
"project of this session?" ),
MainWindow::tr( "Recover" ),
MainWindow::tr( "Recover the file. Please don't run "
"multiple instances of LMMS when you do this." ),
MainWindow::tr( "Ignore" ),
MainWindow::tr( "Launch LMMS as usual but with "
"automatic backup disabled to prevent the "
"present recover file from being overwritten." ),
MainWindow::tr( "Discard" ),
MainWindow::tr( "Launch a default session and delete "
"the restored files. This is not reversible." )
) );
mb.setIcon( QMessageBox::Warning );
mb.setWindowIcon( embed::getIconPixmap( "icon" ) );
mb.setWindowFlags( Qt::WindowCloseButtonHint );
QPushButton * recover;
QPushButton * discard;
QPushButton * ignore;
QPushButton * exit;
#if QT_VERSION >= 0x050000
// setting all buttons to the same roles allows us
// to have a custom layout
discard = mb.addButton( MainWindow::tr( "Discard" ),
QMessageBox::AcceptRole );
ignore = mb.addButton( MainWindow::tr( "Ignore" ),
QMessageBox::AcceptRole );
recover = mb.addButton( MainWindow::tr( "Recover" ),
QMessageBox::AcceptRole );
# else
// in qt4 the button order is reversed
recover = mb.addButton( MainWindow::tr( "Recover" ),
QMessageBox::AcceptRole );
ignore = mb.addButton( MainWindow::tr( "Ignore" ),
QMessageBox::AcceptRole );
discard = mb.addButton( MainWindow::tr( "Discard" ),
QMessageBox::AcceptRole );
#endif
// have a hidden exit button
exit = mb.addButton( "", QMessageBox::RejectRole);
exit->setVisible(false);
// set icons
recover->setIcon( embed::getIconPixmap( "recover" ) );
discard->setIcon( embed::getIconPixmap( "discard" ) );
ignore->setIcon( embed::getIconPixmap( "ignore" ) );
mb.setDefaultButton( recover );
mb.setEscapeButton( exit );
mb.exec();
if( mb.clickedButton() == discard )
{
gui->mainWindow()->sessionCleanup();
}
else if( mb.clickedButton() == recover ) // Recover
{
fileToLoad = recoveryFile;
gui->mainWindow()->setSession( MainWindow::SessionState::Recover );
}
else if( mb.clickedButton() == ignore )
{
if( autoSaveEnabled )
{
gui->mainWindow()->setSession( MainWindow::SessionState::Limited );
}
}
else // Exit
{
return 0;
}
}
// first show the Main Window and then try to load given file
// [Settel] workaround: showMaximized() doesn't work with
// FVWM2 unless the window is already visible -> show() first
gui->mainWindow()->show();
if( fullscreen )
{
gui->mainWindow()->showMaximized();
}
if( !fileToLoad.isEmpty() )
{
if( fileToLoad == recoveryFile )
{
Engine::getSong()->createNewProjectFromTemplate( fileToLoad );
}
else
{
Engine::getSong()->loadProject( fileToLoad );
}
}
else if( !fileToImport.isEmpty() )
{
ImportFilter::import( fileToImport, Engine::getSong() );
if( exitAfterImport )
{
return EXIT_SUCCESS;
}
}
// If enabled, open last project if there is one. Else, create
// a new one. Also skip recently opened file if limited session to
// lower the chance of opening an already opened file.
else if( ConfigManager::inst()->
value( "app", "openlastproject" ).toInt() &&
!ConfigManager::inst()->
recentlyOpenedProjects().isEmpty() &&
gui->mainWindow()->getSession() !=
MainWindow::SessionState::Limited )
{
QString f = ConfigManager::inst()->
recentlyOpenedProjects().first();
QFileInfo recentFile( f );
if ( recentFile.exists() )
{
Engine::getSong()->loadProject( f );
}
else
{
Engine::getSong()->createNewProject();
}
}
else
{
Engine::getSong()->createNewProject();
}
// Finally we start the auto save timer and also trigger the
// autosave one time as recover.mmp is a signal to possible other
// instances of LMMS.
if( autoSaveEnabled &&
gui->mainWindow()->getSession() != MainWindow::SessionState::Limited )
{
gui->mainWindow()->autoSaveTimerReset();
gui->mainWindow()->autoSave();
}
}

You should use a separate file for the class. LmmsApplication or MainApplication would be better names since this the QApplication derivative.

Sure but even if it's a single function only used in main.cpp? I find the lack of mid-init overriding in this language to be quite frustrating, all we need is the ::event(...) function. 😐

Member

tresf commented Jan 8, 2017

It looks like you are trying to load the project before the engine is initialized.

That makes sense.

If Engine::getSong() is null, then delay the loading; you should edit fileToLoad in main.cpp somehow.

I'm not sure the second part of that statement, but it appears we could be missing quite a bit of logic here. Shouldn't this be decoupled from main.cpp and placed into some GUI init function?

lmms/src/core/main.cpp

Lines 714 to 899 in 1f39474

else // otherwise, start the GUI
{
new GuiApplication();
// re-intialize RNG - shared libraries might have srand() or
// srandom() calls in their init procedure
srand( getpid() + time( 0 ) );
// recover a file?
QString recoveryFile = ConfigManager::inst()->recoveryFile();
bool recoveryFilePresent = QFileInfo( recoveryFile ).exists() &&
QFileInfo( recoveryFile ).isFile();
bool autoSaveEnabled =
ConfigManager::inst()->value( "ui", "enableautosave" ).toInt();
if( recoveryFilePresent )
{
QMessageBox mb;
mb.setWindowTitle( MainWindow::tr( "Project recovery" ) );
mb.setText( QString(
"<html>"
"<p style=\"margin-left:6\">%1</p>"
"<table cellpadding=\"3\">"
" <tr>"
" <td><b>%2</b></td>"
" <td>%3</td>"
" </tr>"
" <tr>"
" <td><b>%4</b></td>"
" <td>%5</td>"
" </tr>"
" <tr>"
" <td><b>%6</b></td>"
" <td>%7</td>"
" </tr>"
"</table>"
"</html>" ).arg(
MainWindow::tr( "There is a recovery file present. "
"It looks like the last session did not end "
"properly or another instance of LMMS is "
"already running. Do you want to recover the "
"project of this session?" ),
MainWindow::tr( "Recover" ),
MainWindow::tr( "Recover the file. Please don't run "
"multiple instances of LMMS when you do this." ),
MainWindow::tr( "Ignore" ),
MainWindow::tr( "Launch LMMS as usual but with "
"automatic backup disabled to prevent the "
"present recover file from being overwritten." ),
MainWindow::tr( "Discard" ),
MainWindow::tr( "Launch a default session and delete "
"the restored files. This is not reversible." )
) );
mb.setIcon( QMessageBox::Warning );
mb.setWindowIcon( embed::getIconPixmap( "icon" ) );
mb.setWindowFlags( Qt::WindowCloseButtonHint );
QPushButton * recover;
QPushButton * discard;
QPushButton * ignore;
QPushButton * exit;
#if QT_VERSION >= 0x050000
// setting all buttons to the same roles allows us
// to have a custom layout
discard = mb.addButton( MainWindow::tr( "Discard" ),
QMessageBox::AcceptRole );
ignore = mb.addButton( MainWindow::tr( "Ignore" ),
QMessageBox::AcceptRole );
recover = mb.addButton( MainWindow::tr( "Recover" ),
QMessageBox::AcceptRole );
# else
// in qt4 the button order is reversed
recover = mb.addButton( MainWindow::tr( "Recover" ),
QMessageBox::AcceptRole );
ignore = mb.addButton( MainWindow::tr( "Ignore" ),
QMessageBox::AcceptRole );
discard = mb.addButton( MainWindow::tr( "Discard" ),
QMessageBox::AcceptRole );
#endif
// have a hidden exit button
exit = mb.addButton( "", QMessageBox::RejectRole);
exit->setVisible(false);
// set icons
recover->setIcon( embed::getIconPixmap( "recover" ) );
discard->setIcon( embed::getIconPixmap( "discard" ) );
ignore->setIcon( embed::getIconPixmap( "ignore" ) );
mb.setDefaultButton( recover );
mb.setEscapeButton( exit );
mb.exec();
if( mb.clickedButton() == discard )
{
gui->mainWindow()->sessionCleanup();
}
else if( mb.clickedButton() == recover ) // Recover
{
fileToLoad = recoveryFile;
gui->mainWindow()->setSession( MainWindow::SessionState::Recover );
}
else if( mb.clickedButton() == ignore )
{
if( autoSaveEnabled )
{
gui->mainWindow()->setSession( MainWindow::SessionState::Limited );
}
}
else // Exit
{
return 0;
}
}
// first show the Main Window and then try to load given file
// [Settel] workaround: showMaximized() doesn't work with
// FVWM2 unless the window is already visible -> show() first
gui->mainWindow()->show();
if( fullscreen )
{
gui->mainWindow()->showMaximized();
}
if( !fileToLoad.isEmpty() )
{
if( fileToLoad == recoveryFile )
{
Engine::getSong()->createNewProjectFromTemplate( fileToLoad );
}
else
{
Engine::getSong()->loadProject( fileToLoad );
}
}
else if( !fileToImport.isEmpty() )
{
ImportFilter::import( fileToImport, Engine::getSong() );
if( exitAfterImport )
{
return EXIT_SUCCESS;
}
}
// If enabled, open last project if there is one. Else, create
// a new one. Also skip recently opened file if limited session to
// lower the chance of opening an already opened file.
else if( ConfigManager::inst()->
value( "app", "openlastproject" ).toInt() &&
!ConfigManager::inst()->
recentlyOpenedProjects().isEmpty() &&
gui->mainWindow()->getSession() !=
MainWindow::SessionState::Limited )
{
QString f = ConfigManager::inst()->
recentlyOpenedProjects().first();
QFileInfo recentFile( f );
if ( recentFile.exists() )
{
Engine::getSong()->loadProject( f );
}
else
{
Engine::getSong()->createNewProject();
}
}
else
{
Engine::getSong()->createNewProject();
}
// Finally we start the auto save timer and also trigger the
// autosave one time as recover.mmp is a signal to possible other
// instances of LMMS.
if( autoSaveEnabled &&
gui->mainWindow()->getSession() != MainWindow::SessionState::Limited )
{
gui->mainWindow()->autoSaveTimerReset();
gui->mainWindow()->autoSave();
}
}

You should use a separate file for the class. LmmsApplication or MainApplication would be better names since this the QApplication derivative.

Sure but even if it's a single function only used in main.cpp? I find the lack of mid-init overriding in this language to be quite frustrating, all we need is the ::event(...) function. 😐

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jan 8, 2017

Member

OK I think I get this...

We have two separate use-cases.

  1. LMMS is open, just open the file (not a new QApplication)
  2. New QApplication

If I can detect which use-cases is happening...

The ladder simply needs to set a member variable in main.cpp instructing which file to load, right?

Member

tresf commented Jan 8, 2017

OK I think I get this...

We have two separate use-cases.

  1. LMMS is open, just open the file (not a new QApplication)
  2. New QApplication

If I can detect which use-cases is happening...

The ladder simply needs to set a member variable in main.cpp instructing which file to load, right?

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jan 8, 2017

Member

.. or would it be simpler to just loop and wait for the Engine to be non-NULL and hit two birds?

Member

tresf commented Jan 8, 2017

.. or would it be simpler to just loop and wait for the Engine to be non-NULL and hit two birds?

@jasp00

This comment has been minimized.

Show comment
Hide comment
@jasp00

jasp00 Jan 8, 2017

Member

Sure but even if it's a single function only used in main.cpp?

It is about organization. When you see EventApp in a backtrace (or in any source file), it is faster to get to the code with find -name EventApp.cpp than grep -r "EventApp::method(" source_dirs.

Regarding the implementation:

  1. In event(), set m_eventFileToLoad if song is null.
  2. In main.cpp, if fileToLoad is empty, set it to app->eventFileToLoad().
Member

jasp00 commented Jan 8, 2017

Sure but even if it's a single function only used in main.cpp?

It is about organization. When you see EventApp in a backtrace (or in any source file), it is faster to get to the code with find -name EventApp.cpp than grep -r "EventApp::method(" source_dirs.

Regarding the implementation:

  1. In event(), set m_eventFileToLoad if song is null.
  2. In main.cpp, if fileToLoad is empty, set it to app->eventFileToLoad().
@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jan 8, 2017

Member

Won't that require dynamic casting back in main.cpp since app is a QApplication, not a EventApp?

When you see EventApp in a backtrace (or in any source file), it is faster to get to the code with find -name EventApp.cpp than grep -r "EventApp::method(" source_dirs

Fair enough but the code is riddled with nested classes, I assume you wanna keep'm separated moving forward? I just can't bare to add two new files for such a small change, but if that's the way to do it, that's just fine.

Member

tresf commented Jan 8, 2017

Won't that require dynamic casting back in main.cpp since app is a QApplication, not a EventApp?

When you see EventApp in a backtrace (or in any source file), it is faster to get to the code with find -name EventApp.cpp than grep -r "EventApp::method(" source_dirs

Fair enough but the code is riddled with nested classes, I assume you wanna keep'm separated moving forward? I just can't bare to add two new files for such a small change, but if that's the way to do it, that's just fine.

@jasp00

This comment has been minimized.

Show comment
Hide comment
@jasp00

jasp00 Jan 9, 2017

Member

Won't that require dynamic casting ?

Static casting will do since you are running the GUI case.

the code is riddled with nested classes, I assume you wanna keep'm separated moving forward?

Outer classes in a file with a different name are a problem. Nested classes are fine.

I just can't bare to add two new files for such a small change

I understand how you feel, though I would add those files. On the other hand, LMMS is not that big yet. If you decide to keep EventApp in main.cpp, you should keep the class declaration too.

Member

jasp00 commented Jan 9, 2017

Won't that require dynamic casting ?

Static casting will do since you are running the GUI case.

the code is riddled with nested classes, I assume you wanna keep'm separated moving forward?

Outer classes in a file with a different name are a problem. Nested classes are fine.

I just can't bare to add two new files for such a small change

I understand how you feel, though I would add those files. On the other hand, LMMS is not that big yet. If you decide to keep EventApp in main.cpp, you should keep the class declaration too.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jan 9, 2017

Member

If you decide to keep EventApp in main.cpp, you should keep the class declaration too.

You mean the header contents? That's how I had it originally but it was god awful ugly. I'll split it out, it may come in handy for other Mac events or perhaps non-Mac events down the road.

Member

tresf commented Jan 9, 2017

If you decide to keep EventApp in main.cpp, you should keep the class declaration too.

You mean the header contents? That's how I had it originally but it was god awful ugly. I'll split it out, it may come in handy for other Mac events or perhaps non-Mac events down the road.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Mar 14, 2017

Member

I understand how you feel, though I would add those files. On the other hand, LMMS is not that big yet. If you decide to keep EventApp in main.cpp, you should keep the class declaration too.

Done.

Static casting will do since you are running the GUI case.

Done.

In event(), set m_eventFileToLoad if song is null.

Done.

In main.cpp, if fileToLoad is empty, set it to app->eventFileToLoad()

Done, in a slightly different way. If the event file is not empty, we use it.

This is ready for another review. Tested on already running instances as well as newly fired instances. Thanks for the pointers.

Member

tresf commented Mar 14, 2017

I understand how you feel, though I would add those files. On the other hand, LMMS is not that big yet. If you decide to keep EventApp in main.cpp, you should keep the class declaration too.

Done.

Static casting will do since you are running the GUI case.

Done.

In event(), set m_eventFileToLoad if song is null.

Done.

In main.cpp, if fileToLoad is empty, set it to app->eventFileToLoad()

Done, in a slightly different way. If the event file is not empty, we use it.

This is ready for another review. Tested on already running instances as well as newly fired instances. Thanks for the pointers.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Mar 16, 2017

Member

If there are no objections, I will merge this in 2 days.

Member

tresf commented Mar 16, 2017

If there are no objections, I will merge this in 2 days.

Fix opening of project files on macOS
- Double-click mmpz file in finder will now open properly on macOS
- Adds new MainApplication class for listening to QEvent::FileOpenEvent
- Queues open events until the application is ready to recieve them
- Closes #665

@tresf tresf merged commit 3dfd979 into LMMS:master Mar 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tresf tresf deleted the tresf:mac-assoc branch Mar 18, 2017

+ case QEvent::FileOpen:
+ {
+ QFileOpenEvent * fileEvent = static_cast<QFileOpenEvent *>(event);
+ if(event)

This comment has been minimized.

@jasp00

jasp00 Mar 19, 2017

Member

This condition will always be true.

@jasp00

jasp00 Mar 19, 2017

Member

This condition will always be true.

This comment has been minimized.

@tresf

tresf Mar 19, 2017

Member

Can you please explain? Also, why drop the non-obvious fall-through comment?

Never mind, explained here. #3440. Thanks!

@tresf

tresf Mar 19, 2017

Member

Can you please explain? Also, why drop the non-obvious fall-through comment?

Never mind, explained here. #3440. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment