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

SamCarlberg · 2017-02-10T19:05:24Z

No description provided.

JLLeitschuh · 2017-02-10T19:43:19Z

The only problem I forsee is if the backup file turns out to be corrupted or mal-formed then it makes it impossible to load GRIP without deleting this file.

codecov-io · 2017-02-10T21:07:03Z

Codecov Report

Merging #822 into master will decrease coverage by -0.47%.
The diff coverage is 12.5%.

@@             Coverage Diff              @@
##             master     #822      +/-   ##
============================================
- Coverage     52.47%   52.01%   -0.47%     
- Complexity     1133     1137       +4     
============================================
  Files           240      241       +1     
  Lines          7708     7784      +76     
  Branches        522      535      +13     
============================================
+ Hits           4045     4049       +4     
- Misses         3479     3549      +70     
- Partials        184      186       +2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65ab349...3fb9623. Read the comment docs.

SamCarlberg · 2017-02-12T00:20:15Z

@JLLeitschuh
Well this is a pain... different save files of the same project aren't guaranteed to be textually identical, even if they're functionally identical. I want to be able to compare a previous save file to the backup to determine which one to load (backup if it's different from the previous save, otherwise the previous save).

AustinShalit · 2017-02-12T03:36:53Z

Can you use time modified?

SamCarlberg · 2017-02-12T22:13:55Z

Can you use time modified?


Never mind, all changes are automatically saved to the backup files anyway

JLLeitschuh

I like the overall idea however, this sort of thing could expose an underlying design flaw in GRIP where loading a save loads the file into the active pipeline as it runs. This could put GRIP in a bogus state.

A better solution for GRIP would have been to have XStream read in a data model that then gets copied into what is the running pipeline once everything has been validated correctly.

That being said, I think this could work as it is but I'd want to know it had been tested and was resilient to failures.

Have you tried making the backup something that is bogus (for example half the file isn't there because GRIP is closed mid write of the stream).

Also, adding comments where I indicated would be really helpful.

JLLeitschuh · 2017-02-22T18:46:54Z

core/src/main/java/edu/wpi/grip/core/Pipeline.java

@@ -61,7 +61,7 @@
   */
  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<>();


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

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

JLLeitschuh · 2017-02-22T18:47:30Z

core/src/main/java/edu/wpi/grip/core/events/DirtiesSaveEvent.java

@@ -8,6 +8,13 @@
 */
 public interface DirtiesSaveEvent {

+  DirtiesSaveEvent DIRTIES_SAVE_EVENT = new DirtiesSaveEvent() {


Are fields on interfaces automatically final?

Fields declared in interfaces are always public static final

JLLeitschuh · 2017-02-22T18:50:27Z

core/src/main/java/edu/wpi/grip/core/serialization/Project.java

+            Charset.defaultCharset());
+      } else {
+        // No project file, delete the last_save file
+        GripFileManager.LAST_SAVE_FILE.delete();


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

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.

JLLeitschuh · 2017-02-22T19:28:03Z

ui/src/main/java/edu/wpi/grip/ui/ProjectBackupLoader.java

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


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

JLLeitschuh · 2017-02-22T19:32:31Z

ui/src/main/java/edu/wpi/grip/ui/ProjectBackupLoader.java

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


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?

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

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

JLLeitschuh · 2017-02-22T19:33:06Z

ui/src/main/java/edu/wpi/grip/ui/ProjectBackupLoader.java

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


Should this also fire this event if the deserialization fails?

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

Continuously backup changes and load them when GRIP is launched

08f6007

SamCarlberg requested a review from JLLeitschuh February 10, 2017 19:35

Automatically load last saved file or backup

e4464e4

Move loading backups to dedicated class, improve edge case handling

18bb0f4

Use lastModified timestamp for choosing between last save and backup

3fb9623

JLLeitschuh requested changes Feb 22, 2017

View reviewed changes

SamCarlberg mentioned this pull request Sep 7, 2017

prompt to save layout when closing wpilibsuite/shuffleboard#173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SamCarlberg commented Feb 10, 2017

JLLeitschuh commented Feb 10, 2017

codecov-io commented Feb 10, 2017 •

edited

SamCarlberg commented Feb 12, 2017 •

edited

AustinShalit commented Feb 12, 2017

SamCarlberg commented Feb 12, 2017 •

edited

JLLeitschuh left a comment

JLLeitschuh Feb 22, 2017

SamCarlberg Mar 6, 2017

JLLeitschuh Feb 22, 2017

SamCarlberg Feb 22, 2017 •

edited

JLLeitschuh Feb 22, 2017

SamCarlberg May 6, 2017

JLLeitschuh Feb 22, 2017

JLLeitschuh Feb 22, 2017

SamCarlberg Mar 17, 2017

SamCarlberg May 6, 2017

JLLeitschuh Feb 22, 2017

SamCarlberg Feb 22, 2017

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

Are you sure you want to change the base?

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

Conversation

SamCarlberg commented Feb 10, 2017

JLLeitschuh commented Feb 10, 2017

codecov-io commented Feb 10, 2017 • edited

Codecov Report

SamCarlberg commented Feb 12, 2017 • edited

AustinShalit commented Feb 12, 2017

SamCarlberg commented Feb 12, 2017 • edited

JLLeitschuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamCarlberg Feb 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 10, 2017 •

edited

SamCarlberg commented Feb 12, 2017 •

edited

SamCarlberg commented Feb 12, 2017 •

edited

SamCarlberg Feb 22, 2017 •

edited