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

Option to allow auto save while playing #3088

Merged
merged 3 commits into from Feb 1, 2017

Conversation

@zonkmachine
Copy link
Member

zonkmachine commented Oct 24, 2016

Since most systems have no problems saving a project while playing we could add this as an option.
The problem here is that if you edit a project while playing (or just do very short stops so the autosave doesn't have time enough to start) you may end up not getting a backup for a very long time and you wouldn't know about it.

autosaveagain

@zonkmachine zonkmachine added this to the 1.2.0 milestone Oct 24, 2016
@zonkmachine zonkmachine force-pushed the zonkmachine:autosaveWhileRunning branch from 8b6b3a9 to ec5abd6 Nov 29, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented Dec 3, 2016

@Umcaruje I turned auto save on by default. I'm trying to do the same for the new "running auto save" setting, but inverting button logic in SetupDialog.cpp has a tendency to mess my mind up. All the way up and every time. 💫

@zonkmachine zonkmachine force-pushed the zonkmachine:autosaveWhileRunning branch from 1a68220 to 3e4a9b2 Dec 6, 2016
@zonkmachine zonkmachine removed the in progress label Dec 6, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented Dec 6, 2016

The new LED switch is going to be a bit harder to grey out than the slider.
It may be a bit confusing to leave it on when it will have no function while the auto save is otherwise turned off.

@zonkmachine zonkmachine force-pushed the zonkmachine:autosaveWhileRunning branch from 3e4a9b2 to fa15c31 Dec 13, 2016
@Umcaruje
Copy link
Member

Umcaruje commented Dec 29, 2016

@zonkmachine is this ready for testing?

@zonkmachine
Copy link
Member Author

zonkmachine commented Dec 30, 2016

Yes. As I turned off 'disabling the slider' when auto save is off the action is not as clear as it was previously but it will take some effort to give the same functionality to the 'running auto save button'. Testing and feedback is appreciated.

@zonkmachine zonkmachine force-pushed the zonkmachine:autosaveWhileRunning branch from fa15c31 to 2e32cd5 Jan 29, 2017
@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 29, 2017

All issues solved, ready for review!

As the LED checkbox when disabled still displays it's value, and that's surprisingly confusing, I set the second option invisible instead. Works!
I also tweaked the time label to show "Disabled", when the Auto Save is off.

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 29, 2017

@LMMS/artwork-team Should I move the bottom LED in a bit from the side to show that it's subordinated the Enable LED?

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 29, 2017

Is the wording acceptable?
Capitalization?

autosaveon

autosaveoff

@tresf
Copy link
Member

tresf commented Jan 29, 2017

I would recommend auto-save (while playing). The word "feature" can be removed.

@zonkmachine zonkmachine force-pushed the zonkmachine:autosaveWhileRunning branch from 2e32cd5 to d435e76 Jan 29, 2017
@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 29, 2017

autosaveon

@zonkmachine zonkmachine force-pushed the zonkmachine:autosaveWhileRunning branch from d435e76 to b404109 Jan 29, 2017
@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 29, 2017

Maybe just the word Disabled instead of Auto-save disabled ?

@tresf
Copy link
Member

tresf commented Jan 29, 2017

Maybe just the word Disabled instead of Auto-save disabled ?

If default is on, sure.

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 29, 2017

Cleaner...

autosaveoff

@zonkmachine zonkmachine force-pushed the zonkmachine:autosaveWhileRunning branch from b404109 to 6965774 Jan 29, 2017
@tresf
Copy link
Member

tresf commented Jan 29, 2017

@zonkmachine the only thing that's not immediately intuitive is that the slider enables/disables this feature.

Perhaps the best of both worlds would be to make the slider and the first LED linked, or labels under the tick marks suggesting. My opinion is that auto-save should be on, always with no option to disable. If there are performance problems, we make those priority but I really don't understand why anyone wouldn't want a recovery option. Telling the software how many seconds/minutes is IMO too granular for the average user. That's my 2 cents.

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 29, 2017

the slider enables/disables this feature.

It doesn't for me and it isn't supposed to.

@tresf
Copy link
Member

tresf commented Jan 29, 2017

OK I'm confused then. You've replaced a check box with a label so I'm not sure what's going on.

@tresf
Copy link
Member

tresf commented Jan 29, 2017

Or are you hiding the child check box?

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 29, 2017

I'll look into it.

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 30, 2017

Or are you hiding the child check box?

The Allow auto-save while playing check box is hidden when Auto save is disabled. I do this because setEnabled() doesn't grey out the check box or indicate in any way that somethings changed apart from making the check box not responding on button clicks. I thought that's a bit confusing so I hide it instead.

@tresf
Copy link
Member

tresf commented Jan 30, 2017

@zonkmachine thanks for the explanation. That makes sense.

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 30, 2017

My opinion is that auto-save should be on, always with no option to disable.

I agree on having auto-save always on. I think the best take on it is what @jasp00 suggests in this comment from issue 181. Then we wouldn't need to bother about saving in a lower priority thread and can just drop the auto-save settings altogether. But for now I think this is probably the best option.

If there are performance problems, we make those priority...

One point in doing it this way is that it becomes manageable for someone who isn't an experienced coder like me. Performance was one of the first things I looked at when I started working on the auto-save function but I dropped it pretty quickly. And fundamentally changing how it's done would take someone more experienced and there are more important things to take on.

@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 1, 2017

I'm happy with this now. Anything else to be done here?

@zonkmachine zonkmachine merged commit c54171b into LMMS:master Feb 1, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zonkmachine zonkmachine deleted the zonkmachine:autosaveWhileRunning branch Feb 1, 2017
Copy link
Member

jasp00 left a comment

This request is conceptually wrong.

@@ -124,7 +124,8 @@ SetupDialog::SetupDialog( ConfigTabs _tab_to_open ) :
#endif
m_backgroundArtwork( QDir::toNativeSeparators( ConfigManager::inst()->backgroundArtwork() ) ),
m_smoothScroll( ConfigManager::inst()->value( "ui", "smoothscroll" ).toInt() ),
m_enableAutoSave( ConfigManager::inst()->value( "ui", "enableautosave" ).toInt() ),
m_disableAutoSave( !ConfigManager::inst()->value( "ui", "disableautosave" ).toInt() ),
m_disableRunningAutoSave( !ConfigManager::inst()->value( "ui", "disablerunningautosave" ).toInt() ),

This comment has been minimized.

Copy link
@jasp00

jasp00 Feb 4, 2017

Member

Double negations should be avoided; "enable" should be preferred over "disable". This code does not make sense, because it looks like:

m_disableAutoSave = !disableautosave
m_disableRunningAutoSave = !disablerunningautosave

This comment has been minimized.

Copy link
@zonkmachine

zonkmachine Feb 6, 2017

Author Member

Thanks! I'm looking at clearing this up now.

This comment has been minimized.

Copy link
@zonkmachine

zonkmachine Feb 6, 2017

Author Member

fixed

m_runningAutoSave = new LedCheckBox(
tr( "Allow auto-save while playing" ), auto_save_tw );
m_runningAutoSave->move( 20, 90 );
m_runningAutoSave->setChecked( m_disableRunningAutoSave );

This comment has been minimized.

Copy link
@jasp00

jasp00 Feb 4, 2017

Member

Same problem here:
runningAutoSave = disableRunningAutoSave

This comment has been minimized.

Copy link
@zonkmachine

zonkmachine Feb 6, 2017

Author Member

fixed

ConfigManager::inst()->setValue( "ui", "enableautosave",
QString::number( m_enableAutoSave ) );
ConfigManager::inst()->setValue( "ui", "disableautosave",
QString::number( !m_disableAutoSave ) );

This comment has been minimized.

Copy link
@jasp00

jasp00 Feb 4, 2017

Member

Same problem:
disableautosave = !disableAutoSave

This comment has been minimized.

Copy link
@zonkmachine

zonkmachine Feb 6, 2017

Author Member

fixed

ConfigManager::inst()->setValue( "ui", "saveinterval",
QString::number( m_saveInterval ) );
ConfigManager::inst()->setValue( "ui", "disablerunningautosave",
QString::number( !m_disableRunningAutoSave ) );

This comment has been minimized.

Copy link
@jasp00

jasp00 Feb 4, 2017

Member

Idem.

void SetupDialog::toggleAutoSave( bool _enabled )
{
m_enableAutoSave = _enabled;
m_disableAutoSave = _enabled;

This comment has been minimized.

Copy link
@jasp00

jasp00 Feb 4, 2017

Member

disable means enable, etc.
What is the reason?

m_saveIntervalLbl->setText( tr( "Auto save interval: %1 %2" ).arg(
QString::number( m_saveInterval ), minutes ) );
minutes = QString( "%1 %2" ).arg( QString::number( m_saveInterval ), minutes );
minutes = m_disableAutoSave ? minutes : tr( "Disabled" );

This comment has been minimized.

Copy link
@jasp00

jasp00 Feb 4, 2017

Member

If not disable, it is disabled.

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.

None yet

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