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

Multiple instance recover files #3032

Closed
wants to merge 8 commits into from

Conversation

zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Sep 11, 2016

Fixes #181

Design request. Not ready for code review but I'm curious as to how y'all think the recover file system should behave.
It is currently a working rudimentary implementation of recovery files for multiple sessions.
LMMS is given a unique 8 char hex ID and sticks this to the recover files belonging to the session. On recovery the ID is reused as session ID. The files are sorted so if you have more than one .recover.mmp the system will suggest the most recent for recovery.
Limited Session is dropped and maybe all sessions should be dropped?
Maybe we shouldn't prompt for recover files at all on program launch but just open up a session and if you had a crash there could be an indicator (red R or something) on the MainWindow, maybe superimposed upon the open existing project icon.? Don't know. Ideas?

@zonkmachine zonkmachine changed the title Limited session dropped Multiple instance recover files Sep 11, 2016
@tresf
Copy link
Member

tresf commented Sep 11, 2016

there could be an indicator (red R or something) on the MainWindow, maybe superimposed upon the open existing project icon.? Don't know. Ideas?

This is similar to how Microsoft Word does it, they allow normal operation but put a sidebar with recovery files in it.

I'm not sure a red R on the existing project icon would be that obvious though. Probably better left for a notification area, IMO.

@zonkmachine
Copy link
Member Author

zonkmachine commented Sep 11, 2016

The save files for named/saved projects could use the name somehow. Like:

project-12240c9b.recover.mmp
project-bba2318e.recover.mmp

I'm not sure a red R on the existing project icon would be that obvious though.

Yellow? Green

@tresf
Copy link
Member

tresf commented Sep 11, 2016

green

No, just don't think there's any way to bring attention to any project icons currently as they live in a collapsed sidebar on a collapsed tree.

@zonkmachine
Copy link
Member Author

Notification area is fine with me.

@Umcaruje
Copy link
Member

Could a time stamp be used rather than a session hash? I think a time stamp could be more useful, so the users can easily differentiate between the recover files. Also, when you succesfuly recover a project, does the recover file get removed?

@zonkmachine
Copy link
Member Author

Could a time stamp be used rather than a session hash?

Yes, time stamp sounds good. I think the clock is one second intervals so we would probably have to make it loop until there is sure to be no files with that time already. Tiny odds of that happening but not unimaginable. I like that hash function though... ❤️

Also, when you succesfuly recover a project, does the recover file get removed?

Yes. If you discard/save a recover file it's removed. Next time you launch LMMS it will find the next older recover file if there's more than one.

@zonkmachine
Copy link
Member Author

I think a time stamp could be more useful,

Thinking about it some more... yeah, it probably is. I'll try and scrap the hash later this evening.
💔

@tresf
Copy link
Member

tresf commented Sep 11, 2016

Timestamp is logical but can quickly get bloated. e.g. 2016-09-11 14:00.00

And keep in mind if you have a project open for 3 days, it'll be the day it was opened, not necessarily the day it crashed.

@zonkmachine
Copy link
Member Author

zonkmachine commented Sep 11, 2016

And keep in mind if you have a project open for 3 days, it'll be the day it was opened, not necessarily the day it crashed.

Not if we renew the timestamp on every save.
Edit: Maybe it's just more confusing to have it change name?

@Umcaruje
Copy link
Member

Oh and @zonkmachine please enable autosave on by default, if you didn't do already in this PR. I suffered loss of my work in inkscape just because autosave was off by default, I wouldn't want the same to happen to people in LMMS

@zonkmachine
Copy link
Member Author

zonkmachine commented Sep 11, 2016

please enable autosave on by default...

This is the main theme of #181 . I agree with this.

@Umcaruje
Copy link
Member

Hmm, now that I think about it, session hashes are not bad. If you want to see when the file was saved, you could look into its properties. If we get the timestamp when opening the project, in case of multitple uses of LMMS, this can get confusing.

@tresf
Copy link
Member

tresf commented Sep 11, 2016

Not if we renew the timestamp on every save.

So the plan is to delete and create a new file every second?

@zonkmachine
Copy link
Member Author

if you have a project open for 3 days

I think I misunderstood this. You mean the computer is up and running for three days? In any case I meant updating the timestamp on every auto save, not every second, so the answer is the same in any case. The save function already starts with an intermediate file .new that is later renamed. No big change needed. And I don't really have a plan. It's why I opened this PR at an early stage. I'm happy with the ID tag solution really, and it's working pretty well.

@zonkmachine
Copy link
Member Author

I don't think this will be worth the job. It just blows up the code. If you have that many recover.mmp files hanging around a time stamp won't make it much clearer.

screenshot at 2016-09-12 00 52 52

@jasp00
Copy link
Member

jasp00 commented Sep 12, 2016

the system will suggest the most recent for recovery.

How do you know a recovery file does not belong to a running instance? A second instance should detect the first one and ask its session ID.

@jasp00
Copy link
Member

jasp00 commented Sep 12, 2016

LMMS is given a unique 8 char hex ID and sticks this to the recover files belonging to the session.

Recovery files should be created with:

QTemporaryFile recoveryFile( "XXXXXX.recover.mmp" );
recoveryFile.setAutoRemove( false );

@zonkmachine
Copy link
Member Author

How do you know a recovery file does not belong to a running instance?

😮 I don't?!

A second instance should detect the first one and ask its session ID.

Yes! Does this mean shared memory and semaphores?

Recovery files should be created with:

QTemporaryFile recoveryFile( "XXXXXX.recover.mmp" );
recoveryFile.setAutoRemove( false );

I'll look into it.

@jasp00
Copy link
Member

jasp00 commented Sep 14, 2016

Does this mean shared memory and semaphores?

I would write the session ID in a file named after the process ID. QCoreApplication::applicationPid() should be used for the current process. To find running instances, ps should be run on POSIX, and tasklist on Windows.

@tresf
Copy link
Member

tresf commented Feb 5, 2017

@jasp00 albeit uglier cosmetically, can't we just create a static QUuid at launch and use that as the file name? It seems like it would prevent us from hitting any ps | tasklist | foo complications.

@jasp00
Copy link
Member

jasp00 commented Feb 13, 2017

can't we just create a static QUuid at launch and use that as the file name?

Using QUuid is like using QTemporaryFile. The problem is the same, how do you know a UUID does not belong to a running instance?

@tresf
Copy link
Member

tresf commented Feb 13, 2017

how do you know a UUID does not belong to a running instance?

Can you or @zonkmachine expand on this question? What's wrong with a QTemporaryFile? Each process should own its own file, I don't see the need for complexities, I must be missing something?

@zonkmachine
Copy link
Member Author

I will look into QTemporaryFile and QUuid .

@jasp00
Copy link
Member

jasp00 commented Feb 15, 2017

What's wrong with a QTemporaryFile?

Nothing, it is the best solution.

how do you know a UUID does not belong to a running instance?

The problem is how you know whether the recovery file (QUuid, QTemporaryFile, unique 8 char hex ID, etc.) comes from a crashed session or it is still being used. Actually, ps is not needed, just lock files. There is QLockFile since Qt 5.1; perhaps setStaleLockTime(0) should be called.

@tresf
Copy link
Member

tresf commented Feb 15, 2017

Can the lock file be dual-purpose (mmpz + lock)? Windows allows this, but not sure how POSIX does it. I read somethihg about flock, but I've never tested it. I assume Qt just is a wrapper around the various native techniques?

@jasp00
Copy link
Member

jasp00 commented Feb 15, 2017

Can the lock file be dual-purpose (mmpz + lock)?

Native lock files may allow this, but QLockFile stores information in its lock file.

@zonkmachine
Copy link
Member Author

I wont have time to look into this any further. I suggest that I revert this to only the first commit to remove the limited session which is utterly useless, merge that into stable-1.2, and then someone else can fix the multiple instance recover files for 1.3 ?

@zonkmachine
Copy link
Member Author

Limited Session is dropped in #3545 and I close this PR for now.

@zonkmachine zonkmachine closed this May 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants