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

Recover file work over #2176

Merged
merged 1 commit into from Jan 19, 2016

Conversation

Projects
None yet
9 participants
@zonkmachine
Copy link
Member

zonkmachine commented Jul 9, 2015

Fixes issue: #2174
Edit: I've started a feedback thread in the forum https://lmms.io/forum/viewtopic.php?f=15&t=3479

In summary: Multiple fixes for the recovery file system that introduces clear session states for LMMS.
It won't allow overwriting of recover.mmp if you run multiple instances but it will only do autosave for the first instance of LMMS launched.

recovery-final


Bugs and fixes!

  • If you start LMMS with a recovery file present and don't do anything at all the auto save will write it over with an all empty project after a minute. It's been ticking on in the background. We solve this by starting the timer the very last thing in main() after a project is loaded( if it's not a limited session or auto save is disabled altogether ).
  • When starting if you escape from the recover message the default is to delete recover.mmp This is probably not expected or wanted behaviour for everyone. Escaping from the dialogue had better be non destructive.
    I fix this by defaulting 'Escape' to button 'Exit' instead close the app.
  • If you press yes to recover and immediately shut down LMMS you will not be asked any questions and recover.mmp is deleted. This is a bit rough. Fixed in the new file save dialogue. ( MainWindow::mayChangeProject() message box )
  • If you run multiple sessions of LMMS they will simply take turns to write over recover.mmp :P
    I solve this by introducing the limited session, that will be set if the user press 'Ignore' at the recovery dialogue. This allows you to launch another session that doesn't write to recover.mmp. This way LMMS will launch a slightly limited session which simply turns off autosave for this instance. This together with running autosave() the first thing of all (described below) should minimise the risk of recover.mmp being overwritten.
  • On starting LMMS the autosave() function takes a minute to kick in. This is problematic especially since the introduction of different sessions, because this minute of no backup file is actually a logic state of it's own, and it's an ill defined one. We take care of this by executing MainWindow::autosave() in main() once as a very last step after loading a project, but only if there is no recover file present. For this to work I made MainWindow::autosave() public.
  • Clarity. Brushed up the dialogues involved and update window title to signal session.

I know it's not 100% what users have asked for but it is a solid solution for recover files which has no uncertain states and is hopefully clear enough to use. It's intended as a modification/bugfix for the upcoming 1.2 release.
Separate files for multiple sessions as has been in demand will most likely have to wait until later.
The actual action of the MainWindow::autosave() when it's running however will be one of my next areas to look at.

Last updated on 15 Dec 2015

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 12, 2015

undefined reference to `MainWindow::recoverSessionCleanup()' ... ?

Yeah, I didn't think the last one would pass. I need some feedback here.
Everything was rolling along just fine until I got the mighty fine idea to put some commands in a simple function. these QFile::remove commands works fine in the MainWindow::closeEvent by themselves but not when in the function recoverSessionCleanup()

void recoverSessionCleanup()
{
    // delete recover session files
    QFile::remove( ConfigManager::inst()->recoveryFile() );
    QFile::remove( ConfigManager::inst()->recoveryFile() + ".bak" );
    QFile::remove( ConfigManager::inst()->recoverSessionFile() );
}
void MainWindow::closeEvent( QCloseEvent * _ce )
{
    if( mayChangeProject(true) )
    {
        if( isRecoverSession() )
        {
            recoverSessionCleanup();
        }
        _ce->accept();
    }
    else
    {
        _ce->ignore();
    }
}

Travis says:

Linking CXX executable tests

../src/CMakeFiles/lmmsobjs.dir/gui/MainWindow.cpp.o: In function `MainWindow::closeEvent(QCloseEvent*)':

/home/travis/build/LMMS/lmms/src/gui/MainWindow.cpp:1283: undefined reference to `MainWindow::recoverSessionCleanup()'

collect2: ld returned 1 exit status

make[3]: *** [tests/tests] Error 1

make[2]: *** [tests/CMakeFiles/tests.dir/all] Error 2

make[1]: *** [tests/CMakeFiles/tests.dir/rule] Error 2

make: *** [tests] Error 2

@zonkmachine zonkmachine force-pushed the zonkmachine:recoverFileFix branch from 0024a55 to 44a1a4d Jul 12, 2015

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 12, 2015

I squashed it down to two commits and reverted to the last working state by commenting out the function above, recoverSessionCleanup() .
Changes so far are:

  • Forces question on exit with recover.mmp
    Before you could just open and close the recovery file with no question asked and the files would be deleted on exit.
  • Only one instance of LMMS can access recover.mmp
  • Separate question on close for a modified file and a recovered, for clarity.

What doesn't work?
EDIT: FIXED!
If you open a project or create a new default one during a recover session, which forces the right message box, you will now remain in a recover session and be prompted as such at the next message box. The program will also not clear up any of the files involved.

@Wallacoloo

This comment has been minimized.

Copy link
Member

Wallacoloo commented Jul 12, 2015

By the way, it looks like all you need to do is change

void recoverSessionCleanup()
{
[...]
}

to

void MainWindow::recoverSessionCleanup()
{
[...]
}
@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 12, 2015

You mean like ALL the other functions... :-P

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 12, 2015

@Wallacoloo Thanks! Now I have a working patch for the recovery file. It turned out to need a little bit more than the 5 - 10 lines I was hoping for but there are many places to exit an ongoing project and you need to cover all those events.

What I've done is create a file RECOVER which lets other instances know that they don't need to care about recover.mmp. There is also a bool m_recoverSession which we set to make LMMS aware of that it is doing recover work. We can check session state whith 'MainWindow::isRecoverSession()'

  • Forces question on exit with recover.mmp Before you could just open and close the file with no question asked and the files would be deleted on exit. This works if you 'exit the recover session' <by Ctrl + N>, <Ctrl + q>, <Ctrl + o> or with equivalent menu action.
  • Only one instance of LMMS can access recover.mmp
  • Separate question on close for a modified file and a recovered, for clarity.
  • Sett window title to further distinguish session from a new default project ( next commit )

Issues:

  • { FIxed ] If a recover session crash, or is terminated from the command line, RECOVER remains and needs to be manually removed. Maybe a message box on start to check if there really is an ongoing recover session that can then remove the files. I'll work on this next.
  • Has the potential to be annoying...
@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 12, 2015

Like this the user can recover from a locked recover session.

recovermessagebox2

@zonkmachine zonkmachine changed the title [WIP] Always ask upon closing when the file is recovered Always ask upon closing when the file is recovered Jul 12, 2015

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 13, 2015

This is now a working fix for #2174 and is best described here #2176 (comment)
I can't force it into any unwanted states and it takes good care of recover.mmp
If it's annoying to no ends? Don't know, works for me for now.

@tresf

This comment has been minimized.

Copy link
Member

tresf commented Jul 13, 2015

"Open" might be misleading.... As an example, here's what VIM says:

E325: ATTENTION
Found a swap file by the name ".test.txt.swp"
          owned by: tres   dated: Sun Jul 12 20:26:57 2015
         file name: ~tres/test.txt
          modified: no
         user name: tres   host name: ubuntu-1204
        process ID: 5934 (still running)
While opening file "test.txt"
             dated: Wed May 14 23:18:12 2014

(1) Another program may be editing the same file.  If this is the case,
    be careful not to end up with two different instances of the same
    file when making changes.  Quit, or continue with caution.
(2) An edit session for this file crashed.
    If this is the case, use ":recover" or "vim -r test.txt"
    to recover the changes (see ":help recovery").
    If you did this already, delete the swap file ".test.txt.swp"
    to avoid this message.

Swap file ".test.txt.swp" already exists!
[O]pen Read-Only, (E)dit anyway, (R)ecover, (Q)uit, (A)bort:

-Tres

@tresf

This comment has been minimized.

Copy link
Member

tresf commented Jul 13, 2015

BTW, I'm not a fan of the RECOVER file. I think we should refrain from using a fixed file name and instead keep our current implementation and fix it rather than adding even more recovery temp files.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 13, 2015

"Open" might be misleading.... As an example, here's what VIM says:

I scavenged among the default buttons. I haven't looked into adding custom buttons to QMessageBox yet,
I wanted button names, Recover, Discard and... well, something else than ignore.

BTW, I'm not a fan of the RECOVER file. I think we should refrain from using a fixed file name and instead keep our current implementation and fix it rather than adding even more recovery temp files.

OK. I'm not too happy about the extra file either but it kind of work. I can just remove it and have a more general message like the one from VIM about the dire consequences of running multiple instances.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 13, 2015

There is a one line fix for this that does exactly what the title proposes.
add Engine::getSong()->setModified(); after loading recover.mmp in main().

This would mean you wouldn't be able to just open and close recover.mmp and it's gone, which was what triggered the bug report in the first place. We could do this for 1.2 and do a more complete fix later. Being in a freeze and all. I think however it's a good idea to really have separate messages.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 13, 2015

Simplified. File RECOVER is gone as is the new Messagebox.


Changes in summary:

  • Forces question on exit with recover.mmp Before you could just open and close the file with no question asked and the files would be deleted on exit. This works if you 'exit the recover session' , , or with equivalent menu action.
  • Separate question on close for a modified file and a recovered one, for clarity.
  • Set window title to further distinguish session from a new default project.
"unsaved and will be lost if you don't "
"Save it. Do you want to save it now?" );

QString messageTitleUnsaved = tr( "Project not saved" );

This comment has been minimized.

@zonkmachine

zonkmachine Jul 16, 2015

Author Member

What happens with the already translated strings "Project not saved" and "The current project..." when I move it to a variable? I didn't change the text. Do I need to do anything to link it to the old string?

This comment has been minimized.

@midi-pascal

midi-pascal Jul 16, 2015

Contributor

Since the strings are translated at creation time by using tr() it should be Ok. You can then copy to an other variable because the content is already translated.
So this should work as expected :)

This comment has been minimized.

@zonkmachine

zonkmachine Jul 16, 2015

Author Member

Cool, thanks! 8)

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 17, 2015

recoverqoriginal

Here's the messagebox in master. I've struck a somewhat odd problem. If you try and close the messagebox with the [x] recover.mmp is discarded. In a basic messagebox the closeEvent() isn't defined so you have to take care of this yourself but in the simpler version of messagebox with default buttons closeEvent() takes care of closing the window just fine but also seem to return the enum of the button with the highest value. OK is lowest...:
http://doc.qt.io/qt-4.8/qmessagebox.html#StandardButton-enum
Edit! just don't use abstractButton solves everything.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 18, 2015

[O]pen Read-Only, (E)dit anyway, (R)ecover, (Q)uit, (A)bort:
What's the difference of quit and abort in this context. One closes the program and the other opens a default empty project?

messagebox2 3

Buttons renamed. I changed 'escape/close' button to a new 'close' button instead of as today, 'no' (discard).
In this sense it works pretty much as it did before but has unwanted responses fixed and a bit more text. Old message is reused however and if the translation isn't updated it should still be pretty clear.

@tresf

This comment has been minimized.

Copy link
Member

tresf commented Jul 18, 2015

"Close" may be a but confusing, as it is often associated with the closing of a dialog...

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 18, 2015

Quit or Abort then?

@tresf

This comment has been minimized.

Copy link
Member

tresf commented Jul 18, 2015

Yeah... I think. Quit, or Quit LMMS.

Quit LMMS seems a bit overkill, but the action is clear. We could use a wildcard to replace LMMS with application name too, if desired.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 18, 2015

I prefer Quit over Quit LMMS then.

We could use a wildcard to replace LMMS with application name too, if desired.

You lost me there. What other app?

@tresf

This comment has been minimized.

Copy link
Member

tresf commented Jul 18, 2015

Oh, I just meant the global name "LMMS" to prevent hard-coding.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 18, 2015

messagebox2 5

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 18, 2015

messagebox2 6

I've reimplemented the possibility to open a limited session without autosave that was lost with the file RECOVER. I'm not happy with the name 'Open' for the button though.
There is one more thing now that's on my mind. autosave() hits in first after a minute and during this time plenty can happen. Like launching a second instance of LMMS. One way to go about this is to trigger autosave() once after setting up the session in main.cpp . I've tried this and it works just fine but I had to make autosave() public. I'm currently looking through the Qt manual to see if it's possible to run the timer much like a do-while loop. Execute immediately first run and then timer on.

@Umcaruje

This comment has been minimized.

Copy link
Member

Umcaruje commented Jul 18, 2015

I'm not happy with the name 'Open' for the button though.

I think 'Ignore' would be a good name for that button.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 18, 2015

I think 'Ignore' would be a good name for that button.

That's better yes.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 18, 2015

messagebox2 7

Yeah, 'Ignore' has the right touch to it. 8)

I'm currently looking through the Qt manual to see if it's possible to run the timer much like a do-while loop.

A no go. I pushed the first version instead.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 18, 2015

Closing the messagebox ( Escape / [x] / Alt+F4 ) right now defaults to 'Quit'. With the new button perhaps Ignore should be the default response?

@zonkmachine zonkmachine changed the title Always ask upon closing when the file is recovered Recover file work over Jul 19, 2015

@zonkmachine zonkmachine force-pushed the zonkmachine:recoverFileFix branch from ff08551 to 3183f92 Dec 10, 2015

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 10, 2015

@zonkmachine this has some merge conflicts that must be resolved prior to merging. Can we also add the state variable as well via #2176 (comment)?

Merge conflicts fixed. I'm looking into the suggested improvements now.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 10, 2015

OK. I swapped the bools for a session state int/enum.

Edit2! The "enum SessionState" commit isn't working (crashes).

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 13, 2015

Edit2! The "enum SessionState" commit isn't working (crashes).

I haven't been able to repeat the crashes I saw. It seem to work as intended. I disabled 'open last project' on limited session as the last project in this case probably was the crashed one and should be opened with recover.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 13, 2015

@tresf @Wallacoloo
I've updated the pull request and ditched the session state bools. I haven't looked into public functions that can be made/kept private. I call most of them both from main.cpp and MainWindow.cpp

@tresf

This comment has been minimized.

Copy link
Member

tresf commented Dec 15, 2015

Thanks. Much cleaner with the enum state. I only had one comment to make. Interested in hearing from @Wallacoloo.

@zonkmachine zonkmachine force-pushed the zonkmachine:recoverFileFix branch from 26e43c7 to 9024f5a Dec 15, 2015

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 15, 2015

Squashed and done. I've updated the Issue/OP.
The tests I devised in #2176 (comment) is still passed.

@zonkmachine zonkmachine force-pushed the zonkmachine:recoverFileFix branch from 9024f5a to bd73c6d Dec 15, 2015

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 15, 2015

Just a last minute tweak. I changed the name of function recoverSessionCleanup() to sessionCleanup() .

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 16, 2015

@tresf I think I'm done here. I've found other things related to the auto save function to work on but I think it's probably better to get this committed and start from a fresh pull request.

@tresf

This comment has been minimized.

Copy link
Member

tresf commented Dec 17, 2015

@zonkmachine much cleaner, thanks.

I'll give some time for final comments prior to merging.

@zonkmachine zonkmachine force-pushed the zonkmachine:recoverFileFix branch from bd73c6d to 9e80207 Jan 19, 2016

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jan 19, 2016

@tresf @Wallacoloo @Umcaruje
I want to move on here and I don't think there is anything else to do on this PR.
One last change though. I reverted one line as I had accidentally changed how the .bak/backup file worked. Not touching that file no more.

@tresf

This comment has been minimized.

Copy link
Member

tresf commented Jan 19, 2016

@zonkmachine sorry for the delay. Merging, adding to 1.2 changelog. 👍

tresf added a commit that referenced this pull request Jan 19, 2016

@tresf tresf merged commit d30a7df into LMMS:master Jan 19, 2016

1 check passed

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

@zonkmachine zonkmachine deleted the zonkmachine:recoverFileFix branch Jan 19, 2016

@zonkmachine zonkmachine restored the zonkmachine:recoverFileFix branch Jan 22, 2016

@zonkmachine zonkmachine deleted the zonkmachine:recoverFileFix branch Jan 22, 2016

@musikBear

This comment has been minimized.

Copy link

musikBear commented Jul 19, 2017

RC3 -An unwanted change has sneaked in. I have several times had lmms 'stagger' during song-editor replay, and if i emediately after such a stagger, look at the recover.mmp, is has that exact timestamp. This mean that the protection against making a recover-file during replay, no longer works in RC3
My chosen recover interval is 10 mins, in Settings.
The unwanted save does not happens regulary, or predictable

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 19, 2017

Thanks, looking into it.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 19, 2017

This mean that the protection against making a recover-file during replay, no longer works in RC3

In RC2 we introduced an option to allow auto-save while playing. The controls were incorrect however.
In RC3 tresf fixed this and it's working correct for me. Could it be that you've simply got the wrong settings?
recover

@musikBear

This comment has been minimized.

Copy link

musikBear commented Jul 20, 2017

Could it be that you've simply got the wrong settings?

🤡 o ..m ..g 🤡 + 😊
Somehow i must have chosen both
Or..
@zonkmachine
Could it be that if autosave + interval already are chosen in the previous version (RC2) that both auto-save-options actually will be default true in RC3?
I doubt i made any setting changes after i upgraded from RC2 to RC3.
-and come to think of it, thats actually a bad test-protocol, because then issues like this one will be obscured!

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 20, 2017

Could it be that if autosave + interval already are chosen in the previous version (RC2) that both auto-save-options actually will be default true in RC3?

No. But the default is that both options are on. In your case you need to set this off every time you have a brand new .lmmsrc.xml

@musikBear

This comment has been minimized.

Copy link

musikBear commented Jul 21, 2017

@zonkmachine I dont like that descission. That feature should not be on in replay as default.
Default should be in idle-mode, and i think default time-setting could be ..4 mins. Looking at the things that gives new users 'problems', has forced me to understand:

  • Nobody likes to explore a new program
  • Absolutely Nobody care to read manuals
    :0)
    So having default autosave in replay, will only lead to Post in both forum and hub of the nature:
    "My LMMS cant replay my song right, all the time"
    🙊
@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jul 23, 2017

@musikBear #3722

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