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

Improve change scanner #5665

Merged
merged 2 commits into from Nov 28, 2019
Merged

Improve change scanner #5665

merged 2 commits into from Nov 28, 2019

Conversation

tobiasdiez
Copy link
Member

@tobiasdiez tobiasdiez commented Nov 23, 2019

If a file is changed externally, JabRef is doing a 3-way merge between the last saved value vs current value in JabRef vs new value in file.
The way this is done is by first comparing the last saved file with the current (changed) file, and then trying to find additional/other changes in the currently open database. This is really complex and I believe there is some subtle bug in the logic leading to #5257.

With this PR, the change scanner is changed to a to simple 2-way merge between the current value in JabRef vs new value in file. This is way simpler and I hope this PR fixes #5257.
In a few edge cases, the 2-way merge has a small disadvantage. For example, consider the following steps:

  • User A opens file
  • User A changes field Author to User A and adds field Title with value new
  • User B opens file
  • User B changes field Author to User B
  • User B saves file

At this point, User A gets a message that the file changed with the following changes:

  • field Author changed from User A to User B
  • field Title was removed

The 3-way merge could detect that Title was newly added, and thus could automatically reapply this change. This is no longer possible with the simple 2-way merge.

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for bigger UI changes)
  • Checked documentation: Is the information available and up to date? If not: Issue created at https://github.com/JabRef/user-documentation/issues.

Move from 3-way merge (last saved value vs current value in JabRef vs new value in file) to simple 2-way merge (current value in JabRef vs new value in file).
@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 23, 2019
@Siedlerchr
Copy link
Member

We have to be careful how this works together with a shared database

@tobiasdiez
Copy link
Member Author

The shared databases have their own update/sync mechanism. But I do agree that this should be made consistent at some point.

@koppor
Copy link
Member

koppor commented Nov 23, 2019

I am making it consistent in my local dev branch at #5669. That one uses the merge entries dialog as it is the case in the shared database dialog. Does also cover the edge case by @tobiasdiez

@koppor koppor mentioned this pull request Nov 24, 2019
5 tasks
@koppor
Copy link
Member

koppor commented Nov 24, 2019

Is the reference file updated after the conflicts are reported? (Is it necessary? I would guess: yes, because changes reported should not be reported again - and the reference file is not the bib file on disk)

Is it possible to add some test cases?

@tobiasdiez
Copy link
Member Author

I removed the reference file completely...

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.

Tried it out locally (just local file system, not dropbox), works great.

Just to nitpick comments

src/main/java/org/jabref/gui/collab/ChangeScanner.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/collab/ChangeScanner.java Outdated Show resolved Hide resolved
@koppor
Copy link
Member

koppor commented Nov 28, 2019

How hard is it to embed the merge entries dialog at the yellow place below?

grafik

@Siedlerchr
Copy link
Member

@koppor The merge entries dialog is already consisting of a BorderPane called MergeEntries which can be embedded easily. See the MergeEntriesDialog

@koppor
Copy link
Member

koppor commented Nov 28, 2019

Minor wish: Esc should be enabled for closing the dialog.

(Refs #2949)

@koppor
Copy link
Member

koppor commented Nov 28, 2019

@Siedlerchr great! May I ask whether you could do it?

@Siedlerchr
Copy link
Member

I if can wait until this evening or afternoon I can try to add it.

@tobiasdiez
Copy link
Member Author

tobiasdiez commented Nov 28, 2019

Improved the PR based on the comments. Didn't change anything to the UI as this was not the subject of this PR. So, we are going ahead with this PR and further work on #5669 after the release of 5.0?

@koppor
Copy link
Member

koppor commented Nov 28, 2019

This PR has the advantage that it's finished and seems to work 🎉

Maybe we will offer both possibilities:

@Siedlerchr Can you do the integration of the merge entries dialog in a separate PR? - This evening is still long and I am really looking forward to it. Meanwhile, I'll work on the other PR.

@koppor koppor merged commit ce0e887 into master Nov 28, 2019
@koppor koppor deleted the improveDiffScan branch November 28, 2019 19:39
@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 28, 2019
@koppor koppor mentioned this pull request Jan 28, 2020
koppor pushed a commit that referenced this pull request Nov 1, 2021
12dbba5 Create tyndale-bulletin (#5673)
b0746db Create Engineered Regeneration (#5682)
e38b953 wikipedia citation template (#5662)
5e7f731 Create early-music-history.csl (#5679)
86443f3 Create zeitschrift-fur-politik.csl (#5676)
68f1996 Create annals-of-work-exposures-and-health.csl (#5666)
1ba9dc6 Create brazilian-journal-of-psychiatry.csl (#5672)
438f92c fix error for speech in ama styles (#5693)
7a0c2d3 set initialize-with-hyphen to false (#5689)
3bd2765 Update emu-austral-ornithology.csl (#5671)
31492b2 fix various errors in natura-croatica.csl (#5687)
94d6b23 Update iso690-author-date-cs.csl (#5677)
5d017da minor update on the "Haute école de gestion de Genève - ISO 690" style (#5665)
2cad8f6 add ibid/subsequent to comparative-politics.csl (#5669)
de0b116 Create taylor-and-francis-vancouver-national-library-of-medicine.csl (#5650)
ed87f99 Update bulletin-de-correspondance-hellenique.csl (#5663)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 12dbba5
koppor pushed a commit that referenced this pull request Nov 15, 2021
0654e16 Create scandinavian-journal-of-information-systems.csl (#5716)
ce2d537 Update journal-of-computer-applications-in-archaeology.csl (#5715)
755d3d3 Create human-rights-law-review.csl (#5626)
0feda94 Create journal-of-intercultural-studies.csl (#5709)
ae4756d Update acta-universitatis-agriculturae-sueciae.csl (#5713)
323d9ac Update mohr-siebeck-recht.csl (#5559)
15530a8 Bch corr (#5712)
094a1af Create forschungsjournal-soziale-bewegungen-fjsb.csl (#5699)
cb91566 initialize authors and editors (#5714)
2d5cfff Create cancer-biomarkers.csl (#5703)
5e264d5 Update multidisciplinary-digital-publishing-institute.csl (#5708)
46e961f Create klinische-padiatrie.csl (#5711)
e81e877 Create bulletin-archeologique-des-ecoles-francaises-a-l-etranger.csl (#5704)
0029c5a Create polar-research.csl 🧊 (#5702)
7db1361 Update vancouver-imperial-college-london.csl (#5641)
b953e9f Update iso690-author-date-fr-no-abstract.csl (#5706)
91eda8c Update thieme-german.csl (#5710)
ebe0787 Update harvard-imperial-college-london.csl (#5643)
2d4db76 Fix UNESCO IIEP in text
436cbf4 Create revue-archeologique-de-narbonnaise.csl (#5688)
5150bcf Create journal-of-computer-assisted-tomography.csl (#5690)
dd6f050 Create anti-trafficking-review.csl (#5658)
08e622f Create the-angle-orthodontist.csl (#5685)
c6a1907 journal-of-palm-oil-research.csl fix several errors (#5686)
6cbe29d Create bern-university-of-applied-sciences-school-of-agricultural-for… (#5684)
f590dc1 Update biomed-central.csl (#5701)
1efce81 Update turabian-author-date.csl (#5695)
12dbba5 Create tyndale-bulletin (#5673)
b0746db Create Engineered Regeneration (#5682)
e38b953 wikipedia citation template (#5662)
5e7f731 Create early-music-history.csl (#5679)
86443f3 Create zeitschrift-fur-politik.csl (#5676)
68f1996 Create annals-of-work-exposures-and-health.csl (#5666)
1ba9dc6 Create brazilian-journal-of-psychiatry.csl (#5672)
438f92c fix error for speech in ama styles (#5693)
7a0c2d3 set initialize-with-hyphen to false (#5689)
3bd2765 Update emu-austral-ornithology.csl (#5671)
31492b2 fix various errors in natura-croatica.csl (#5687)
94d6b23 Update iso690-author-date-cs.csl (#5677)
5d017da minor update on the "Haute école de gestion de Genève - ISO 690" style (#5665)
2cad8f6 add ibid/subsequent to comparative-politics.csl (#5669)
de0b116 Create taylor-and-francis-vancouver-national-library-of-medicine.csl (#5650)
ed87f99 Update bulletin-de-correspondance-hellenique.csl (#5663)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 0654e16
Siedlerchr added a commit that referenced this pull request Nov 16, 2021
* Squashed 'buildres/csl/csl-styles/' changes from 3a6a0a7..0654e16

0654e16 Create scandinavian-journal-of-information-systems.csl (#5716)
ce2d537 Update journal-of-computer-applications-in-archaeology.csl (#5715)
755d3d3 Create human-rights-law-review.csl (#5626)
0feda94 Create journal-of-intercultural-studies.csl (#5709)
ae4756d Update acta-universitatis-agriculturae-sueciae.csl (#5713)
323d9ac Update mohr-siebeck-recht.csl (#5559)
15530a8 Bch corr (#5712)
094a1af Create forschungsjournal-soziale-bewegungen-fjsb.csl (#5699)
cb91566 initialize authors and editors (#5714)
2d5cfff Create cancer-biomarkers.csl (#5703)
5e264d5 Update multidisciplinary-digital-publishing-institute.csl (#5708)
46e961f Create klinische-padiatrie.csl (#5711)
e81e877 Create bulletin-archeologique-des-ecoles-francaises-a-l-etranger.csl (#5704)
0029c5a Create polar-research.csl 🧊 (#5702)
7db1361 Update vancouver-imperial-college-london.csl (#5641)
b953e9f Update iso690-author-date-fr-no-abstract.csl (#5706)
91eda8c Update thieme-german.csl (#5710)
ebe0787 Update harvard-imperial-college-london.csl (#5643)
2d4db76 Fix UNESCO IIEP in text
436cbf4 Create revue-archeologique-de-narbonnaise.csl (#5688)
5150bcf Create journal-of-computer-assisted-tomography.csl (#5690)
dd6f050 Create anti-trafficking-review.csl (#5658)
08e622f Create the-angle-orthodontist.csl (#5685)
c6a1907 journal-of-palm-oil-research.csl fix several errors (#5686)
6cbe29d Create bern-university-of-applied-sciences-school-of-agricultural-for… (#5684)
f590dc1 Update biomed-central.csl (#5701)
1efce81 Update turabian-author-date.csl (#5695)
12dbba5 Create tyndale-bulletin (#5673)
b0746db Create Engineered Regeneration (#5682)
e38b953 wikipedia citation template (#5662)
5e7f731 Create early-music-history.csl (#5679)
86443f3 Create zeitschrift-fur-politik.csl (#5676)
68f1996 Create annals-of-work-exposures-and-health.csl (#5666)
1ba9dc6 Create brazilian-journal-of-psychiatry.csl (#5672)
438f92c fix error for speech in ama styles (#5693)
7a0c2d3 set initialize-with-hyphen to false (#5689)
3bd2765 Update emu-austral-ornithology.csl (#5671)
31492b2 fix various errors in natura-croatica.csl (#5687)
94d6b23 Update iso690-author-date-cs.csl (#5677)
5d017da minor update on the "Haute école de gestion de Genève - ISO 690" style (#5665)
2cad8f6 add ibid/subsequent to comparative-politics.csl (#5669)
de0b116 Create taylor-and-francis-vancouver-national-library-of-medicine.csl (#5650)
ed87f99 Update bulletin-de-correspondance-hellenique.csl (#5663)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 0654e16

* Squashed 'buildres/csl/csl-locales/' changes from 0cc3885f61..d5ee85de8e

d5ee85de8e Period after Übers. added (#241)

git-subtree-dir: buildres/csl/csl-locales
git-subtree-split: d5ee85de8e74d4109509014758b6f496a968ff03

* fix  merge error

Co-authored-by: github actions <jabrefmail+webfeedback@gmail.com>
Co-authored-by: Siedlerchr <siedlerkiller@gmail.com>
koppor pushed a commit that referenced this pull request Dec 1, 2021
3bb4b5f infoclio.ch styles for German: remove non-breaking space delimiters (#5754)
adf28db Create journal-of-health-care-for-the-poor-and-underserved.csl (#5752)
0713a8e Update chinese-gb7714-2005-numeric.csl (#5737)
1cd3754 Update china-national-standard-gb-t-7714-2015-author-date.csl (#5746)
c2536b7 Update china-national-standard-gb-t-7714-2015-numeric.csl (#5745)
f8c1392  Create steel-research-international.csl (#5720)
21fe1f5 Create asian-myrmecology.csl (#5718)
91e9e2b Update harvard-university-of-the-west-of-england.csl (#5734)
dd453d1 fix minor erros in polar-research.csl (#5730)
038a8f5 Remove group around no-date cluster (#5731)
0710b51 remove et-al from bibtex.csl (#5728)
bbd703d Add editorial-director to universite-laval-departement-des-sciences-historiques.csl (#5727)
58ea430 Create german-journal-of-agricultural-economics.csl (#5717)
0654e16 Create scandinavian-journal-of-information-systems.csl (#5716)
ce2d537 Update journal-of-computer-applications-in-archaeology.csl (#5715)
755d3d3 Create human-rights-law-review.csl (#5626)
0feda94 Create journal-of-intercultural-studies.csl (#5709)
ae4756d Update acta-universitatis-agriculturae-sueciae.csl (#5713)
323d9ac Update mohr-siebeck-recht.csl (#5559)
15530a8 Bch corr (#5712)
094a1af Create forschungsjournal-soziale-bewegungen-fjsb.csl (#5699)
cb91566 initialize authors and editors (#5714)
2d5cfff Create cancer-biomarkers.csl (#5703)
5e264d5 Update multidisciplinary-digital-publishing-institute.csl (#5708)
46e961f Create klinische-padiatrie.csl (#5711)
e81e877 Create bulletin-archeologique-des-ecoles-francaises-a-l-etranger.csl (#5704)
0029c5a Create polar-research.csl 🧊 (#5702)
7db1361 Update vancouver-imperial-college-london.csl (#5641)
b953e9f Update iso690-author-date-fr-no-abstract.csl (#5706)
91eda8c Update thieme-german.csl (#5710)
ebe0787 Update harvard-imperial-college-london.csl (#5643)
2d4db76 Fix UNESCO IIEP in text
436cbf4 Create revue-archeologique-de-narbonnaise.csl (#5688)
5150bcf Create journal-of-computer-assisted-tomography.csl (#5690)
dd6f050 Create anti-trafficking-review.csl (#5658)
08e622f Create the-angle-orthodontist.csl (#5685)
c6a1907 journal-of-palm-oil-research.csl fix several errors (#5686)
6cbe29d Create bern-university-of-applied-sciences-school-of-agricultural-for… (#5684)
f590dc1 Update biomed-central.csl (#5701)
1efce81 Update turabian-author-date.csl (#5695)
12dbba5 Create tyndale-bulletin (#5673)
b0746db Create Engineered Regeneration (#5682)
e38b953 wikipedia citation template (#5662)
5e7f731 Create early-music-history.csl (#5679)
86443f3 Create zeitschrift-fur-politik.csl (#5676)
68f1996 Create annals-of-work-exposures-and-health.csl (#5666)
1ba9dc6 Create brazilian-journal-of-psychiatry.csl (#5672)
438f92c fix error for speech in ama styles (#5693)
7a0c2d3 set initialize-with-hyphen to false (#5689)
3bd2765 Update emu-austral-ornithology.csl (#5671)
31492b2 fix various errors in natura-croatica.csl (#5687)
94d6b23 Update iso690-author-date-cs.csl (#5677)
5d017da minor update on the "Haute école de gestion de Genève - ISO 690" style (#5665)
2cad8f6 add ibid/subsequent to comparative-politics.csl (#5669)
de0b116 Create taylor-and-francis-vancouver-national-library-of-medicine.csl (#5650)
ed87f99 Update bulletin-de-correspondance-hellenique.csl (#5663)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 3bb4b5f
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.

Jabref reports inexistent changes
3 participants