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

Add source to changes dialog #9533

Merged
merged 37 commits into from
Feb 7, 2023
Merged

Add source to changes dialog #9533

merged 37 commits into from
Feb 7, 2023

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Jan 5, 2023

Fixes #9509 and make Olly happy :)
Does not yet fix issue #9534

I added source tabs for deleted and added.
Remove for entry change.
For entry change I thought about showing the diff in the preview or the source. No idea how this would work. On the other hand the current status with the Preview was confusing because it was not clear which entry shows

Deleted:
grafik

Added
grafik

Changed:
grafik

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 5, 2023
setBottomAnchor(previewViewer, 8d);

getChildren().setAll(previewViewer);
// TOOD: Show maybe diff of preview and source?
Copy link
Member

Choose a reason for hiding this comment

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

Why is the Preview removed in the change details view? Looks like a regression to me

Copy link
Member Author

Choose a reason for hiding this comment

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

For entry change I thought about showing the diff in the preview or the source. No idea how this would work.

On the other hand the current status with the Preview was confusing because it was not clear to the user which entry is shown in the preview

Copy link
Member Author

Choose a reason for hiding this comment

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

And for entry change, the merge dialog is basically showing all relevant info

@calixtus
Copy link
Member

calixtus commented Jan 5, 2023

Also missing indentation

@Siedlerchr
Copy link
Member Author

The entry editor bibtex code area has some padding, guess this needs to be added here as well

@koppor
Copy link
Member

koppor commented Jan 5, 2023

I have an "empty" preview?

image

@Siedlerchr
Copy link
Member Author

@koppor READ THE TEXT: FOR ADDED AND DELETED!

@Siedlerchr
Copy link
Member Author

After thinking about this I also added the view for entry change, but this will only show the new entry

@HoussemNasri
Copy link
Member

HoussemNasri commented Jan 5, 2023

@Siedlerchr It would be more useful to show both, the Disk and JabRef versions. I was thinking of a nested tabpane inside the source and preview tabs that shows the two versions. Me and ThilotE had a discussion about this.

import org.jabref.model.database.BibDatabaseContext;
import org.jabref.preferences.PreferencesService;

public final class EntryAddDetailsView extends DatabaseChangeDetailsView {
Copy link
Member

Choose a reason for hiding this comment

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

You can still reuse the details layout for EntryDelete and EntryAdd changes without deleting this. I believe it is better to have a consistent structure and maintain a separate class for each change's details layout.

@koppor
Copy link
Member

koppor commented Jan 5, 2023

@koppor READ THE TEXT: FOR ADDED AND DELETED!

Ah, sorry, I thought, this PR fixed #9534.

add to string methods
simplify enum parsing
HoussemNasri and others added 3 commits January 16, 2023 11:48
…ialog

* upstream/main:
  Bump xmlunit-core from 2.9.0 to 2.9.1
  Bump mockito-core from 4.11.0 to 5.0.0
  Bump xmlunit-matchers from 2.9.0 to 2.9.1
  Bump junit-platform-launcher from 1.9.1 to 1.9.2
  Squashed 'buildres/csl/csl-styles/' changes from 43566f2..50eea55b2c
  New translations JabRef_en.properties (Portuguese, Brazilian) (#9559)
  Changed the color of light-color-text and highlighted text in … (#9546)
  New translations JabRef_en.properties (Portuguese, Brazilian) (#9557)
  chore: improve debug output in powershell starter script (#9550)
  Show development information (#9555)
  Observable Preferences T (NameDisplayPreferences, MainTablePreferences, ColumnPreferences) (#9535)
@Siedlerchr
Copy link
Member Author

Siedlerchr commented Jan 16, 2023

Clarification: Split pane to show both tabs at the same time:
old bibtex + old preview | new bibtex + new preview
sync tab view

…ialog

* upstream/main: (22 commits)
  Avoid using Globals.entryTypeManager
  try with liberica jdk
  test with unsigned image
  adjust dmg scpt and add mac sign id
  don't use keychain profile
  use older one
  return true in case security keychain not existing
  readd old codesiginng steps
  codesign image with timestamp
  read app image
  fuu yaml
  again
  try contains
  try to add braces
  fix error in yaml
  test new codesigning
  Test entries equality based on their citation key
  Fix failing tests
  Detect more cases when an entry is modified
  Refactor
  ...
HoussemNasri
HoussemNasri previously approved these changes Jan 21, 2023
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.

Very cool feature!

I only have a question about why BibTeX is editable, otherwise, we can merge

Comment on lines +82 to +91
private void showOrHideEqualFields() {
for (FieldRowView row : fieldRows) {
if (toolbar.shouldHideEqualFields()) {
if (row.hasEqualLeftAndRightValues()) {
row.hide();
}
} else {
row.show();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I found it a bit hard to locate the code that shows or hides equal fields, so I moved it into its own method. It is a little slower than the old code, but since the number of fields per entry is small, it shouldn't make any difference.

@calixtus
Copy link
Member

About the UI: there should be some indication which entry is the modified an the unmodified / old and new entry in the splitpane

…ialog

* upstream/main:
  Fixed ADS tests
  Bump junit-jupiter from 5.9.1 to 5.9.2 (#9579)
  Bump actions/configure-pages from 2 to 3
  open office csv format correction for abstract column (#9570)
  Update CHANGELOG.md
  add change log entry for the given change
  typo removed (#9577)
  remove filter that consumes mouse rightclick
@Siedlerchr
Copy link
Member Author

I'm just unsure if this is the right order if "old entry" really means in JabRef or the file on disk...

grafik

@HoussemNasri
Copy link
Member

@Siedlerchr Yeah, that's correct; the old entry is the entry on JabRef.

@calixtus
Copy link
Member

Maybe sthg like "in jabref" and "imported"? Since an SSD is no "disc"...

@ThiloteE
Copy link
Member

Devcall decision:

in JabRef
on Disc
imported
(or however we wanna call it)

should - if possible - be put into a row above it's current position. It should be something like a "headline" to the two tabs below.

…ialog

* upstream/main:
  Remove overquoting in jabrefHost on Linux (#9590)
  Squashed 'buildres/csl/csl-locales/' changes from 3961332022..9b9366b71b
  Squashed 'buildres/csl/csl-styles/' changes from 50eea55b2c..b111f31efe
  Checkstyle correction
  checkstyle issue removal
  removed unwanted resources related to BibTeXml
  Bump tinylog-impl from 2.5.0 to 2.6.0
  Bump slf4j-tinylog from 2.5.0 to 2.6.0
  Exporter Bibtex Xml removed
  Importer Format changes
todo needs styling

rename hide equal fields to  -> Only show changed fields
@Siedlerchr
Copy link
Member Author

Siedlerchr commented Feb 3, 2023

Now added some padding as well:

grafik

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.

In general: works. One small strange thing.

Siedlerchr and others added 4 commits February 5, 2023 22:49
…rceTab.java

Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>
… addSourceToChangesDialog

* upstream/addSourceToChangesDialog:
  Update src/main/java/org/jabref/gui/collab/entrychange/PreviewWithSourceTab.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicateFinder entry-preview export / save status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

External Changes resolver should show the bib entry source next to the preview
5 participants