"Discard changes" in "Save before closing" should delete "newer" backups

[koppor]

As user, I work on JabRef. Then, I add some things. I maybe want to cancel my changes. Example:

Then, at the start of JabRef, JabRef should NOT prompt me to restore these changes.

Four options:

• Option 1: Delete newer backup files
• Option 2: Rename backup files to files with certain keywords (and delete them after some time)
• Option 3: Make backups appearing "older" (we need to fake time stamps)
• Option 4: Create discarded file to mark that the user discarded changes. On start of JabRef, JabRef removes that file and backup takes normally place. Example filename: 269b5589--test.bib--discarded.bak. When JabRef starts freshly, it creates new backups in the backup manner. After a normal save and an additional restart, JabRef MUST NOT assume that the last quit was a discard.

We opt for Option 4.

[mmohanb1]

Hi Koppor,
I would like to contribute to this project and would like to work on this issue. Could you please assign this to me.
Thanks.

[github-actions]

As a general advice for newcomers: check out Contributing for a start. Also, guidelines for setting up a local workspace is worth having a look at.

Feel free to ask here at GitHub, if you have any issue related questions. If you have questions about how to setup your workspace use JabRef's Gitter chat. Try to open a (draft) pull-request early on, so that people can see you are working on the issue and so that they can see the direction the pull request is heading towards. This way, you will likely receive valuable feedback.

[koppor]

@mmohanb1 This issue is on our track for the upcoming release. Do you have a rough schedule when you will come up with a first proposal?

[mmohanb1]

Hi Oliver, What is the next release date? I should be able to come up with a proposal by today or Tuesday at the latest. Thank you.

…

On Sun, Nov 20, 2022 at 9:39 AM Oliver Kopp ***@***.***> wrote: @mmohanb1 <https://github.com/mmohanb1> This issue is on our track for the upcoming release. Do you have a rough schedule when you will come up with a first proposal? — Reply to this email directly, view it on GitHub <#9361 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVTCRF7A5LU4DPJGQRFHOQTWJIZ35ANCNFSM6AAAAAARZTM7SU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mmohanb1]

Edited below,

Hello,
Here is the proposal. I am going to add code in confirmClose method of jabRefFrame.java class to check if user wants to discard the changes to items and if yes, I am going to save a --discarded.bak version file in the database backup path. I will also check in performBackUp method of BackupManager.java class if a --discarded.bak file exists in backup path, if it exists I will simply return without performing backup of that bib file when user quits jabRef. When user opens jabRef after discarding and closing the application, I am going to write code in loadDatabase method of OpenDatabaseAction.java class to check if a corresponding --discarded.bak file is present in the database backup directory and if it is present, I will delete the discarded file and open the original file(from an older backup file) without showing the restore discarded dialog and if not present I will simply open the original file.

[mmohanb1]

I was able to work on this issue and functionality seems to be working fine as expected but I need to test more using the testing documentation on jabref.

[ThiloteE]

What is the next release date?

Initially we would have wanted to have a release in November, but a few things have come up and now it has been a little postponed. The new date might roughly be the ~ 19th of december, but don't take it as face value. JabRef's releases are never permanently fixed to a date, as sometimes regressions show up and maintainers have personal lives too. Ideally, it would be nice to have a fix for this issue until ~ 9th or so, so that there is still time to have it lingering within the main branch for a week or two before the 5.8 release. That would allow us to catch potential regressions. I really would like to avoid having this merged a day before the release.

[koppor]

Hello, Here is the proposal. I am going to add code in confirmClose method of jabRefFrame.java class to check if user wants to discard the changes to items and if yes, I am going to save a --discarded.bak version file in the database backup path.

Maybe, the BackupManager should be notified that the last changes were discarded. Then, the BackupManager can stop working. It will be shut down directly afterwards (or even by the "Discard" button)

I will also check in performBackUp method of BackupManager.java class if a --discarded.bak file exists in backup path, if it exists I will simply return without performing backup of that bib file when user quits jabRef.

Not necessary - see above. -- Checking for files during backup is time consuming.

When user opens jabRef after discarding and closing the application, I am going to write code in loadDatabase method of OpenDatabaseAction.java class to check if a corresponding --discarded.bak file is present in the database backup directory

The check should be done by the BackupManager - at org.jabref.logic.autosaveandbackup.BackupManager#backupFileDiffers. That should return false in the case --discarded.bak is present.

I think, the return for that should be set at org.jabref.logic.autosaveandbackup.BackupManager#fillQueue. There, it should be checked, if a --discarded file is present. It should be deleted there, but the flag discardedFileExists should be set to true. And in backupFileDiffers, the flag should be checked first. If it is true, backupFileDiffers returns false.

Then, the dialog does not need to be modified - all the code happens inside the BackupManager. #seperationOfConcerns.

[mmohanb1]

Hi Oliver,
Thank you for the comments. I will work on this per your suggestions. I have an academic project that's due next week but I should be able to work on this in the 2nd week of dec and hopefully be able to finish and test at least a few days ahead of the release.

[koppor]

@mmohanb1 Thank you for the update! Please submit a pull request early - we also need time to test, since this is a feature seen by many users.

[mmohanb1]

Hi @koppor ,
so just to be clear, a *--discarded.bak file should or can still be saved in backup path on clicking "discard changes" and all the rest of the logic implemented in backUpManager.?

[koppor]

Hi @koppor ,
so just to be clear, a *--discarded.bak file should or can still be saved in backup path on clicking "discard changes" and all the rest of the logic implemented in backUpManager.?

Yes! - Maybe even the creation of the discarded file can be done in the Backup-Manager? Maybe in the shutdown method? I don't know exactly the flow of that dialog.

[mmohanb1]

Hi @koppor
A few problems I am facing with this approach.

1. Even when I delete the *--discarded.bak file in fillQueue. There is another backup of the *--discarded.bak file created in the backup path. Which on opening jabRef again, compares this version with original file and shows up the restore dialog.
2. I am not sure how will the discardedFileExists flag play a part here, as even if the flag is set at fillQueue, the app is shut down and on opening the app, the initial value of the flag will be accessed by backUpFileDiffers method(I am assuming backUpFileDiffers method is called in loadDatabse method of OpenDatabaseAction class which is at the start of jabRef), unless there is a way of persisting this flag value somewhere outside the instance of the application(pardon my naivety here).

Having said that, I am still trying to figure a way of doing it with suggested approach.

[mmohanb1]

Hi.
I was able to fix the first issue, but one thing I am noticing is that after jabRef is closed and opened again, the last bib file on which the changes were discarded do not open by default and the user has to open them manually. Is this a major concern?

The second issue still remains. Is there any database table where I can save the value of discardedFileExists after changes are discarded.

Would need some help on these things.

[koppor]

Hi. I was able to fix the first issue, but one thing I am noticing is that after jabRef is closed and opened again, the last bib file on which the changes were discarded do not open by default and the user has to open them manually. Is this a major concern?

Yes, it is. Then, IMHO something is wrong in the implementation.

The second issue still remains. Is there any database table where I can save the value of discardedFileExists after changes are discarded.

The idea of the existence of "discarded" file (which is a Boolean flag) was to be a kind of database store.

We are currently discussing symptoms only.

For a real software-engineered project, there would have been UML diagrams made. Maybe a UML sequence diagram or a UML state diagram.

The second approach is to follow the idea of test-driven development and craft test cases.

The third approach is to do code and fix on the code. This is also OK, but I (and maybe other developers) should see the code. Can you open a pull request?

[mmohanb1]

Could we instead of deleting the discarded file in fillQueue, delete it in backUpFileDiffers method and return false there.?

[koppor]

Could we instead of deleting the discarded file in fillQueue, delete it in backUpFileDiffers method and return false there.?

Thoughts:

• Deleting it there - and if a user directly exists JabRef afterwards would trigger the dialog again. Maybe, this is acceptable?
• Why not just returning false in the method backUpFileDiffers? - No, because if a later backup is made, the algorithm would again return false
• Note: By "it should be deleted there" I meant the normal algorithm of handling the queue. At each new backup, the top most file is deleted. Maybe, this is too over-engineerined and the file can be deleted earlier. I thought, this handling could made it simpler.

Idea: Can the algorithm be changed to check whether the discarded file is the last one? If it is not the last one, it can be deleted? If it is the last one, return false at backUpFileDiffers

[mmohanb1]

I have problem figuring out in which part of backUpManager should we create the "--discarded.bak" file, if not in GUI(jabRefFrame.java). When the user chooses to close jabRef and discards changes, shutdown method is called from where performBackUp is called, which picks up the backup file path and saves it. I think the discarded file should be already created at this point, so performBackUp method can save it. So it has to be before performBackUp call.