-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Create sensible default settings for "Enable save actions" and "Cleanup" dialogs #2051
Conversation
@@ -73,6 +73,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `# | |||
- Fixed [#1958](https://github.com/JabRef/jabref/issues/1958): Verbatim fields are no longer checked for HTML encoded characters by integrity checks | |||
- Fixed [#1937](https://github.com/JabRef/jabref/issues/1937): If no help page for the current chosen language exists, the english help page will be shown | |||
- Fixed [#1996](https://github.com/JabRef/jabref/issues/1996): Unnecessary other fields tab in entry editor removed (BibTeX mode) | |||
- Fixed [#128](https://github.com/koppor/jabref/issues/128): Sensible default settings for "Enable save actions" and "Cleanup" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use koppor/#128
as normal numbers reference to the JabRef /jabref repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done it 🐨
Sorry, but not all settings from the original dialog have been included:
|
I don't like all these "title" formatters. I would propose to have a minimal default preset which does not change to much. For example, "protect terms" is really hard to revert by hand. Thus I would only include the formatters which do a minimal job which probably everybody wants (i.e normalized months, dates and pages) but not more. |
@koppor oliver what do you think about tobiasdiez idea ??? 😏 |
I have a question about the function of each checkbox. Which change will be setted, if i select one. Maybe like "Convert 1st, 2nd, ... to real superscript, what will be change if i select it and i approve with "OK".
Please go through line by line and state which new converter you chose. Thy for explaining 😄 |
To real superscripts is ordinals to latex. You could also have tried it out
by creating an inprocredings entry containing a bootitle such as "2nd
conference on something". Or just testing it with available bibtex files
and checking the deltas (e.g., using git)
|
@tobiasdiez We should have buttons for presets. Minimal, Recommended (as in
old JabRef versions) and Maximal (if we can come up w something useful)
I think, I really want the ordinals thing included. Don't like the protect
terms. Just want to put automatically braces around the complete title. But
we don't have a formatter for that...
|
@koppor Event without the checkbox "convert to real superscripts" you can add the entryfield manually to the fieldformatter with the converter you chose eg.
with this everyone can add his own converter for his own use thats why the checkbox "convert 1st, 2nd, ... to real superscripts" is not needed. |
I know how to use the dialog. I want sensible defaults - see JabRef#128 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still a changelog entry due to merge I guess, other than that LGTM.
@@ -96,6 +96,8 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `# | |||
- Fixed [#2104](https://github.com/JabRef/jabref/issues/#2104): Crash after saving BibTeX source with parsing error | |||
- Fixed [#2109](https://github.com/JabRef/jabref/issues/#2109): <kbd>Ctrl-s</kbd> doesn't trigger parsing error message | |||
- Fixed RTFChars would only use "?" for characters with unicode over the value of 127, now it uses the base character (é -> e instead of ?) | |||
- Fixed [#1996](https://github.com/JabRef/jabref/issues/1996): Unnecessary other fields tab in entry editor removed (BibTeX mode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
The convert checkbox of old cleanup entries dialog is defined now in field formatter like following:
All are done, please take a look for merge 🎅 |
Can the unicode converter (line 2) also be run on Run filter on title keeping the case --> This is "protect terms". Please enable it on title, journal, and journal title. |
@@ -17,6 +17,9 @@ | |||
// author is not set): | |||
public static final String FIELD_SEPARATOR = "/"; | |||
|
|||
public static final String ABSTRACT_ALL_FIELD = "all"; | |||
public static final String ABSTRACT_ALL_TEXT_FIELDS_FIELD = "all_text_fields"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be named all-text-fields
. I think dash ("-
") is more intuitive than underscore ("_
").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
private List<FieldChange> cleanupAllTextFields(BibEntry entry) { | ||
List<FieldChange> fieldChanges = new ArrayList<>(); | ||
Set<String> fields = entry.getFieldNames(); | ||
fields.removeAll(Arrays.asList(FieldName.DOI, FieldName.FILE, FieldName.URL, FieldName.URI)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please introduce a class constant ALL_TEXT_FIELDS containing these fields. Please also add ISBN and ISSN, MONTH, DATE, YEAR,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
Some comments:
|
I'm really no big fan of the proposed default values:
In my opinion all these formatters depend on too much assumptions (BibTeX vs BibLaTeX and used packages) to be default. I find it better to have a minimal default preset and let the user decide which additional formatters he wants. Adding something feels better than removing something. |
@koppor: all-text-fields will be function with unicode converter |
@koppor: i have implemented recommend button for BibLatex /BibTex and refactoring the default setting |
String mode; | ||
|
||
if (isBibTex) { | ||
mode = "BibTex"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be "BibTeX"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
if (isBibTex) { | ||
mode = "BibTex"; | ||
} else { | ||
mode = "BibLaTex"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be "BibLaTeX"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
mode = "BibLaTex"; | ||
} | ||
|
||
recommendButton = new JButton(Localization.lang("Recommend for %0", mode)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "Recommended"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
DEFAULT_SAVE_ACTIONS = new FieldFormatterCleanups(false, defaultFormatters); | ||
|
||
List<FieldFormatterCleanup> recommendBibTexFormatters = new ArrayList<>(defaultFormatters); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also rename variable to recommendedBibTeXFormatters
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
recommendBibTexFormatters.add(new FieldFormatterCleanup(FieldName.EDITOR, new UnicodeToLatexFormatter())); | ||
RECOMMEND_BIBTEX_ACTIONS = new FieldFormatterCleanups(false, recommendBibTexFormatters); | ||
|
||
List<FieldFormatterCleanup> recommendBibLatexFormatters = new ArrayList<>(defaultFormatters); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also rename variable to recommendedBibLaTeXFormatters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
…tabase properties
…tton incl. defaut setting
* Rename BibTex and BibLaTex to BibTeX and BibLaTeX * Rename Formatter list of BibTeX and BibLaTeX
"Recommended for %0" button is now disabled, if save actions are not enabled
* Testcase ensureNoDuplicates in JabRef_vi.properties
@@ -193,8 +217,7 @@ private JPanel getSelectorPanel() { | |||
"pref, 2dlu, pref:grow, 2dlu")); | |||
|
|||
List<String> fieldNames = InternalBibtexFields.getAllPublicFieldNames(); | |||
fieldNames.add(BibEntry.KEY_FIELD); | |||
fieldNames.add("all"); | |||
fieldNames.addAll(Arrays.asList(BibEntry.KEY_FIELD, FieldName.ABSTRACT_ALL_FIELD, FieldName.ABSTRACT_ALL_TEXT_FIELDS_FIELD)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and extract it to a new method InternalBibtexFields.getAllPublicAndInteralFieldNames()
.
Btw, I would suggest to rename the ABSTRACT
prefix to INTERNAL
(when reading "FieldName.abstract_Something" I always think of the "abstract" field)
defaultFormatters.add(new FieldFormatterCleanup(FieldName.MONTH, new NormalizeMonthFormatter())); | ||
defaultFormatters.add(new FieldFormatterCleanup(FieldName.BOOKTITLE, new OrdinalsToSuperscriptFormatter())); | ||
defaultFormatters.add(new FieldFormatterCleanup(FieldName.ABSTRACT_ALL_TEXT_FIELDS_FIELD, new OrdinalsToSuperscriptFormatter())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would completely remove the OrdinalsToSuperscript from the defaults.
DEFAULT_SAVE_ACTIONS = new FieldFormatterCleanups(false, defaultFormatters); | ||
|
||
List<FieldFormatterCleanup> recommendedBibTeXFormatters = new ArrayList<>(defaultFormatters); | ||
recommendedBibTeXFormatters.add(new FieldFormatterCleanup(FieldName.TITLE, new HtmlToLatexFormatter())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to not run the HTML/Unicode -> Latex converter on all [text] fields? Similar question for the Latex/HTML -> Unicode for biblatex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koppor what do you think about that ??? 😄
DEFAULT_SAVE_ACTIONS = new FieldFormatterCleanups(false, defaultFormatters); | ||
|
||
List<FieldFormatterCleanup> recommendedBibTeXFormatters = new ArrayList<>(defaultFormatters); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it more clear that default formatters are also added:
recommendedbibTexFormatters = new ArrayList();
recommendendBibTexFormatters.addAll(defaulttFormatters);
(same for biblatex)
private List<FieldChange> cleanupAllTextFields(BibEntry entry) { | ||
List<FieldChange> fieldChanges = new ArrayList<>(); | ||
Set<String> fields = entry.getFieldNames(); | ||
fields.removeAll(Arrays.asList(FieldName.DOI, FieldName.FILE, FieldName.URL, FieldName.URI, FieldName.ISBN, FieldName.ISSN, FieldName.MONTH, FieldName.DATE, FieldName.YEAR)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return these fields from a new method FieldNames.getNotTextFieldNames()
@@ -1352,7 +1325,14 @@ public void setDefaultEncoding(Charset encoding) { | |||
put(DEFAULT_ENCODING, encoding.name()); | |||
} | |||
|
|||
private static void insertCleanupPreset(Map<String, Object> storage, CleanupPreset preset) { | |||
private static void insertCleanupPreset(Map<String, Object> storage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename method to insertDefaultCleanupPreset
It is part of the old dialog, isn't it? It doesnt have to be part for the
minimum cleanups, but for the recommendations, it must!
|
Its my favourite. So keep it, please.
|
* create test for fieldformattercleanup * refactoring for default
@@ -46,12 +46,14 @@ | |||
recommendedBibTeXFormatters.add(new FieldFormatterCleanup(FieldName.JOURNAL, new UnicodeToLatexFormatter())); | |||
recommendedBibTeXFormatters.add(new FieldFormatterCleanup(FieldName.AUTHOR, new UnicodeToLatexFormatter())); | |||
recommendedBibTeXFormatters.add(new FieldFormatterCleanup(FieldName.EDITOR, new UnicodeToLatexFormatter())); | |||
defaultFormatters.add(new FieldFormatterCleanup(FieldName.INTERNAL_ALL_TEXT_FIELDS_FIELD, new OrdinalsToSuperscriptFormatter())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recommendedBibTeXFormatters instead of defaultFormatters (same below)
…ultSetting # Conflicts: # CHANGELOG.md
@koppor all thing are doing. Pls take a look for merge 🎅 |
recommendedBibTeXFormatters.add(new FieldFormatterCleanup(FieldName.JOURNAL, new UnicodeToLatexFormatter())); | ||
recommendedBibTeXFormatters.add(new FieldFormatterCleanup(FieldName.AUTHOR, new UnicodeToLatexFormatter())); | ||
recommendedBibTeXFormatters.add(new FieldFormatterCleanup(FieldName.EDITOR, new UnicodeToLatexFormatter())); | ||
defaultFormatters.add(new FieldFormatterCleanup(FieldName.INTERNAL_ALL_TEXT_FIELDS_FIELD, new OrdinalsToSuperscriptFormatter())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its still "defaultFormatters" here instead of "recommendedBibTeXFormatters"....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
@@ -131,12 +133,32 @@ public void contentsChanged(ListDataEvent e) { | |||
resetButton = new JButton(Localization.lang("Reset")); | |||
resetButton.addActionListener(e -> ((CleanupActionsListModel) actionsList.getModel()).reset(defaultFormatters)); | |||
|
|||
boolean isBibTeX = !JabRefGUI.getMainFrame().getCurrentBasePanel().getDatabaseContext().isBiblatexMode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
work with "isBibLatex" instead of negating it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
* rename the recommended list * change "isBibTex" to "isBibLatex"
* Fixing: Sensible default settings for "Enable save actions" and "Cleanup" * Sensible default settings for "Enable save actions" in database properties and "Cleanup entries" in menu are now identical * Insert allTextField fieldName and refactoring the reset entries of database properties * CHANGELOG Fixing * Feedback * rename all_text_fields to all-text-fields * remove ISBN and ISSN, MONTH, DATE, YEAR form all-text-fields * Create Recommend Button for BibTex and BibLatex and refactor reset button incl. defaut setting * Refactoring: * Rename BibTex and BibLaTex to BibTeX and BibLaTeX * Rename Formatter list of BibTeX and BibLaTeX * Refactoring_15102016_2343: "Recommended for %0" button is now disabled, if save actions are not enabled * Fix LocalizationConsistencyTest FAIL: * Testcase ensureNoDuplicates in JabRef_vi.properties * Refactoring: * create test for fieldformattercleanup * refactoring for default * add OrdinalsToSuperscriptFormatter to recommand button * REFACTORING_27102016_0744: * rename the recommended list * change "isBibTex" to "isBibLatex" * Follow to refactor_27102016_1340
Sensible default settings for "Enable save actions" field of database properties dialog and "Run field formatter" field of Cleanup eintries dialog are now identical 🐨
Please take a look for merge. THX 🐰
For further information have a look at:
JabRef#128
😄