Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Configure backup path in prefs #9809

Merged
merged 22 commits into from
May 8, 2023
Merged

Configure backup path in prefs #9809

merged 22 commits into from
May 8, 2023

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Apr 27, 2023 •
edited
Loading

Adds a new option in the preferences to enable/disable backups and to choose the backup directory
Fallback/Default dir is always app dir as usual

  • Enable/disable backup
  • Fill the textfield in the prefs

Compulsory checks
Preview Give feedback

@Siedlerchr
Configure backup path in prefs
2ff3e9b 
TODO: Enable/disable backup
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into addPrefsForBak
12b0979 
* upstream/main: (31 commits)
  Fixes #9372 Change default File Chooser to show supported files extensions
  Fix path in devs documentation (#9810)
  New translations JabRef_en.properties (Tagalog)
  New translations JabRef_en.properties (Persian)
  New translations JabRef_en.properties (Indonesian)
  New translations JabRef_en.properties (Portuguese, Brazilian)
  New translations JabRef_en.properties (Vietnamese)
  New translations JabRef_en.properties (Chinese Traditional)
  New translations JabRef_en.properties (Chinese Simplified)
  New translations JabRef_en.properties (Ukrainian)
  New translations JabRef_en.properties (Turkish)
  New translations JabRef_en.properties (Swedish)
  New translations JabRef_en.properties (Russian)
  New translations JabRef_en.properties (Portuguese)
  New translations JabRef_en.properties (Polish)
  New translations JabRef_en.properties (Norwegian)
  New translations JabRef_en.properties (Dutch)
  New translations JabRef_en.properties (Korean)
  New translations JabRef_en.properties (Japanese)
  New translations JabRef_en.properties (Italian)
  ...
@Siedlerchr
fix l1ßn
7c534b2
@Siedlerchr
checkstyle and l10n
473a5e0
@Siedlerchr
checkstyle
0bb9321
koppor
koppor requested changes Apr 28, 2023
View reviewed changes
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments.

Yeah, the enable and disable of the backup (start/stop of BackupManager with other configuraiton) really needs to be done.

src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/logic/util/io/BackupFileUtil.java Show resolved Hide resolved
src/main/java/org/jabref/logic/util/io/BackupFileUtil.java Show resolved Hide resolved
src/main/java/org/jabref/logic/util/io/BackupFileUtil.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/preferences/JabRefPreferences.java Show resolved Hide resolved
Siedlerchr added 3 commits May 1, 2023 20:24
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into addPrefsForBak
302c7e9 
* upstream/main: (68 commits)
  Bump org.junit.platform:junit-platform-launcher from 1.9.2 to 1.9.3
  Bump org.jsoup:jsoup from 1.15.4 to 1.16.1
  Bump org.junit.jupiter:junit-jupiter from 5.9.2 to 5.9.3
  Bump com.puppycrawl.tools:checkstyle from 10.9.3 to 10.10.0
  Bump com.vladsch.flexmark:flexmark from 0.64.0 to 0.64.2
  Refresh example styles
  Squashed 'buildres/csl/csl-styles/' changes from 4728902faa..a985762505
  New translations JabRef_en.properties (Tagalog)
  New translations JabRef_en.properties (Persian)
  New translations JabRef_en.properties (Indonesian)
  New translations JabRef_en.properties (Portuguese, Brazilian)
  New translations JabRef_en.properties (Vietnamese)
  New translations JabRef_en.properties (Chinese Traditional)
  New translations JabRef_en.properties (Chinese Simplified)
  New translations JabRef_en.properties (Ukrainian)
  New translations JabRef_en.properties (Turkish)
  New translations JabRef_en.properties (Swedish)
  New translations JabRef_en.properties (Russian)
  New translations JabRef_en.properties (Portuguese)
  New translations JabRef_en.properties (Polish)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/preferences/file/FileTab.java
#	src/main/java/org/jabref/gui/preferences/file/FileTabViewModel.java
#	src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java
@Siedlerchr
fix merge conflicts
b935fde
@Siedlerchr
add comment
5dae372
Siedlerchr and others added 7 commits May 4, 2023 20:54
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into addPrefsForBak
f6688b8 
* upstream/main:
  Fix modernizer (#9824)
  Improve search history by attaching change listener (#9794)
  Fix split multiline localization (#9814)
  New Crowdin updates (#9834)
  change versin to 0.8.10
  remove  jacoco version config
@Siedlerchr
fix checkstyle
2f447d9
@calixtus
Moved backup settings to GeneralTab
bf4c43d
@calixtus
Merge remote-tracking branch 'upstream/addPrefsForBak' into addPrefsF…
749fb42 
…orBak
@calixtus
Refine ui
9a270dc
@Siedlerchr
fix unit tests
de97779
@Siedlerchr
add tests
cfebdbe
@Siedlerchr Siedlerchr changed the title WIP: Configure backup path in prefs Configure backup path in prefs May 4, 2023
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 4, 2023
@HoussemNasri
Copy link
Member

I checkout the PR, but then I can't seem to trigger the backup manager dialog. I have tried closing JabRef from IntelliJ and from the Taskmanager by ending the process but neither has worked; when I start JabRef again, the backup dialog doesn't show. Keep in mind that the "Create backup" is enabled.

polish: besides that problem, I think the backup path text field should be disabled when the "Create backup" checkbox is unchecked.

@koppor
Copy link
Member

koppor commented May 4, 2023

I checkout the PR, but then I can't seem to trigger the backup manager dialog. I have tried closing JabRef from IntelliJ and from the Taskmanager by ending the process but neither has worked;

Did you also modify the library without saving?

I did and I got the dialog:

image

polish: besides that problem, I think the backup path text field should be disabled when the "Create backup" checkbox is unchecked.

Current state:

image

We should also have a button "reset to default" (same icon as in "Reset preferences")

@HoussemNasri
Copy link
Member

Yes, I modified the library before closing it. I also changed the backup path.

@Siedlerchr
Copy link
Member Author

Siedlerchr commented May 5, 2023 via email

Siedlerchr added 2 commits May 5, 2023 19:40
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into addPrefsForBak
bc4f938 
* upstream/main:
  Minimal config for openRewrite
  Fix missing #
  CHANGELOG.md
  Removed unused code
  Refined ui
  Dissolved FileTab and moved contents to EntryTab
  Renamed CustomEditorFieldsTab to EntryEditorTabsTab
  Fixed antipattern, fixed radiobutton with checkbox
  Renamed ImportExportPreferences to ExportPreferences
  Separated WebSearchPrefs and ExportPrefs
  Separated WebSearchTab and ExportTab
  Renamed ImportExportTab to WebSearchTab

# Conflicts:
#	src/main/java/org/jabref/gui/preferences/export/ExportTab.java
#	src/main/java/org/jabref/gui/preferences/file/FileTab.fxml
#	src/main/java/org/jabref/gui/preferences/file/FileTabViewModel.java
@Siedlerchr
disable backup dir when unchecked
b9c1af2
@HoussemNasri
Copy link
Member

@Siedlerchr It is working now! A restart was needed as you said. Also, I noticed that if I open JabRef, make a change, and then close it (kill the process) very fast, the backup dialog doesn't show the next time.

@Siedlerchr
Copy link
Member Author

Yep, you have to wait a few seconds because the backup path is using seconds in the filename only as minimum String timeSuffix = ZonedDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd--HH.mm.ss"));

That's why I added a sleep in the test as well

@HoussemNasri
Copy link
Member

issue: Crossref field changes aren't recorded; It is probably not caused by this pr but I'm documenting it here to not forget it.

HoussemNasri
HoussemNasri requested changes May 5, 2023
View reviewed changes
Copy link
Member

@HoussemNasri HoussemNasri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM codewise, I didn't look too much into the tests but they look good overall. It would be great if you can get the backup directory to be updated without restarting, otherwise, a "you need to restart" message is fine.

src/main/java/org/jabref/gui/JabRefFrame.java Show resolved Hide resolved
src/main/java/org/jabref/logic/util/io/BackupFileUtil.java Show resolved Hide resolved
@calixtus calixtus merged commit 3de6c7b into main May 8, 2023
@calixtus calixtus deleted the addPrefsForBak branch May 8, 2023 18:57
@koppor koppor mentioned this pull request May 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@koppor koppor koppor requested changes

@HoussemNasri HoussemNasri HoussemNasri requested changes

Assignees
No one assigned
Labels
backup preferences status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Siedlerchr @HoussemNasri @koppor @calixtus @ThiloteE