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

Continuously backup changes and load them when GRIP is launched #822

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions core/src/main/java/edu/wpi/grip/core/GripFileManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ public class GripFileManager implements FileManager {
public static final File GRIP_DIRECTORY
= new File(System.getProperty("user.home") + File.separator + "GRIP");
public static final File IMAGE_DIRECTORY = new File(GRIP_DIRECTORY, "images");
public static final File BACKUP_FILE = new File(GRIP_DIRECTORY, ".backup.grip");
public static final File LAST_SAVE_FILE = new File(GRIP_DIRECTORY, ".last_save");

@Override
public void saveImage(byte[] image, String fileName) {
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/edu/wpi/grip/core/Pipeline.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.locks.Lock;
Expand Down Expand Up @@ -61,7 +61,7 @@ public class Pipeline implements ConnectionValidator, SettingsProvider, StepInde
*/
private final List<Source> sources = new ArrayList<>();
private final List<Step> steps = new ArrayList<>();
private final Set<Connection> connections = new HashSet<>();
private final Set<Connection> connections = new LinkedHashSet<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment explaining the specific use of LinkedHashSet here because clearly it matters now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually going to revert this. Just looking at the last modified time now.

@Inject
@XStreamOmitField
private transient EventBus eventBus;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@
*/
public interface DirtiesSaveEvent {

DirtiesSaveEvent DIRTIES_SAVE_EVENT = new DirtiesSaveEvent() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are fields on interfaces automatically final?

Copy link
Member Author

@SamCarlberg SamCarlberg Feb 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fields declared in interfaces are always public static final

@Override
public boolean doesDirtySave() {
return true;
}
};

/**
* Some events may have more logic regarding whether they make the save dirty or not.
*
Expand Down
52 changes: 47 additions & 5 deletions core/src/main/java/edu/wpi/grip/core/serialization/Project.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package edu.wpi.grip.core.serialization;

import edu.wpi.grip.core.GripFileManager;
import edu.wpi.grip.core.Pipeline;
import edu.wpi.grip.core.PipelineRunner;
import edu.wpi.grip.core.events.DirtiesSaveEvent;
Expand All @@ -8,6 +9,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.eventbus.EventBus;
import com.google.common.eventbus.Subscribe;
import com.google.common.io.Files;
import com.google.common.reflect.ClassPath;
import com.thoughtworks.xstream.XStream;
import com.thoughtworks.xstream.annotations.XStreamAlias;
Expand All @@ -22,11 +24,15 @@
import java.io.Reader;
import java.io.StringReader;
import java.io.Writer;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.LinkedList;
import java.util.List;
import java.util.Optional;
import java.util.function.Consumer;
import java.util.logging.Level;
import java.util.logging.Logger;

import javax.inject.Inject;
import javax.inject.Singleton;

Expand All @@ -36,6 +42,8 @@
@Singleton
public class Project {

private static final Logger logger = Logger.getLogger(Project.class.getName());

protected final XStream xstream = new XStream(new PureJavaReflectionProvider());
@Inject
private EventBus eventBus;
Expand Down Expand Up @@ -74,7 +82,6 @@ public void initialize(StepConverter stepConverter,
} catch (InternalError ex) {
throw new AssertionError("Failed to load class: " + clazz.getName(), ex);
}

});
} catch (IOException ex) {
throw new AssertionError("Could not load classes for XStream annotation processing", ex);
Expand All @@ -88,8 +95,27 @@ public Optional<File> getFile() {
return file;
}

/**
* Sets the current project file to the given optional one. This also updates a file on disk
* so the app can "remember" the most recent save file when it's opened later.
*
* @param file the optional file that this project is associated with.
*/
public void setFile(Optional<File> file) {
file.ifPresent(f -> logger.info("Setting project file to: " + f.getAbsolutePath()));
this.file = file;
try {
if (file.isPresent()) {
Files.write(file.get().getAbsolutePath(),
GripFileManager.LAST_SAVE_FILE,
Charset.defaultCharset());
} else {
// No project file, delete the last_save file
GripFileManager.LAST_SAVE_FILE.delete();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic here doesn't quite make sense in my head. Can you explain this better?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, the LAST_SAVE_FILE stores the full path to the most recently loaded save file. If no file was set, then the LAST_SAVE_FILE is deleted to prevent auto-loading a stale file.

}
} catch (IOException e) {
logger.log(Level.WARNING, "Could not set last file", e);
}
}

/**
Expand All @@ -100,7 +126,7 @@ public void open(File file) throws IOException {
StandardCharsets.UTF_8)) {
this.open(reader);
}
this.file = Optional.of(file);
setFile(Optional.of(file));
}

/**
Expand Down Expand Up @@ -158,11 +184,29 @@ public void save(File file) throws IOException {
this.file = Optional.of(file);
}

/**
* Save the project using a writer to write the data. This will clear the dirty flag.
*
* @param writer the writer to use to save the project
*
* @see #saveRaw(Writer)
*/
public void save(Writer writer) {
this.xstream.toXML(this.pipeline, writer);
saveIsDirty.set(false);
}

/**
* Save the project using a writer to write the data. This has no other side effects.
*
* @param writer the writer to use to save the project
*
* @see #save(Writer)
*/
public void saveRaw(Writer writer) {
xstream.toXML(pipeline, writer);
}

public boolean isSaveDirty() {
return saveIsDirty.get();
}
Expand All @@ -173,9 +217,7 @@ public void addIsSaveDirtyConsumer(Consumer<Boolean> consumer) {

@Subscribe
public void onDirtiesSaveEvent(DirtiesSaveEvent dirtySaveEvent) {
// Only update the flag the save isn't already dirty
// We don't need to be redundantly checking if the event dirties the save
if (!saveIsDirty.get() && dirtySaveEvent.doesDirtySave()) {
if (dirtySaveEvent.doesDirtySave()) {
saveIsDirty.set(true);
}
}
Expand Down
22 changes: 21 additions & 1 deletion ui/src/main/java/edu/wpi/grip/ui/Main.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package edu.wpi.grip.ui;

import edu.wpi.grip.core.CoreCommandLineHelper;
import edu.wpi.grip.core.GripCoreModule;
import edu.wpi.grip.core.GripFileManager;
import edu.wpi.grip.core.GripFileModule;
import edu.wpi.grip.core.PipelineRunner;
import edu.wpi.grip.core.events.UnexpectedThrowableEvent;
Expand All @@ -27,6 +29,9 @@
import org.apache.commons.cli.CommandLine;

import java.io.IOException;
import java.io.Writer;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand Down Expand Up @@ -65,6 +70,7 @@ public class Main extends Application {
@Inject private CVOperations cvOperations;
@Inject private GripServer server;
@Inject private HttpPipelineSwitcher pipelineSwitcher;
@Inject private ProjectBackupLoader backupLoader;
private Parent root;
private boolean headless;
private final UICommandLineHelper commandLineHelper = new UICommandLineHelper();
Expand Down Expand Up @@ -128,6 +134,16 @@ public void start(Stage stage) throws IOException {
Platform.runLater(() -> stage.setTitle(MAIN_TITLE));
}
});
project.addIsSaveDirtyConsumer(dirty -> {
if (dirty) {
try (Writer fw = Files.newBufferedWriter(
GripFileManager.BACKUP_FILE.toPath(), StandardCharsets.UTF_8)) {
project.saveRaw(fw);
} catch (IOException e) {
logger.log(Level.WARNING, "Could not save backup file", e);
}
}
});

stage.setTitle(MAIN_TITLE);
stage.getIcons().add(new Image("/edu/wpi/grip/ui/icons/grip.png"));
Expand All @@ -141,7 +157,11 @@ public void start(Stage stage) throws IOException {
operations.addOperations();
cvOperations.addOperations();

commandLineHelper.loadFile(parsedArgs, project);
if (parsedArgs.hasOption(CoreCommandLineHelper.FILE_OPTION)) {
commandLineHelper.loadFile(parsedArgs, project);
} else {
backupLoader.loadBackupOrPreviousSave();
}
commandLineHelper.setServerPort(parsedArgs, settingsProvider, eventBus);

// This will throw an exception if the port specified by the save file or command line
Expand Down
94 changes: 94 additions & 0 deletions ui/src/main/java/edu/wpi/grip/ui/ProjectBackupLoader.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
package edu.wpi.grip.ui;

import edu.wpi.grip.core.GripFileManager;
import edu.wpi.grip.core.events.DirtiesSaveEvent;
import edu.wpi.grip.core.serialization.Project;

import com.google.common.eventbus.EventBus;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.thoughtworks.xstream.XStreamException;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.util.List;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
* Class handling loading project backups when the app is launched. This improves the UX by letting
* users resume where they left off.
*/
@Singleton
public class ProjectBackupLoader {

private static final Logger logger = Logger.getLogger(ProjectBackupLoader.class.getName());

@Inject
private EventBus eventBus;
@Inject
private Project project;

/**
* Loads the backup project, or the previous save if it's identical to the backup. If there was
* no previous save, the project will have no file associated with it. Does nothing if the backup
* file does not exist.
*/
public void loadBackupOrPreviousSave() {
if (GripFileManager.LAST_SAVE_FILE.exists()) {
// Load whichever is readable and more recent, backup or previous save
try {
File lastSaveFile = lastSaveFile();
if (lastSaveFile != null) {
// Compare last save to backup
if (lastSaveFile.lastModified() >= GripFileManager.BACKUP_FILE.lastModified()) {
// Last save is more recent, load that one
logger.info("Loading the last save file");
project.open(lastSaveFile);
} else if (GripFileManager.BACKUP_FILE.exists()) {
// Load backup, set the file to the last save file (instead of the backup),
// and post an event marking the save as dirty
loadBackup();
project.setFile(Optional.of(lastSaveFile));
eventBus.post(DirtiesSaveEvent.DIRTIES_SAVE_EVENT);
}
} else if (GripFileManager.BACKUP_FILE.exists()) {
// Couldn't read from the last save, just load the backup if possible
loadBackup();
project.setFile(Optional.empty());
}
} catch (XStreamException | IOException e) {
logger.log(Level.WARNING, "Could not open the last project file", e);
}
} else if (GripFileManager.BACKUP_FILE.exists()) {
// Load the backup, if possible
loadBackup();
project.setFile(Optional.empty());
}
}

private File lastSaveFile() throws IOException {
List<String> lines = Files.readAllLines(GripFileManager.LAST_SAVE_FILE.toPath());
if (lines.size() == 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why if there is only one line in the file is it considered valid? I'm confused.

return new File(lines.get(0));
} else {
logger.warning("Unexpected data in last_save file: " + lines);
return null;
}
}

private void loadBackup() {
try {
logger.info("Loading backup file");
project.open(GripFileManager.BACKUP_FILE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this fails won't the pipeline be in a bogus state and be unusable? this could even crash GRIP because the APP was in a bad state.
If this happens then the user can't open GRIP without deleting the load backup file correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welp... guess I'm going to rebase this onto #692, which fixes all of those problems

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, since #692 is pretty big and breaking... maybe leave this as is and have #692 address this specific problem

} catch (XStreamException | IOException e) {
logger.log(Level.WARNING, "Could not load backup file", e);
}
eventBus.post(DirtiesSaveEvent.DIRTIES_SAVE_EVENT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also fire this event if the deserialization fails?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does... That's why the event is after the whole try-catch block.

}

}