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.

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

Fix Autosave threading issue #5967

Closed
wants to merge 2 commits into from

Conversation

Ka0o0
Copy link
Contributor

@Ka0o0 Ka0o0 commented Feb 20, 2020

This PR fixes #5658
Please see my comment in the associated issue.

  • Change in CHANGELOG.md described (if applicable)
  • Manually tested changed features in running JabRef (always required)

@koppor koppor added this to the v5.0 milestone Feb 20, 2020
@tobiasdiez tobiasdiez modified the milestones: v5.0, v5.1 Feb 23, 2020
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks for your PR. I guess this works, but has the disadvantage that now the complete save operation is run on the javafx thread and thus JabRef gets inresponsible for large databases. Would prefer if you find a solution that only certain parts (the ones that are needed) run on the javafx thread.

Either way, merging should be postponed until 5.0 stable is released to be on the safe side.

@koppor koppor force-pushed the master branch 5 times, most recently from b8ef7b7 to 21c6e5e Compare March 4, 2020 17:02
@koppor koppor changed the title Fix Autosave threading issue [WIP] Fix Autosave threading issue Mar 6, 2020
@koppor koppor removed this from the v5.1 milestone Mar 6, 2020
@tobiasdiez
Copy link
Member

@Ka0o0 did you already had the chance to investigate this further?

@Ka0o0 Ka0o0 force-pushed the fix-5658-autosave-threading-issues branch from fbc1357 to d3f2be3 Compare April 13, 2020 13:08
@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Apr 13, 2020

@tobiasdiez yes. So I found that the exception happened in the updateAllTabTitles method in JabRefFrame. So by just putting this into a foreground thread, I think the performance issues should be resolved. However, I'm not sure if that jumping between threads isn't making development a bit more complicated. Are there any rules whether or not it is okay to call UI methods classes from classes that operate on a background thread?

@tobiasdiez tobiasdiez marked this pull request as draft April 17, 2020 15:57
@koppor koppor changed the title [WIP] Fix Autosave threading issue Fix Autosave threading issue Apr 23, 2020
@koppor
Copy link
Member

koppor commented Apr 23, 2020

@Ka0o0 Without thinking deeper, UI should never be called from the background. For instance the integrity check, we just call the logic from the UI: org.jabref.gui.integrity.IntegrityCheckAction#execute

@tobiasdiez
Copy link
Member

@Ka0o0 sorry for the late answer. The general principle is that every background process is not allowed do change anything in the UI. Using the BackgroundWorker the strategy is the following:

BackgroundWorker.wrap(do the operation that takes long, return result of calculation)
                .success(update ui)

Right now the auto-save operation triggers also UI updates, which shouldn't be the case. The code needs to be refactored so that only the actual save operation is on the background thread, while the rest should be on the UI thread.

I fear this might be more work. Could you please investigate if you find a solution along these lines. If it is indeed too complicated, we can go with your fix as well for the moment.

@koppor koppor marked this pull request as ready for review August 31, 2020 22:36
@koppor
Copy link
Member

koppor commented Aug 31, 2020

JabCon decision: We did not see the issue appearing. Reading #5658 (comment), we should really go for a clean fix. Thus, closing this out.

@koppor koppor closed this Aug 31, 2020
koppor pushed a commit that referenced this pull request Apr 1, 2022
21e2177 Create respiratory-care-journal.csl (#5998)
d0a2846 Update archaeonautica.csl (#5987)
664784e Update cardiff-university-harvard.csl (#5982)
07ce91e improve journal-of-avian-biology.csl (#5984)
349a5d6 update based on latest version of ISO 690 standard (#5976)
d8c3725 Update mary-ann-liebert-vancouver.csl (#5981)
7b2e70d Update style-manual-australian-government.csl (#5975)
bebf50c Create extracellular-vesicles-and-circulating-nucleic-acids.csl (#5979)
cabcdd4 Update united-states-international-trade-commission.csl (#5967)
948c48a Update cardiff-university-harvard.csl (#5977)
d6bf535 Add name to label gun
cf0d930 Test label gun
2f40fcd Update the-journal-of-egyptian-archaeology.csl (#5964)
aa023e1 Create materials-research-society-bulletin.csl (#5917)
ea31783 Changed quotation marks to Spanish ones (#5968)
27245f9 AJA: Add "." suffix to citation layout. (#5971)
7726a40 Update american-medical-association.csl (#5970)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 21e2177
Siedlerchr pushed a commit that referenced this pull request Apr 1, 2022
)

21e2177 Create respiratory-care-journal.csl (#5998)
d0a2846 Update archaeonautica.csl (#5987)
664784e Update cardiff-university-harvard.csl (#5982)
07ce91e improve journal-of-avian-biology.csl (#5984)
349a5d6 update based on latest version of ISO 690 standard (#5976)
d8c3725 Update mary-ann-liebert-vancouver.csl (#5981)
7b2e70d Update style-manual-australian-government.csl (#5975)
bebf50c Create extracellular-vesicles-and-circulating-nucleic-acids.csl (#5979)
cabcdd4 Update united-states-international-trade-commission.csl (#5967)
948c48a Update cardiff-university-harvard.csl (#5977)
d6bf535 Add name to label gun
cf0d930 Test label gun
2f40fcd Update the-journal-of-egyptian-archaeology.csl (#5964)
aa023e1 Create materials-research-society-bulletin.csl (#5917)
ea31783 Changed quotation marks to Spanish ones (#5968)
27245f9 AJA: Add "." suffix to citation layout. (#5971)
7726a40 Update american-medical-association.csl (#5970)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 21e2177

Co-authored-by: github actions <[email protected]>
koppor pushed a commit that referenced this pull request Apr 15, 2022
9ff3112 Update bibtex.csl (#6021)
68ed5f7 Don't initialize with hyphen in Vancouver/NLM (#6022)
80206ad Update american-physics-society.csl (#5934)
cfbdba6 Update csl style (#6003)
546ee88 Update institut-francais-darcheologie-orientale-en.csl (#6018)
1153e3e Update institut-francais-darcheologie-orientale-etudes-arabes.csl (#6019)
42d5031 Update institut-francais-darcheologie-orientale-arab-studies.csl (#6020)
2e10c7a Update institut-francais-darcheologie-orientale.csl (#6015)
0c7d6a1 Create science-china-materials.csl (#6017)
0eb4bc7 Update frontiers.csl (#6014)
0fd9cc2 Update urad-rs-za-makroekonomske-analize-in-razvoj.csl (#5963)
1393837 Update harvard-university-of-the-west-of-england.csl (#6013)
4474b0e Update frontiers-of-biogeography.csl (#6010)
736c6df Render status field (for forthcoming, in press, etc.) to date groups (#6005)
51ff3dc Create journal-of-contemporary-water-research-and-education.csl (#5912)
7387ed3 cell.csl: Add a macro for DOI and webpage accessed date. (#5999)
21e2177 Create respiratory-care-journal.csl (#5998)
d0a2846 Update archaeonautica.csl (#5987)
664784e Update cardiff-university-harvard.csl (#5982)
07ce91e improve journal-of-avian-biology.csl (#5984)
349a5d6 update based on latest version of ISO 690 standard (#5976)
d8c3725 Update mary-ann-liebert-vancouver.csl (#5981)
7b2e70d Update style-manual-australian-government.csl (#5975)
bebf50c Create extracellular-vesicles-and-circulating-nucleic-acids.csl (#5979)
cabcdd4 Update united-states-international-trade-commission.csl (#5967)
948c48a Update cardiff-university-harvard.csl (#5977)
d6bf535 Add name to label gun
cf0d930 Test label gun
2f40fcd Update the-journal-of-egyptian-archaeology.csl (#5964)
aa023e1 Create materials-research-society-bulletin.csl (#5917)
ea31783 Changed quotation marks to Spanish ones (#5968)
27245f9 AJA: Add "." suffix to citation layout. (#5971)
7726a40 Update american-medical-association.csl (#5970)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 9ff3112
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Find and Replace raises IllegalStateException
3 participants