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

Convert New entry dialog to javafx #4312

Merged
merged 22 commits into from
Sep 9, 2018
Merged

Convert New entry dialog to javafx #4312

merged 22 commits into from
Sep 9, 2018

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Sep 2, 2018

Followup from #4266

bibtex +custom
grafik

biblatex + custom
grafik


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

1160300608 and others added 6 commits August 15, 2018 10:25
…wEntryDialog

* 'master' of https://github.com/1160300608/jabref:
  fix pdfImport old dialog problem
  fix order and empty else statement
  delete useless class file
  convert the NewEntryType Dialog to javafx

# Conflicts:
#	src/main/java/org/jabref/gui/EntryTypeDialog.java
#	src/main/java/org/jabref/gui/actions/NewEntryAction.java
add title pane
add bindings
Create view model todo

Co-authored-by: 1160300608 <18846010223@163.com>
@Siedlerchr Siedlerchr changed the title Convert New entry dialog to javafx [WIP] Convert New entry dialog to javafx Sep 2, 2018
…fetchr

bind combobox directly to fetcher
improve binding
pass preferences to constructor
delete obsolete gui test class and old dialog
@Siedlerchr Siedlerchr changed the title [WIP] Convert New entry dialog to javafx Convert New entry dialog to javafx Sep 3, 2018
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 3, 2018
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 a lot for investing the time to get the PR in. Sadly, there are still a lot of (small) code issues that need to be fixed.

@@ -772,10 +772,13 @@ public BibEntry newEntry(EntryType type) {
EntryType actualType = type;
if (actualType == null) {
// Find out what type is wanted.
final EntryTypeDialog etd = new EntryTypeDialog(frame);
//final EntryTypeDialog etd = new EntryTypeDialog(frame);
Copy link
Member

Choose a reason for hiding this comment

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

Remove the old commented code

Copy link
Member

Choose a reason for hiding this comment

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

Rename etv.

//etd.setVisible(true);
//actualType = etd.getChoice();
etv.showAndWait();
actualType = etv.getChoice();
Copy link
Member

Choose a reason for hiding this comment

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

It is more in line with the JavaFX way, if EntryTypeView derives from Dialog<EntryType> and then you can get the choice as the return value of showAndWait().


@FXML private ButtonType generateButton;
@FXML private TextField idTextField;
@FXML private ComboBox<IdBasedFetcher> comboBox;
Copy link
Member

Choose a reason for hiding this comment

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

better name

@FXML private TitledPane bibTexTitlePane;
@FXML private TitledPane ieeeTranTitlePane;
@FXML private TitledPane customTitlePane;
@FXML private VBox vBox;
Copy link
Member

Choose a reason for hiding this comment

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

here too

private final JabRefPreferences prefs;

private EntryType type;
private Task<Optional<BibEntry>> fetcherWorker = new FetcherWorker();
Copy link
Member

Choose a reason for hiding this comment

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

move worker to the view model

@@ -772,10 +772,13 @@ public BibEntry newEntry(EntryType type) {
EntryType actualType = type;
if (actualType == null) {
// Find out what type is wanted.
final EntryTypeDialog etd = new EntryTypeDialog(frame);
//final EntryTypeDialog etd = new EntryTypeDialog(frame);
Copy link
Member

Choose a reason for hiding this comment

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

Is this method even used with null argument? At least the code below suggests that the JabRefFrame uses the NewEntryAction and does not calls this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just found out that this method and the insertEntry method are doing roughly the same, I think it makes sense to combine their functionaltiy

@@ -31,13 +37,15 @@ public void execute() {
return;
}

EntryTypeDialog typeChoiceDialog = new EntryTypeDialog(jabRefFrame);
typeChoiceDialog.setVisible(true);
//EntryTypeDialog typeChoiceDialog = new EntryTypeDialog(jabRefFrame);
Copy link
Member

Choose a reason for hiding this comment

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

remove old code

@@ -224,8 +224,10 @@ private void doContentImport(String fileName, List<BibEntry> res) {

private Optional<BibEntry> createNewEntry() {
// Find out what type is desired
EntryTypeDialog etd = new EntryTypeDialog(frame);
etd.setVisible(true);
//EntryTypeDialog etd = new EntryTypeDialog(frame);
Copy link
Member

Choose a reason for hiding this comment

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

here too

//EntryTypeDialog etd = new EntryTypeDialog(frame);
EntryTypeView etd = new EntryTypeView(frame.getCurrentBasePanel(), frame.getDialogService(), Globals.prefs);
//etd.setVisible(true);
etd.showAndWait();
Copy link
Member

Choose a reason for hiding this comment

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

The code below looks like yet another place with code duplication (which is however slightly different to the one above, eg. timestamp generation).

getBoolean(ENFORCE_LEGAL_BIBTEX_KEY),
getKeyPattern(),
getKeywordDelimiter());
get(KEY_PATTERN_REGEX),
Copy link
Member

Choose a reason for hiding this comment

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

the code formatting was ok before...this indention looks ugly to be honest.

TODO: merge insertentry and newEntry in basePanel
@Siedlerchr
Copy link
Member Author

Adressed all comments, The Duplicate check is actually different handled and is slightly coupled to the import inspection dialog. This is stuff for a new PR.

@Siedlerchr
Copy link
Member Author

I would like to merge this now, if no further comments are added.,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants