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

Refactor EntryEditor & EntryEditorTab #2199

Merged
merged 9 commits into from
Nov 1, 2016
Merged

Refactor EntryEditor & EntryEditorTab #2199

merged 9 commits into from
Nov 1, 2016

Conversation

chriba
Copy link
Contributor

@chriba chriba commented Oct 26, 2016

I refactored many comments (some of them even referred to variables which do not exist anymore).
I removed some unused Methods, replaced some code with already existing methods and refactored a switch-case.

Copy link
Contributor

@tschechlovdev tschechlovdev left a comment

Choose a reason for hiding this comment

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

Please fix codacy. Beside the minor comments it's good from my side.

@@ -328,7 +327,8 @@ private void addSourceTab() {
srcPanel.setName(panelName);
tabbed.addTab(panelName, IconTheme.JabRefIcon.SOURCE.getSmallIcon(), srcPanel, toolTip);
tabs.add(srcPanel);
sourceIndex = tabs.size() - 1; // Set the sourceIndex variable.
// Set the sourceIndex variable
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comment can be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/**
* Corrected to fix [ 1594169 ] Entry editor: navigation between panels
*/
// Corrected to fix [ 1594169 ] Entry editor: navigation between panels
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be also deleted I think. Or you could replace it to not point to the bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tschechlovdev tschechlovdev added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels Oct 26, 2016
@grimes2 grimes2 mentioned this pull request Oct 27, 2016
6 tasks
@grimes2
Copy link
Contributor

grimes2 commented Oct 28, 2016

In the prompt Firstname Lastname... "others" is not translated. My proposal is to replace it by "et al.". This is international, so no translation needed.
prompt = String.format("%1$s and %1$s and others", Localization.lang("Firstname Lastname"))
->prompt = String.format("%1$s and %1$s and et al.", Localization.lang("Firstname Lastname"))

For the month field in BibLaTeX mode MM (1-12) is recommended, but also #mmm# (#jan#) is allowed for backward compatibilty to BibTeX. BibTeX mode only recommends #mmm#. So my proposal:
MM or #mmm#

addOptionalTab(type);
} else {
displayedOptionalFields.addAll(type.getPrimaryOptionalFields());
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 this removed here? If I remember this right this was needed to distinguish optional fields from fields that are displayed in the other fields tab?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Items of the list are never used, only added to it (displayedOptionalFields was only used in line 234, 240, 244, and 245; or am I missing something?)

@koppor
Copy link
Member

koppor commented Oct 28, 2016

and others is the bibtex string to procude et al. using bibtex/biblatex Therefore, this must not be translated. It is contained in the help to educate users to use this string.

@chriba
Copy link
Contributor Author

chriba commented Oct 28, 2016

Changed the prompt for Month-field.
I don't know about the prompts for the other fields, maybe you can point me into the right direction?

@grimes2
Copy link
Contributor

grimes2 commented Oct 28, 2016

From my side it looks ok now. I misinterpreted the author... prompt, because I didn't know the function of and others in BibTeX author/editor field. I found this documented in the official specification "Bibtexing".

@grimes2
Copy link
Contributor

grimes2 commented Oct 29, 2016

Just one small addition. I found following person name fields for BibLaTeX:

private static final List<String> BIBLATEX_PERSON_NAME_FIELDS = Arrays.asList(FieldName.AUTHOR, FieldName.EDITOR,
            FieldName.EDITORA, FieldName.EDITORB, FieldName.EDITORC, FieldName.TRANSLATOR, FieldName.ANNOTATOR,
            FieldName.COMMENTATOR, FieldName.INTRODUCTION, FieldName.FOREWORD, FieldName.AFTERWORD,
            FieldName.BOOKAUTHOR, FieldName.HOLDER, FieldName.SHORTAUTHOR, FieldName.SHORTEDITOR, FieldName.SORTNAME,
            FieldName.NAMEADDON, FieldName.ASSIGNEE);

I assume that the suggestion syntax
return String.format("%1$s and %1$s and others", Localization.lang("Firstname Lastname"));
is valid for all the mentioned fields above. What do you think? Should we add the missing person name fields to the switch construction?
Exception: nameaddon is a field (literal) not a list (name)

@chriba
Copy link
Contributor Author

chriba commented Oct 29, 2016

Now I reuse the already existing lists.

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.

LGTM

@tobiasdiez tobiasdiez merged commit 458ee79 into JabRef:master Nov 1, 2016
@chriba chriba deleted the refactorEntryEditor branch November 1, 2016 15:16
@koppor koppor mentioned this pull request Nov 2, 2016
@koppor
Copy link
Member

koppor commented Mar 10, 2017

This PR breaks Ctrl + A. See #2615

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.

6 participants