Shutdown previus AutosaveManager and BackupManager during SaveAs #2994

mpele · 2017-07-11T08:57:57Z

Hi,
Could anyone check is everything done correctly in this simple bug fix #2947.

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

updating

lenhard · 2017-07-11T09:10:47Z

Thanks for your take on this! Unfortunately, the current version will not do. If you shutdown the AutoSaveManager and the BackupManager, then there won't be any autosaves or backups anymore and this is not what we want here.

As far as I can see, the way to go would be the following:

Check if the save as operation actually uses a different file path than the old one
If this is true and the save operation is successful, remove the old .sav and .bak files explicitly

mpele · 2017-07-11T09:22:21Z

@lenhard, as I tested it works. After SaveAs new .sav file is created (AutosaveManager and BackupManager are restarted later in lines 351 and 356).

lenhard · 2017-07-11T09:42:24Z

I see, my bad! It sure makes sense to disable the two managers for the old file location.

But the problem that remains is what if the save operation fails? Then, autosave and backup will no longer work for the old file. I still think it would be better to have a cleanup phase after the safe operation has been successful, i.e., shutting done the obsolete managers and removing any unnecessary files.

mpele · 2017-07-14T08:04:23Z

Hi @lenhard,
Is the proposed code acceptable? Right now the shutdown of AutosaveManager and BackupManager is done if the save operation is successful.

lenhard · 2017-07-14T12:18:26Z

I've gone through #2947 and the code seems to solve the problem: The sav file is gone and the user is no longer asked if he wants to restore a backup when opening the old file.

On looking at the code of BackupManager and AutosaveManager, I am a little unsure why we restart them to begin with, but that is not your problem here. So from my side the code is good to go.

We have the policy that two of the developers need to review every pull request before it is merged. So could somebody else from @JabRef/developers have a look at this?

Siedlerchr · 2017-07-14T13:28:31Z

src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

@@ -316,7 +316,8 @@ public void saveAs() throws Exception {
     */
    public void saveAs(File file) throws Exception {
        BibDatabaseContext context = panel.getBibDatabaseContext();
-
+        Path oldFile = context.getDatabasePath().get();


Not your fault: I always wondered why this is an Optional, I think in this case I see no reason why it could be empty, so you are probably on the save side calling directly get.

I think you don't have a database path if the library comes from a SQL server.

Siedlerchr · 2017-07-14T13:28:41Z

CHANGELOG.md

@@ -29,6 +29,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
 - We improved the duplicate checker such that different editions of the same publication are not marked as duplicates. [2960](https://github.com/JabRef/jabref/issues/2960)

 ### Fixed
+- We fixed the a bug that leaves .sav file after SaveAs [#2947]


A little typo: fixed a bug and please inlcude the URL behind like we do with other entries

Siedlerchr

From my point of view it looks good. Just a little improvement in the changelog, but we can add this on merge.

* upstream/master: (30 commits) Add preference migration for keybdingings (#3007) Shutdown previus AutosaveManager and BackupManager during SaveAs (#2994) Run Checkstyle task after Test task (#3010) Mark LibraryOfCongressTest as a fetcher test (#3012) When browsing through the MainTable remember which EntryEditor tab was open (#3011) Improve performance of journal abbreviation loader (#3009) Update checkstyle 7.6.1 -> 8.0 Don't abort build when there are checkstyle violations (#3006) Reimplement content selectors (#3003) Only do a back up for bigger changes or if a different field is edited (#3004) Listen to change events for setting dirty status of database (#3001) Fix #2967: MathSciNet tab works again Fix #2902: Tab in entry editor moves to next text area Fix #2946: external changes are now correctly shown in the entry editor Fix #2998: improve auto completion (#3002) Fix mac keybinding by replacing ctrl it with meta key (#3000) Less backups (#2995) [WIP] Complete rework of the auto completion (#2965) Eclipse J Add switch indentation for Eclipse and add some new missing formatting options ...

* upstream/master: Add preference migration for keybdingings (#3007) Shutdown previus AutosaveManager and BackupManager during SaveAs (#2994) Run Checkstyle task after Test task (#3010) Mark LibraryOfCongressTest as a fetcher test (#3012) When browsing through the MainTable remember which EntryEditor tab was open (#3011) Improve performance of journal abbreviation loader (#3009) Update checkstyle 7.6.1 -> 8.0 Don't abort build when there are checkstyle violations (#3006) Reimplement content selectors (#3003) Only do a back up for bigger changes or if a different field is edited (#3004) Listen to change events for setting dirty status of database (#3001) Fix #2967: MathSciNet tab works again Fix #2902: Tab in entry editor moves to next text area Fix #2946: external changes are now correctly shown in the entry editor Fix #2998: improve auto completion (#3002) Fix mac keybinding by replacing ctrl it with meta key (#3000) Less backups (#2995) [WIP] Complete rework of the auto completion (#2965)

mpele added 3 commits July 10, 2017 23:51

Merge pull request #1 from JabRef/master

c8d66cb

updating

Shutdown previus AutosaveManager and BackupManager

d8b7482

Changelog

150b4d1

lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 11, 2017

closing AutosaveManager and BackupManager for original library

17adc49

Siedlerchr reviewed Jul 14, 2017

View reviewed changes

Siedlerchr approved these changes Jul 14, 2017

View reviewed changes

mpele and others added 2 commits July 14, 2017 15:58

Corrected text in CHANGELOG.md

0e2fbbf

Fix space

5c346b1

koppor merged commit 8b50ce9 into JabRef:master Jul 14, 2017

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 14, 2017

mpele deleted the cleanSaveAs branch July 18, 2017 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shutdown previus AutosaveManager and BackupManager during SaveAs #2994

Shutdown previus AutosaveManager and BackupManager during SaveAs #2994

mpele commented Jul 11, 2017 •

edited by koppor

Loading

lenhard commented Jul 11, 2017

mpele commented Jul 11, 2017

lenhard commented Jul 11, 2017

mpele commented Jul 14, 2017

lenhard commented Jul 14, 2017

Siedlerchr Jul 14, 2017

tobiasdiez Jul 14, 2017

Siedlerchr Jul 14, 2017

Siedlerchr left a comment

Shutdown previus AutosaveManager and BackupManager during SaveAs #2994

Shutdown previus AutosaveManager and BackupManager during SaveAs #2994

Conversation

mpele commented Jul 11, 2017 • edited by koppor Loading

lenhard commented Jul 11, 2017

mpele commented Jul 11, 2017

lenhard commented Jul 11, 2017

mpele commented Jul 14, 2017

lenhard commented Jul 14, 2017

Siedlerchr Jul 14, 2017

Choose a reason for hiding this comment

tobiasdiez Jul 14, 2017

Choose a reason for hiding this comment

Siedlerchr Jul 14, 2017

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

mpele commented Jul 11, 2017 •

edited by koppor

Loading