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

Added AutoSave feature as an enhancement #70

Merged
merged 7 commits into from Jun 12, 2014

Conversation

Projects
None yet
2 participants
@joelmoniz
Copy link
Member

joelmoniz commented Jun 8, 2014

This consists of a pop-up, which includes a "Remember this decision" check-box, that pops-up every time the user tries to run a sketch that hasn't been saved, and gives the user the option of saving and running the sketch, or running it directly without saving.

However, the AutoSave feature has deliberately been left out for an untitled sketch, since untitled sketches are often used for prototyping.

*/
@Override
public void prepareRun() {
super.prepareRun();

This comment has been minimized.

@Manindra29

Manindra29 Jun 9, 2014

Member

I think super method should be called after the user decides to save/not-save and not before?

autoSaveDialog.pack();
autoSaveDialog
.setLocationRelativeTo(base.getActiveEditor());
autoSaveDialog.setVisible(true);

This comment has been minimized.

@Manindra29

Manindra29 Jun 9, 2014

Member

All the autoSaveDialog code(lines 1109-1179) should be in a separate function and not inside prepareRun(). Adding it inside this method makes it look too long. This code can be live outside of prepareRun() I believe?

@joelmoniz

This comment has been minimized.

Copy link
Member

joelmoniz commented Jun 10, 2014

@Manindra29 : Thanks. I have now added a separate autoSave() function. I have also called super.prepareRun() after the user chooses to save/not.

.isSelected();
ExperimentalMode.defaultAutoSaveEnabled = true;
dmode.savePreferences();
}

This comment has been minimized.

@Manindra29

Manindra29 Jun 10, 2014

Member

super.prepareRun(); should be called here.
Are you getting the run -> dialog prompt -> user clicks a button -> only then, sketch starts running sequence with this code?
According to the current implementation, it seems to me, the sketch would start running, while the dialog is still displayed to the user, waiting for his response.

This comment has been minimized.

@joelmoniz

joelmoniz Jun 10, 2014

Member

In the current implementation, I have set the modal boolean of the JDialog to true, which makes it a blocking JDialog and prevents anything further from happening until the user makes a choice (or closes the window).

So basically, it is run -> dialog prompt -> user clicks a button -> only then, sketch starts running.

This comment has been minimized.

@Manindra29

Manindra29 Jun 10, 2014

Member

Ah, missed that. Cool.

ExperimentalMode.defaultAutoSaveEnabled = false;
dmode.savePreferences();
}
autoSaveDialog.dispose();

This comment has been minimized.

@Manindra29

Manindra29 Jun 10, 2014

Member

super.prepareRun(); should be called here too I think.

This comment has been minimized.

@joelmoniz

joelmoniz Jun 10, 2014

Member

Why? Wouldn't it suffice calling prepareRun() once?

This is all that prepareRun() does:

  public void prepareRun() {
    internalCloseRunner(); // Halts current runner
    statusEmpty(); // Clears status area
    for (int i = 0; i < 10; i++) System.out.println(); // advance/clear the terminal window / dos prompt / etc

    // clear the console on each run, unless the user doesn't want to
    if (Preferences.getBoolean("console.auto_clear")) {
      console.clear();
    }

    sketch.ensureExistence();     // make sure the user didn't hide the sketch folder

    sketch.getCurrentCode().setProgram(getText());

  }

This comment has been minimized.

@Manindra29

Manindra29 Jun 10, 2014

Member

No, no need anymore. I'd suggested that before I learnt you have made it modal.

}
});
panel1.add(button);
button = new JButton("Run, Don't Save");

This comment has been minimized.

@Manindra29

Manindra29 Jun 10, 2014

Member

Shouldn't there be two Jbutton objects - one for each button??

This comment has been minimized.

@joelmoniz

joelmoniz Jun 10, 2014

Member

There are: I've created 2 JButton objects, one at line 1143 and another at line 1160.

This comment has been minimized.

@Manindra29

Manindra29 Jun 10, 2014

Member

Please use JButton btnRunSave and JButton btnRunNoSave
It's bad practice to reinitialize the same variable. The object it was pointing to previously loses all references in code.

This comment has been minimized.

@joelmoniz

joelmoniz Jun 10, 2014

Member

Sure. Sorry, my mistake. Should I do this for the panels as well?

This comment has been minimized.

@Manindra29

Manindra29 Jun 10, 2014

Member

Yes, you have initialized panel1 twice.
Thanks

autoSave();
super.prepareRun();
if (wasSaved)
statusTimedNotice("Saved. Running...", 5);

This comment has been minimized.

@Manindra29

Manindra29 Jun 10, 2014

Member

Status messages are unnecessary. handleSave() already displays a status notice.

This comment has been minimized.

@joelmoniz

joelmoniz Jun 10, 2014

Member

The thing is, this notice is cleared out just before the run, since super.prepareRun() is now called after the user makes a choice. Shall I still go ahead and delete the status messages though? It seems like they add a bit of not-so-necessary code (though they may help in informing the user about what's going on).

More changes to auto-save
Removing status notices
@joelmoniz

This comment has been minimized.

Copy link
Member

joelmoniz commented Jun 10, 2014

Ok. I've removed the status notices and the variables are no longer re-initialized.

Please let me know of anything else that needs to be corrected. Thanks

@Manindra29

This comment has been minimized.

Copy link
Member

Manindra29 commented Jun 10, 2014

Yes, this looks much better!
I'll merge it soon 👍

@joelmoniz

This comment has been minimized.

Copy link
Member

joelmoniz commented Jun 10, 2014

Thanks

Manindra29 added a commit that referenced this pull request Jun 12, 2014

Merge pull request #70 from joelmoniz/savePrompt
Added AutoSave feature as an enhancement

@Manindra29 Manindra29 merged commit 0cea05b into processing:master Jun 12, 2014

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