JAL-3141 Backup files being created with default suffix up to 10 backups...
CR-JAL-172 33
- Details
- Objectives
- General Comments 5
- Unresolved
- Resolved
- Number of files included: 34
-
jalview
0
-
Folder
examples
0
- File backupfilestest.fa 0 Remove
-
Folder
resources/lang
0
- File Messages.properties 1 Remove
- File Messages_es.properties 0 Remove
-
Folder
src
0
-
Folder
jalview
0
-
Folder
bin
0
- File Cache.java 0 Remove
-
Folder
binding/types
0
- File ColourThreshTypeType.java 0 Remove
- File FeatureMatcherByType.java 0 Remove
- File NoValueColour.java 0 Remove
-
Folder
gui
0
- File AlignFrame.java 0 Remove
- File Desktop.java 0 Remove
- File FeatureSettings.java 0 Remove
- File JalviewBooleanRadioButtons.java 8 Remove
- File Preferences.java 0 Remove
-
Folder
io
0
- File BackupFilenameFilter.java 1 Remove
- File BackupFilenameParts.java 0 Remove
- File BackupFiles.java 14 Remove
- File JalviewFileChooser.java 0 Remove
- File VamsasAppDatastore.java 0 Remove
-
Folder
jbgui
0
- File GDesktop.java 0 Remove
- File GPreferences.java 1 Remove
-
Folder
project
0
- File Jalview2XML.java 0 Remove
-
Folder
bin
0
- File castor.properties 0 Remove
-
Folder
jalview
0
-
Folder
test/jalview
0
-
Folder
gui
0
- File AlignFrameTest.java 0 Remove
-
Folder
io
0
- File AnnotationFileIOTest.java 0 Remove
- File BackupFilesTest.java 3 Remove
- File CrossRef2xmlTests.java 0 Remove
- File Jalview2xmlTests.java 0 Remove
- File testProps.jvprops 0 Remove
-
Folder
ws/jabaws
0
- File RNAStructExportImport.java 0 Remove
-
Folder
gui
0
-
Folder
utils
0
- File castor-1.1-cycle-codegen-anttask.jar 0 Remove
- File castor-1.1-cycle-codegen.jar 0 Remove
- File .ant-targets-build.xml 0 Remove
- File .classpath 0 Remove
- File .gitignore 0 Remove
- File build.xml 0 Remove
-
Folder
examples
0
-
Filter
- Only show me content:
- Unfiltered files: dynamically added content
- Filtered files: dynamically added content
- Clear filters
Summarize the review outcomes (optional)
Details
Participant | Role | Time Spent | Comments | Latest Comment |
---|---|---|---|---|
Author | 2h 24m | 10 | exampleLabel (and label.suffix_example_filenames) removed... | |
Reviewer - 96% reviewed | 30m | 3 | sounds good. | |
Total | 2h 55m | 33 |
- Linked Issue:
-
Objectives
- JAL-3141 Backup files being created with default suffix up to 10 backups. Configurable in .jalview_properties but no GUI for this yet.
Branches in review
Repository | Branch to review | Branched from |
---|
General Comments
Ben Soares
I've spotted that Ctrl-S by-passes that (of course it does!) https://source.j...I've spotted that Ctrl-S by-passes that (of course it does!)
That means the backup/version rolling code needs to be more generally placed so I've currently put it in its own class entirely (see next commit). That might seem overkill but see rationale below.
Having re-read your comment in JAL-3141 I can see the suggestion is the opposite numbering to what I implemented. I'm making that a toggle option, with default being your original suggestion (and using "-v%n" as the default suffix instead of "-bak%n").
Rationale for making this a completely separate class is that without a single place where it needs to be implemented I want it to be as simple to call as possible.
Also, ideally I think it would be a good (i.e. safest for the user) idea to write the file to a temp file first, and only if that succeeds perform the backup/version file roll, and then move the saved temp file into place. That means the backup code has to happen in stages:
i) create/allocate temp file name,
ii) data saved into temp file and checked it got written okay (or at least that Java thinks it was),
iii) then perform the backup/version file roll and
iv) move the temp file to the correct filename.
The two benefits are that if writing fails, the backup/version file roll doesn't happen, and even better the original file is untouched so there's no data loss for the user. Downside, requires at least as much space again on the filesystem (as original will be kept while writing).
Step ii) is obviously the PrintWriter etc code that's already there. I've identified three places this happens for saving project files and alignment files and so will implement those first: AlignFrame.saveAlignment(), Jalview2XML.saveAlignment, Jalview2XML.saveState(). Are there other file types of file/places of saving I should be looking at? . iii) and iv) can be lumped together as iv) will happen straight after iii) but I like to think of them separately.
There is a configurable limit on the number of backup/version files kept, so:
1) for versioning the numbers will increase to the maximum, N, and then roll down so file 1 drops off the cliff and you only have the N most recent versions (01 oldest surviving, NN newest-but-one). That does mean that -v01 will not remain constant once all N files are made. Not sure if that's too unintuitive? Without that though I can see a filesystem getting filled quickly, presumably superquick with large alignments.
2) for backup (REVERSE_ORDER flag) the numbering will roll like normal log file rotation (but with leading zeros so they list in a numeric order) from newest (file 01) to oldest (file NN) and the N file will drop off the cliff once it's reached.
I've made the number of digits in %n configurable too, but I daresay that might not make it into the GUI options for simplicity's sake as I'd imagine %02d would be okay for everyone?
-
Ben Soares marked as
Resolved
08 Jan 19
Ben Soares
Made some changes from the code review, and attempted to defend the other cod...Made some changes from the code review, and attempted to defend the other coding decisions!
Ben Soares
exampleLabel (and label.suffix_example_filenames) removed (had been taken out...exampleLabel (and label.suffix_example_filenames) removed (had been taken out to save vertical space in the Preferences pane).
Nice idea to implement rolling backup in the FileChooser - but that isn't going to help if someone hits CTRL-S (save file with current filename), since no file chooser is shown.
Also - rolling backup scheme looks like it is '.1' -> '.2' when a new backup is created, so '.2' is older than '.1'. Is that the case ? I was thinking we would instead increment version numbers (ie v1, v2, v3... where v3 is newer than v2..)
Jim Procter marked as Resolved 29 Oct 18