JAL-3141 Backup files being created with default suffix up to 10 backups...

Activity

CR-JAL-172 33

Keyboard shortcuts  
  • Summarize the review outcomes (optional)
     
    #permalink

    Details

    Warning: no files are visible, they have all been filtered.
    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  
    #permalink

    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

    #permalink

    Issues Raised From Comments

    Key Summary State Assignee
    #permalink

    General Comments

    Jim Procter

    Nice idea to implement rolling backup in the FileChooser - but that isn't goi...

    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..)

    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?

    Jim Procter

    sounds good.

    sounds good.

    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).

    /examples/backupfilestest.fa Added
    Open in IDE #permalink
    /resources/lang/Messages.properties Changed 1
    Open in IDE #permalink
    /resources/lang/Messages_es.properties Changed
    Open in IDE #permalink
    /src/jalview/bin/Cache.java Changed
    Open in IDE #permalink
    /src/.../types/ColourThreshTypeType.java Deleted
    Open in IDE #permalink
    /src/.../types/FeatureMatcherByType.java Deleted
    Open in IDE #permalink
    /src/jalview/.../types/NoValueColour.java Deleted
    Open in IDE #permalink
    /src/jalview/gui/AlignFrame.java Changed
    Open in IDE #permalink
    /src/jalview/gui/Desktop.java Changed
    Open in IDE #permalink
    /src/jalview/gui/FeatureSettings.java Changed
    Open in IDE #permalink
    /src/.../gui/JalviewBooleanRadioButtons.java Added 8
    Open in IDE #permalink
    /src/jalview/gui/Preferences.java Changed
    Open in IDE #permalink
    /src/jalview/io/BackupFilenameFilter.java Added 1
    Open in IDE #permalink
    /src/jalview/io/BackupFilenameParts.java Added
    Open in IDE #permalink
    /src/jalview/io/BackupFiles.java Added 14
    Open in IDE #permalink
    /src/jalview/io/JalviewFileChooser.java Changed
    Open in IDE #permalink
    /src/jalview/io/VamsasAppDatastore.java Changed
    Open in IDE #permalink
    /src/jalview/jbgui/GDesktop.java Changed
    Open in IDE #permalink
    /src/jalview/jbgui/GPreferences.java Changed 1
    Open in IDE #permalink
    /src/jalview/project/Jalview2XML.java Added
    Open in IDE #permalink
    /src/castor.properties Deleted
    Open in IDE #permalink
    /test/jalview/gui/AlignFrameTest.java Changed
    Open in IDE #permalink
    /test/jalview/io/AnnotationFileIOTest.java Changed
    Open in IDE #permalink
    /test/jalview/io/BackupFilesTest.java Added 3
    Open in IDE #permalink
    /test/jalview/io/CrossRef2xmlTests.java Changed
    Open in IDE #permalink
    /test/jalview/io/Jalview2xmlTests.java Deleted
    Open in IDE #permalink
    /test/jalview/io/testProps.jvprops Changed
    Open in IDE #permalink
    /test/.../jabaws/RNAStructExportImport.java Changed
    Open in IDE #permalink
    /utils/castor-1.1-cycle-codegen-anttask.jar Deleted
    Open in IDE #permalink
    /utils/castor-1.1-cycle-codegen.jar Deleted
    Open in IDE #permalink
    /.ant-targets-build.xml Changed
    Open in IDE #permalink
    /.classpath Changed
    Open in IDE #permalink
    /.gitignore Changed
    Open in IDE #permalink
    /build.xml Changed
    Open in IDE #permalink

    Review updated: Reload | Ignore | Collapse

    You cannot reload the review while writing a comment.

    Log time against