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

Show merge entries for modified entries #5688

Merged
merged 34 commits into from
Dec 29, 2019
Merged

Show merge entries for modified entries #5688

merged 34 commits into from
Dec 29, 2019

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Nov 30, 2019

When an entry's content changes, the merge dialog is now displayed.
For added and removed, it's not possible to show the merge dialog.

Follow up from #5665

grafik

@koppor
Copy link
Member

koppor commented Dec 4, 2019

May I ask what's missing here?

@Siedlerchr
Copy link
Member Author

I want to expand it for delete and added. Not sure if this will work.

* upstream/master:
  Fix eclipse classpath build file
  New translations JabRef_en.properties (French) (#5709)
  Check for existing master branch
  Disable failing MathSciNet test due to missing subscription
  Try to force creating reflog
  Change travis to github in Readme
  Use GitVersion to increment build numbers (#5682)
  New Crowdin translations (#5687)
  Bump checkstyle from 8.26 to 8.27 (#5694)
  Bump byte-buddy-parent from 1.10.3 to 1.10.4
  Bump classgraph from 4.8.54 to 4.8.58
  Bump unirest-java from 3.1.04 to 3.2.00
  Bump richtextfx from 0.10.2 to 0.10.3
  Bump mockito-core from 3.1.0 to 3.2.0
  Update JavaFX (#5690)
  Run outdated dependency check after dpendabot
  Update outdatedDependencies.md
* upstream/master:
  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)
  Add CHANGELOG.md entry for Oracle
  Enable oracle tests (#5683)
* upstream/master: (21 commits)
  New Crowdin translations (#5729)
  Update license of Oracle JDBC
  Changes string representation when detecting custom entry types. (#5741)
  Try to fix GitVersion
  Also run workflow on tag creation
  Add README.md with an initial help to work on command line para… (#5720)
  Bugfix for Multiscreen (#5738)
  Fix #2868 - keep source groups selected after drag and drop
  retrigger build
  Squashed 'src/main/resources/csl-styles/' changes from f71cd32..49a1841
  try to fix pull of csl styles
  try to fix csl update
  Fixed untranslated Strings issue #5701
  Fix #5603 - account for file extension in file name length (#5726)
  New translations JabRef_en.properties (French)
  Bump unirest-java from 3.2.00 to 3.3.00
  Bump postgresql from 42.2.8 to 42.2.9
  Bump tika-core from 1.22 to 1.23
  Fix upload to GitHub artifacts (#5712)
  Try to fix csl update (#5718)
  ...
@koppor
Copy link
Member

koppor commented Dec 20, 2019

Does this interfere with #5770? @Siedlerchr Maybe, you find some more time to finish this PR this year? 🎉 I would be very happy. I think, the community would really benefit from it.

@Siedlerchr
Copy link
Member Author

Yes. I would have continued with this PR, but the last time I was unable to trigger the changes dialog at all even with directly modifying the file in an editor . So this depends on

# By Tobias Diez (11) and others
# Via GitHub (1) and Tobias Diez (1)
* upstream/master: (29 commits)
  Improve things arround change detection (#5770)
  Revert "Update to most recent journal abbreviation list" (#5769)
  Various fixes to the dark theme (#5764)
  Bump mockito-core from 3.2.0 to 3.2.4 (#5760)
  Bump classgraph from 4.8.58 to 4.8.59 (#5761)
  Improve dependency update rules
  Update jpackage to build 27 (#5758)
  Persistent column sortorder (#5730)
  Fix medline fetcher/importer when using installer (#5752)
  New Crowdin translations (#5751)
  Bump byte-buddy-parent from 1.10.4 to 1.10.5 (#5750)
  Fix checkstyle
  Fix filename
  Update to most recent journal abbreviation list
  Remove obsolete string
  Revert "Switch back to development"
  Switch back to development
  Next development cycle
  Release 5.0-beta (#5684)
  Fix multiple entries allowed in crossref (issue #5284) (#5724)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java
#	src/main/java/org/jabref/gui/collab/EntryChangeViewModel.java
@Siedlerchr
Copy link
Member Author

Siedlerchr commented Dec 20, 2019

After #4877 is now working again, I could finally test this again.
@koppor
The merge dialog is now shown when an entry's fields are modified.
For adding and deletion it's not possible to show the dialog.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 20, 2019
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.

Nice! Code looks good to me, only a few very minor remarks / questions.

Can you please add a bit of space around the control since otherwise it looks a bit cramped.

@Siedlerchr Siedlerchr changed the title [WIP] Show merge entries for modified entries Show merge entries for modified entries Dec 20, 2019
@Siedlerchr
Copy link
Member Author

Siedlerchr commented Dec 20, 2019

I beautified the dialog a bit. Looks better now. I replaced the screenshot in the first post

@koppor
Copy link
Member

koppor commented Dec 20, 2019

Default size:

grafik

Here, I cannot change the colum width and I dont have a higt what "N..." "..." and "Ri..." stand for.

I would have expected that i can tweak like in Excel.

Maybe, it is not necessary - maybe, the "Left entry" column can be reduced in their width.

Maximized:

grafik

I would have expected that the left "external changes" enlarges until the middle of the screen.

General comment

Can't the space between the buttons "Dismiss changes" and "Accept changes" be larger? It is possible that one hits "Dismiss changes" by accident and one can't trigger a rescan of the changes.

@koppor
Copy link
Member

koppor commented Dec 20, 2019

The buttons should be centered. Reason: I currently thought that I accept the changes in the "Diff view" and that I am lead to the next entry diff to review.

Similar to when going through errors in IntelliJ. One is always guided to the next until one is finished.

Maybe, a button "Accept and next" and "Reject and next" would be good.

Since I am used the pair "Accept" and "Reject" (from science (papers) and IntelliJ), I propose to reword "DiscardDismiss" to "Reject"

@koppor
Copy link
Member

koppor commented Dec 20, 2019

Maybe, it also helps to replace L N R by ◄ ∅ ►

(Source http://xahlee.info/comp/unicode_arrows.html and https://www.fileformat.info/info/unicode/char/2205/browsertest.htm)

@koppor
Copy link
Member

koppor commented Dec 25, 2019

Currently looks as follows:

grafik

I think, the parameters for "left" and "right" have been chosen with some thoughts. The thing is, that the default behavior without any more user interaction should be that all changes from disk should go in. With the current implementation, all modifications in entries are discarded, because "left" is the "in memory" bibentry and "right" the entry from disk. "Left" is chosen as default, so "in memory" always wins.

I propose to keep "left" and "right" as is (as right is the future), but have the default activation for "right".

@Siedlerchr Is that possible?

With that small update, I am OK to merge in. I would still beg to keep working on it to have "Left" and "Right" be replaced with "In JabRef" and "On Disk". (This is switched as above now as I learned what left/right means. Currently, I think, it is OK that right is the new content on disk)

@Siedlerchr
Copy link
Member Author

We can also switch the entry order, switching left and right argument.
That would indeed make more sense.

change labels and preselegt right side
Select first change
fix l10n
@koppor
Copy link
Member

koppor commented Dec 26, 2019

Here the current state with a slight modification by me:

grafik

@koppor
Copy link
Member

koppor commented Dec 26, 2019

I would love to see the middle radio button centered, but I do not find the code.

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.

Generally looks good to me.

use jabref icons instead of unicode
* upstream/master:
  Improve ignored dependencies
  Run tests also on PR
  Add latex2unicode formatter for displaying previews (#5785)
@Siedlerchr Siedlerchr merged commit 5f7f133 into master Dec 29, 2019
@Siedlerchr Siedlerchr deleted the changemergesdialog branch December 29, 2019 18:31
@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 1, 2020
@koppor koppor mentioned this pull request Feb 27, 2020
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.

3 participants