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.

Sign up for GitHub

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

Jump to bottom

Convert crossref editor to tag-like interface using JavaFX #2840

Merged
merged 6 commits into from
Jun 9, 2017
Merged

Convert crossref editor to tag-like interface using JavaFX #2840

merged 6 commits into from
Jun 9, 2017

Conversation

tobiasdiez
Copy link
Member

The crossref editor is the last one still implemented in Swing. With this PR it is converted to JavaFX and looks like this:

image

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@tobiasdiez tobiasdiez added this to the v4.0 milestone May 15, 2017
@tobiasdiez tobiasdiez changed the title [WIP] Convert crossref editor to tag-like interface using JavaFX Convert crossref editor to tag-like interface using JavaFX Jun 8, 2017
@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 8, 2017
@tobiasdiez
Copy link
Member Author

This PR is now ready-for-review. There are still some points that can be improved but I think for now this is ok:

  • Auto-completion to make it easier to add a new crossref.
  • Popup over linked entry with further information and a button that lets the user jump to this entry in the main table.

lenhard
lenhard approved these changes Jun 9, 2017
View reviewed changes
Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

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

I looked at the code and there are no major obstacles. I haven't tested this in a running JabRef, though, since I do not really use the crossref feature.

Somebody else should still test this.

src/main/java/org/jabref/gui/fieldeditors/LinkedEntriesEditor.java Outdated
linkedEntriesBar.setOnTagClicked((parsedEntryLink, mouseEvent) -> viewModel.jumpToEntry(parsedEntryLink));
Bindings.bindContentBidirectional(linkedEntriesBar.tagsProperty(), viewModel.linkedEntriesProperty());

//TODO: Add toolitp for tag: Localization.lang("Jump to entry")
Copy link
Member

Choose a reason for hiding this comment

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

I assume this TODO is legacy and not coming from the current PR, right?

src/main/java/org/jabref/gui/fieldeditors/LinkedEntriesEditorViewModel.java
}

public void jumpToEntry(ParsedEntryLink parsedEntryLink) {
// TODO: Implement jump to entry
Copy link
Member

Choose a reason for hiding this comment

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

Related to the comment above: I would prefer it very much to remove the commented-out code here directly. Way back in time when JabRef was still full of such stuff, we decided to remove commented-out code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok that policy is very understandable. In the current case, I'm not sure if we really should remove it. The "jump to entry" feature was present in previous versions and this PR removes it (temporarily). I don't see a nice way to re-implement it right now as we have no good interface to control the focus of the main table (I really don't want to directly use the JabRefFrame class). But removing the comment here and above results also in a change of the language files and the "Jump to entry" string has to be readded by the translators at some point in the future.

Should I still remove it? I don't have a strong opinion about it.

Copy link
Member

Choose a reason for hiding this comment

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

I see. It would be better to keep it in the language file. Thus, I would say it is ok to keep the code, but please extend the explanation, so that we know that this is not just the legacy stuff. Be verbose (no-problem with a multi-line comment) and explain the rationale and why it's problematic to re-add the code, like in you comment here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@tobiasdiez tobiasdiez merged commit b5f1e7d into master Jun 9, 2017
@tobiasdiez tobiasdiez deleted the crossrefEditor branch June 9, 2017 11:01
Siedlerchr added a commit that referenced this pull request Jun 10, 2017
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into sharelatex
1af149f 
* upstream/master:
  Convert crossref editor to tag-like interface using JavaFX (#2840)
  Fix #2847: Add scrollbars to entry editor
  Update Localization of Greek Translations (#2895)
@koppor koppor mentioned this pull request Sep 4, 2017
Closed
2 tasks
@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 mentioned this pull request Jan 23, 2022
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lenhard lenhard lenhard approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
v4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@tobiasdiez @lenhard