Add throttle to AutosaveUIManager#5680
Conversation
koppor
left a comment
There was a problem hiding this comment.
I checked the JavaDoc for Timer:
Terminates this timer, discarding any currently scheduled tasks. Does not interfere with a currently executing task (if it exists). Once a timer has been terminated, its execution thread terminates gracefully, and no more tasks may be scheduled on it.
I think, this is not the thing one wants to achieve here: The current save should be canceled a new one should be triggered.
I remember a discussion we had with BackgroundWorker, but I currently don't find it.
Refs #2838 (comment)
We wanted to implement a real tasking framework in JabRef. Maybe, JavaFX offers such a thing... This would really solve issues with the PDF indexer and also with our tex feature (http://blog.jabref.org/2019/08/06/GSoC-LatexCitationsTab/)
|
When working on this, the comments on #5071 (comment) should be checked 😇 |
|
I found the ScheduledService from javafx, mabye this is an option as well? |
|
@koppor by introducing a deferred execution of the actual save operation, a race condition can happen when the old save operation is still in progress. As I can see there are two |
|
@Siedlerchr I think this is more for recurring tasks rather to simply defer a task, as it says that it will go from scheduled -> running -> finished -> scheduled automatically. |
a950aee to
6442ac9
Compare
6442ac9 to
5013f8b
Compare
|
Actually, @Siedlerchr you were right! There is a I now completely changed the way to tackle the problem. The current implementation replaces the Additionally, the bug of #4877 can now be reproduced. When autosave is enabled, change something and quickly press ctrl-s -> you will see the error. So we should make the SaveDatabaseAction thread safe to prevent the error. @koppor what do you think? |
|
Ah cool, that sounds good. Thread safe sounds fitting. |
tobiasdiez
left a comment
There was a problem hiding this comment.
I like this approach! Good job. I've only a few minor nitpicking comments.
In general, I'm not a big fan that the AutoSaveManager controls their own executor services and would prefer if this could be moved to https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/JabRefExecutorService.java. However, I couldn't come up with a good reusable solution (as you want to have exactly one service per database...)
|
@Ka0o0 Do you use Eclipse or IntelliJ? With IntelliJ (and the complete setup from https://github.com/JabRef/jabref/wiki/Guidelines-for-setting-up-a-local-workspace), the autoformat takes care.
(Unsure whether this is still the question...) - Either cancel the current autosave or let it go and then trigger it again. I would prefer the former (as long as we write to |
|
@koppor actually I have important that already but I forgot to do auto formatting before committing. I've now created a pre-commit hook which runs checkstyle so that this doesn't happens again. |
|
Thanks again! |
* upstream/master: Fix upload to GitHub artifacts (#5712) Try to fix csl update (#5718) Fix import into currently open library (#5717) Add throttle to AutosaveUIManager (#5680) Do not couple search position to sidebar width (#5716) fix springer fetcher tests Bump controlsfx from 11.0.0 to 11.0.1 (#5714)
As discussed in #5679 there are a lot of save actions performed when autosave is turned on. This PR adds a timer which limits the save actions to one in 200ms which is especially handy when automated tasks like find and replace are performed.