Implementation of autosave & backup feature#2118
Conversation
|
I have not tested it, but what are the implacations of this? If I open a bib-File in JabRef, do some changes and then decide to discard all changes since opening the database. Is this still possible? (While this is a rather obscure use case for normal users, this is exactly my approach when trying to reproduce bugs or checking changes made in code. So I should know whether I have to change my workflow in order not to "destroy" some files...) |
c1bdbf0 to
520a095
Compare
|
@matthiasgeiger Thanks for the objection. For the case to discard all changes while Autosave is enabled the Undo operation could be used gradually as Undo and Redo are also supported by this feature. An idea is to add an "Undo all" operation (possibly a new button next to the existing Undo button) which could recover the state the file was opened with. Currently I can not judge whether it's worth to do this but I could open a new issue or PR for this if desired. |
and file synchronization for shared DBs. - Root out old autosave functionality - Add and integrate new Autosaver - Add ExecutorService and apropriate workerQueue - Add writing methods for BibDatabaseWriter (databaseID) - Add parsing methods for databaseID - Introduce BibDatabaseContextChangedEvent - Introduce AutosaveEvent - Construct preferences saving methods - Extend preferences tree - Adjust preftab - Adjust test classes - Update localization keys - Add comments - Adjust tab and window naming
| }); | ||
| } catch (RejectedExecutionException e) { | ||
| LOGGER.debug("Rejecting autosave while another save process is already running."); | ||
| // do not save |
There was a problem hiding this comment.
I think the comment should be one line above.
| /** | ||
| * This Event is fired from {@link Autosaver} in case that a save task is pending. | ||
| */ | ||
| public class AutosaveEvent { |
There was a problem hiding this comment.
Could you shortly explain, why a class without data is needed ?
There was a problem hiding this comment.
This is just an Object which is passed through the eventBus according to the context. It carries no special data as it's only used for notifying methods which are listening for AutosaveEvent. I'm following this strategy to preserve the MVC pattern. Otherwise I could call saving methods directly from Autosaver without using the eventBus.
|
|
||
| boolean autosave = (((context.getLocation() == DatabaseLocation.SHARED) && Globals.prefs.getBoolean(JabRefPreferences.SHARED_AUTO_SAVE)) || | ||
| ((context.getLocation() == DatabaseLocation.LOCAL) && Globals.prefs.getBoolean(JabRefPreferences.LOCAL_AUTO_SAVE))) && | ||
| context.getDatabaseFile().isPresent(); |
There was a problem hiding this comment.
Would be nice if you could split autosave in a method instead of a variable, since it is a very complex expression.
| // Register undo/redo listener | ||
| bp.getUndoManager().registerListener(new UndoRedoEventManager()); | ||
|
|
||
| BibDatabaseContext context = bp.getBibDatabaseContext(); |
There was a problem hiding this comment.
I know this doesn't come from you, but can you rename bp?
| new SharedDatabasePreferences(databaseID).putAllDBMSConnectionProperties(properties); | ||
| } | ||
|
|
||
| File oldFile = context.getDatabaseFile().orElse(null); |
There was a problem hiding this comment.
This can be moved to the if-part beneath, right?
| return; | ||
| } | ||
| // Register so we get notifications about outside changes to the file. | ||
|
|
There was a problem hiding this comment.
Please remove this empty line.
|
|
||
| boolean autosave = (((context.getLocation() == DatabaseLocation.SHARED) && Globals.prefs.getBoolean(JabRefPreferences.SHARED_AUTO_SAVE)) || | ||
| ((context.getLocation() == DatabaseLocation.LOCAL) && Globals.prefs.getBoolean(JabRefPreferences.LOCAL_AUTO_SAVE))) && | ||
| context.getDatabaseFile().isPresent(); |
There was a problem hiding this comment.
Same as above, please split to method.
An alternative would be if you split some parts of the expression only, for example the parts that are together in brackets.
| new Autosaver(context).registerListener(new AutoSaveUIManager(panel)); | ||
| } | ||
|
|
||
| frame.getFileHistory().newFile(context.getDatabaseFile().get().getPath()); |
There was a problem hiding this comment.
What if getDatabaseFile() is not present? Shouldn't this be checked before?
Please check this everywhere.
There was a problem hiding this comment.
This line has been assumed. However I added a ifPresent check for this.
| // Should this be done _after_ we know it was successfully opened? | ||
| String fileName = file.getPath(); | ||
| Globals.prefs.put(JabRefPreferences.WORKING_DIRECTORY, file.getParent()); | ||
| // Should this be done _after_ we know it was successfully opened? |
There was a problem hiding this comment.
To which line/code part does this belong?
| Preferences.userNodeForPackage(JabRefMain.class).parent().node(PARENT_NODE).clear(); | ||
| } | ||
|
|
||
| public void putAllDBMSConnectionProperties(DBMSConnectionProperties properties) { |
There was a problem hiding this comment.
Why don't check here if the properties are valid? (Seen this method somewhere above...)
There was a problem hiding this comment.
DBMSConnectionProperties.isValid() is performed by methods which open a shared database from the local file. This method is only a shortcut for all put* methods.
There was a problem hiding this comment.
So you can guarantee that they are valid when calling this method?
There was a problem hiding this comment.
It's only a shortcut. But in this context this method is always called with legal parameters.
|
@obraliar Thanks for the explanation! I'm not sure whether we'll make our users happy with this changes. I know that this feature is inspired by the way IntelliJ is working, but using IntelliJ the files modified are a) mostly under version control, b) IntelliJ also has a good local history to quickly revert to a specific state and c) the undo/redo functionality is working in all cases. I don't think that we can assume a) for (the majority of) our users, b) is not there in JabRef and I'm not sure whether c) is really given... @JabRef/developers WDYT? |
|
My 2 cents (not from a developer), but from a user: As a User I do not like the software to change if there is no obvious gain. Here, the change could be detrimental since I am used to save after I am happy with my changes. |
|
Well, looking at #344 I would expect that the autosave is disabled by default and can be enabled by the user. As long as this is the case, I do not see too much of a problem, since the workflow of most users would not be disturbed. Enabling it by default would be problematic and I would oppose this strongly. Speaking for myself, I want to be in control when the save happens (and I do not really like the IntellJ feature too much anyway). Regarding dropping or keeping the time-based autosave, I am rather agnostic. |
|
|
|
|
Basically, the old autosave was no autosave since it only created the backup file. Backup file creation can be done as is, but without the user having a choice about it. So the old preferences can be removed. The real autosave implemented here is really new and should base on a new preference option, which is disabled by default. |
|
@koppor I think it will make sense for autosave and backup to be described in the same help file. |
|
Yes, @mlep. Especially as the old backup functionality was named "Autosave" ;-) So it must be made clear that the functionality is now enabled by default (and can not be disabled) and the new functionality is now a real autosave affecting the actual bib-file. |
- Add check for backup file - Add BackupManager & BackupUIManager - Integration into JabRefFrame, SaveDatabaseAction and OpenDatabaseAction - Make Autosave disabled by default for local DBs - Small refactorings - Update/add localization keys - Fix import order
|
Thanks for the discussion! Here comes the clarification & short summary:
For my part, this feature is ready. |
tschechlovdev
left a comment
There was a problem hiding this comment.
Beside some minor comments, LGTM now 👍
| */ | ||
| public static void shutdown(BibDatabaseContext bibDatabaseContext) { | ||
| for (AutosaveManager autosaver : runningInstances) { | ||
| if (autosaver.bibDatabaseContext == bibDatabaseContext) { |
There was a problem hiding this comment.
Why do you use here == and not equals()?
There was a problem hiding this comment.
This should only be a reference check.
| } | ||
|
|
||
| if (result.isNullResult()) { | ||
| JOptionPane.showMessageDialog(null, Localization.lang("Error opening file") + " '" + fileName + "'", |
There was a problem hiding this comment.
Can't this be done in the catch block above?
There was a problem hiding this comment.
This come out from defensive programming. But yes you are right. Done.
| this.base = base; | ||
| this.metaData = metaData; | ||
| this.entryTypes = entryTypes; | ||
| if (Objects.nonNull(base) && Objects.nonNull(metaData)) { |
There was a problem hiding this comment.
why do you check base and metaData for nonNull but not file?
| setEncoding(encoding, true); | ||
| } | ||
|
|
||
| // The parameter postChanges has been introduced to avoid event loops while saving a database |
There was a problem hiding this comment.
Why not change it to a javadoc? Then one can see it, if he wants to call the method ;)
| Preferences.userNodeForPackage(JabRefMain.class).parent().node(PARENT_NODE).clear(); | ||
| } | ||
|
|
||
| public void putAllDBMSConnectionProperties(DBMSConnectionProperties properties) { |
There was a problem hiding this comment.
So you can guarantee that they are valid when calling this method?
|
I'd say move |
tobiasdiez
left a comment
There was a problem hiding this comment.
Just a question (for now :) ) based on quickly scrolling over the code: where do you need the new database id? It looks like it is only used in if checks to test if the file is local (since shared db have no id, right?).
Why not use the BibContext for this?
|
|
||
| @Test | ||
| public void testGetConnection() throws SQLException { | ||
| public void testGetConnection() throws SQLException, InvalidDBMSConnectionPropertiesException { |
There was a problem hiding this comment.
(For the next time:) Just use the generic throws Exception in tests so that you don't have to edit tests if you decide to throw a different exception.
|
@tobiasdiez |
tobiasdiez
left a comment
There was a problem hiding this comment.
Sorry, I still don't really get the idea behind the ID-construction. Could you please help me by providing some background.
| } else { | ||
| JabRefGUI.getMainFrame().addParserResult(pr, first); | ||
| first = false; | ||
| } else if (Objects.nonNull(pr.getDatabase().getDatabaseID())) { |
There was a problem hiding this comment.
What is the purpose of this check?
There was a problem hiding this comment.
Return optional by getDatabaseId instead of null.
| final ParserResult finalReferenceToResult = result; | ||
| SwingUtilities.invokeLater( | ||
| () -> OpenDatabaseAction.performPostOpenActions(panel, finalReferenceToResult, true)); | ||
| if (Objects.nonNull(result.getDatabase().getDatabaseID())) { |
There was a problem hiding this comment.
Same here, I don't understand the purpose of getDatabaseID. You load the db above and what are you now doing in this if statement? I'm probably just to stupid to understand what openSharedDatabaseFromParserResult does. Can you please enlighten me.
There was a problem hiding this comment.
Probably I should explain it in general. A databaseID is used to identify shared databases when they are stored on the local file system. Furthermore it's used to allocate connection details as you are able to have multiple shared databases somewhere. When opening a shared db (from file) the method openSharedDatabaseFromParserResult is called. This method uses databaseID which has been parsed by BibtexParser to do what I described above.
For the case databaseID == null it's not a shared database. I hope that's helpful.
There was a problem hiding this comment.
Ok, thank you very much. Your remarks were indeed helpful. I think I got it now 😄 .
In this case, I would propose to add the db id as a meta-data. Reasoning: all other metadata is also stored there and we had some problems with parsing the %Encoding string. Thus I would minimize all data which is stored in such comments. (As a key, I propose remoteDatabaseID. )
@obraliar what do you think?
There was a problem hiding this comment.
I already thought about this option, but this is unfortunately not possible as meta-data, preamble, etc. are also shared. This would end in disaster. This key/ID has to be stored in a local file but it also must not be in shared.
There was a problem hiding this comment.
Ok, thats a good reason.
So we have two options: store the database id as something special (e.g. as a comment % DB: some id) or we exclude some specific metadata items from the sync (or we create a new metadata which is never synced).
I have no strong opinion about this. What do you think @JabRef/developers ?
There was a problem hiding this comment.
So we have two options: store the database id as something special (e.g. as a comment % DB: some id) or (...)
This is exactly the method I'm using now and it works well. I dont see any reasons for changing it.
An example for the id comment is % DBID: 2mvhh73ge3hc5fosdsvuoa808t.
|
Tested it locally and works good. |
|
Went through with @obraliar and we made the fixes in a pair-pgramming style. LGTM. An UI update on the autosave of shared database feature will coming to make it more visible to the users. |
tobiasdiez
left a comment
There was a problem hiding this comment.
I started reviewing this PR in the afternoon but didn't finished it. In the meantime the PR got merged (and changed again). I finished the review nonetheless, since I think there were some things which still could be improved on. There are no major showstoppers, so I will not revert the merge but please follow-up on the comments with a new PR. Thanks.
| } else { | ||
| JabRefGUI.getMainFrame().addParserResult(pr, first); | ||
| first = false; | ||
| } else if (Objects.nonNull(pr.getDatabase().getDatabaseID())) { |
There was a problem hiding this comment.
Return optional by getDatabaseId instead of null.
| session.commit(file.toPath()); | ||
| panel.getBibDatabaseContext().getMetaData().setEncoding(encoding); // Make sure to remember which encoding we used. | ||
| // Make sure to remember which encoding we used. | ||
| panel.getBibDatabaseContext().getMetaData().setEncoding(encoding, ChangePropagation.DO_NOT_POST_EVENT); |
There was a problem hiding this comment.
Why don't we want to send a change event here? Probably you don't want to trigger a new save of the file, but the problem with the current solution is that nobody else can listen to encoding changes in a reliable way (not that I have a particular case in mind where listeners to the encoding are important, but I think we shouldn't start with exempting something from the event system).
There was a problem hiding this comment.
Probably you don't want to trigger a new save of the file
It's even worse, this would cause an event loop which would never end.
the problem with the current solution is that nobody else can listen to encoding changes in a reliable way
There is still the old method setEncoding(Charset encoding) which is now a shortcut for setEncoding(encoding, ChangePropagation.POST_EVENT). Every time you set the encoding the event is also going to be posted (which will also trigger the autosave). It's ok so far. But the event is not going to be posted while saving a file. I'm more skaptical why this is called here at all. The old comment // Make sure to remember which encoding we used. is not really justified, as the encoding should be set immediately when it changes and not "at the last second".
| // If the operation failed, revert the file field and return: | ||
| if (!success) { | ||
| panel.getBibDatabaseContext().setDatabaseFile(oldFile); | ||
| context.setDatabaseFile(context.getDatabaseFile().orElse(null)); |
There was a problem hiding this comment.
I don't understand this line (set/get from the same context), especially since you set the database file a few lines earlier.
There was a problem hiding this comment.
This comes out from defensive programming. It basically does nothing. Removed.
| panel.getBibDatabaseContext().setDatabaseFile(file); | ||
| BibDatabaseContext context = panel.getBibDatabaseContext(); | ||
|
|
||
| if (context.getLocation() == DatabaseLocation.SHARED) { |
There was a problem hiding this comment.
This additional code should be in some logic code (some exporter?) instead of gui.
There was a problem hiding this comment.
Now I moved the logic part as much as possible from here. But nevertheless there are multiple methods like run(...) or even worse like saveDatabase(...) which radically violate the MVC pattern. So this is a general problem in jabRef which requires a special attention.
| if (context.getLocation() == DatabaseLocation.SHARED) { | ||
| // Generate an ID when saving a shared database | ||
| String databaseID = new BigInteger(128, new SecureRandom()).toString(32); | ||
| context.getDatabase().setDatabaseID(databaseID); |
There was a problem hiding this comment.
The database should take care of its ID and thus also how a new one is generated -> move this two lines to a new method createNewID in the database class. I would also rename everything there from databaseID to simply ID or sharedID (since it is already a member of the database class so it should be clear from the context)
| @Override | ||
| protected void writeDatabaseID(String databaseID) throws SaveException { | ||
| try { | ||
| if (Objects.nonNull(databaseID)) { |
There was a problem hiding this comment.
Please don't use null values for databaseID. (also check for empty string, StringUtils.isBlank)
There was a problem hiding this comment.
This check is not needed any more. Done.
|
|
||
| public BibDatabaseContext getDatabaseContext() { | ||
| return new BibDatabaseContext(base, metaData, file); | ||
| if (Objects.isNull(this.bibDatabaseContext)) { |
There was a problem hiding this comment.
I think the good old something == null is more readable. According to the documentation, Objects.isNull just exists to be used as a predicate (i.e. ̀filter(Objects::isNull))
There was a problem hiding this comment.
Good to know. Done.
| import java.util.prefs.BackingStoreException; | ||
| import java.util.prefs.Preferences; | ||
|
|
||
| import net.sf.jabref.Globals; |
There was a problem hiding this comment.
Now that this globals is removed, I think the shared namespace can be moved to logic.
|
|
||
| @Test | ||
| public void testRecognizesDatabaseID() throws Exception { | ||
| Path file = Paths.get(BibtexImporterTest.class.getResource("AutosavedSharedDatabase.bib").toURI()); |
There was a problem hiding this comment.
Could you please add the database id also to complex.bib. This test file should illustrate every feature Jabref has. Then also an additional AutosavedSharedDb.bib is no longer necessary.
| String actualDatabaseID = parserResult.getDatabase().getDatabaseID(); | ||
|
|
||
| assertEquals(expectedDatabaseID, actualDatabaseID); | ||
| } |
There was a problem hiding this comment.
Please also add a test that the database id comment is not recognized as the comment of the first entry.
|
The more test cases, the better. I would not remove any test file to ensure
high test coverage.
A DBID is uncommon, therefore I would create a second complex.bib.
If we want, we can do a generation of all variants of compley.bib (with
encoding info, without, with special fields, without, with old groups, with
new grouos, ...) and check them all.
Currently, complex.bib is not written with a recent JabRef version (casing
at the entry types is wrong).
|
|
The roundtrip tests only work for the complex.bib and thus I would just add everything in this file. And for this particular test, there is no reason to have a special test file instead of creating the input directly as a string in the test method. |
* upstream/master: Code cleanups (#2141) Remove obsolete string in .mailmap Update .mailmap and AUTHORS Implementation of autosave & backup feature (#2118) Make all files selectable in file chooser dialogs (#2094) Fix group drag and drop (#2161) Change donation badge and point to https://donations.jabref.org # Conflicts: # src/main/java/net/sf/jabref/logic/util/OptionalUtil.java
Related to: #344.
Utility for local databases:
This feature is event based and saves the database if activated on every user interaction. An intelligent
ExecutorServiceandworkerQueueare preventing a high load while saving. Furthermore all redundant save tasks will be rejected.An event based
BackupManagerreplaces the old time based backup system. This one is also using aExecutorServiceto prevent high load issues.Utility for shared databases:
For shared databases this feature actually synchronizes the local file which is associated with the shared database supporting LiveUpdate!
gradle localizationUpdate?