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

Create sensible default settings for "Enable save actions" and "Cleanup" dialogs #2051

Merged
merged 14 commits into from
Oct 27, 2016
Merged

Create sensible default settings for "Enable save actions" and "Cleanup" dialogs #2051

merged 14 commits into from
Oct 27, 2016

Conversation

motokito
Copy link
Contributor

@motokito motokito commented Sep 24, 2016

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 🐰

  • Change the default setting - by click on "Reset" button:

2016-10-15_23h31_31

2016-10-15_23h31_45

  • Change the default setting - by click on "Recommend for BibTeX" button:

2016-10-15_23h35_28

2016-10-15_23h36_12

  • Change the default setting - by click on "Recommend for BibLaTeX" button:

2016-10-15_23h38_27

2016-10-15_23h38_40

For further information have a look at:
koppor#128
😄

  • Change in CHANGELOG.md described
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Internal QS

@@ -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"
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done it 🐨

@koppor
Copy link
Member

koppor commented Sep 29, 2016

Sorry, but not all settings from the original dialog have been included:

grabbed_20160708-205241

  • I miss the "convert to real superscripts". I think, this is "Ordinals to superscript"
  • Please go through line by line and state which new converter you chose. Example:
    • Run HTML converter on title -> title: HTML to LaTeX
  • I would also love to have a toggle button for "Unicode conversion", which adds/removes the respective formatters being equivalent to Run Unicode converter on title, author(s), and abstract.
  • In case you are in context, could you check, why "Reformat ISSN" is there? Isn't that just another formatter?

Refs https://github.com/JabRef/help.jabref.org/issues/8

@tobiasdiez
Copy link
Member

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.

@motokito
Copy link
Contributor Author

@koppor oliver what do you think about tobiasdiez idea ??? 😏

@motokito
Copy link
Contributor Author

motokito commented Sep 29, 2016

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".

  • Also i have not understand, what do you meant in this task:

Please go through line by line and state which new converter you chose.

Thy for explaining 😄

@koppor
Copy link
Member

koppor commented Sep 29, 2016 via email

@koppor
Copy link
Member

koppor commented Sep 29, 2016 via email

@motokito
Copy link
Contributor Author

@koppor Event without the checkbox "convert to real superscripts" you can add the entryfield manually to the fieldformatter with the converter you chose eg.

  • Run Field Formatter -> choose entryfield -> booktitle: -> choose converter for superscripts -> for example HTML to Latex -> add

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.

@koppor
Copy link
Member

koppor commented Sep 30, 2016

I know how to use the dialog. I want sensible defaults - see koppor#128

Copy link
Contributor

@boceckts boceckts left a 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line.

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 😄

@boceckts boceckts added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 9, 2016
@boceckts boceckts changed the title [WIP] Create sensible default settings for "Enable save actions" and "Cleanup" dialogs Create sensible default settings for "Enable save actions" and "Cleanup" dialogs Oct 9, 2016
@motokito
Copy link
Contributor Author

motokito commented Oct 10, 2016

The convert checkbox of old cleanup entries dialog is defined now in field formatter like following:

  • Run HTML converter on title -> title: HTML to LateX
  • Run Unicode converter on title, author(s), and abstract -> title: , author: or abstract: Unicode to LateX or LateX to Unicode
  • Run filter on title keeping the case of selected words -> title: , journal: or journal title: Protect terms
  • Remove unneccessary $, {, and } and move adjacent numbers into equations -> all_text_fields: LaTeX cleanup
  • Add brackets and replace separators with their non-breaking version for units -> all_text_fields: Units to LaTeX
  • Convert 1st, 2nd, ... to real superscripts -> all_text_fields: Ordinals to Latex superscript
  • Format content of month field to #mon# -> month: Normalize month
  • Ensure that page ranges are of the form num1--num2 -> pages: Normalize page numbers
  • Format date field in the form yyyy-mm or yyyy-mm-dd -> date: Normalize date

All are done, please take a look for merge 🎅

@koppor
Copy link
Member

koppor commented Oct 10, 2016

Can the unicode converter (line 2) also be run on all_text_fields?

Run filter on title keeping the case --> This is "protect terms". Please enable it on title, journal, and journal title.

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 10, 2016
@@ -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";
Copy link
Member

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 ("_").

Copy link
Contributor Author

@motokito motokito Oct 12, 2016

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));
Copy link
Member

@koppor koppor Oct 10, 2016

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,

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 😄

@oscargus
Copy link
Contributor

Some comments:

  • Most bst:s doesn't change the case of Journal titles, so no need to protect them (please provide any counter examples if I am wrong).
  • Adding braces around a complete field is in most cases the wrong way to do this as it will basically disable the bst handling of the field. (Note to @koppor )
  • Using an all_text_fields meta field would be quite nice. I'd suggest HTML to LaTeX on those and maybe Unicode to LaTeX (although this is indeed really BibTeX/Biblatex dependent, therefore my hesitation, although we may have "recommended for BibTeX" and "recommended for Biblatex" versions...).

@tobiasdiez
Copy link
Member

tobiasdiez commented Oct 12, 2016

I'm really no big fan of the proposed default values:

  • ProtectTermsFormatter: not needed in BibLaTex (and depends on the bibtex style)
  • UnitsToLatexFormatter: I use the si-units package and thus this conversion is counterproductive.
  • LatexCleanupFormatter: Is not good enough to be default in my opinion, but if the majority wants it then it would be fine with me
  • HtmlToLatexFormatter: for BibLaTeX you want Html -> Unicode instead
  • OrdinalsToSuperscriptFormatter: as a user of the recommended nth-package, this conversion is again suboptimal

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.
But any way...its just default values.

@motokito
Copy link
Contributor Author

motokito commented Oct 12, 2016

@koppor: all-text-fields will be function with unicode converter
i have done all. Please take a look for merge. THX 🍪

@motokito
Copy link
Contributor Author

@koppor: i have implemented recommend button for BibLatex /BibTex and refactoring the default setting
please take a look for merge BIG THX 😄

@boceckts boceckts added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Oct 13, 2016
String mode;

if (isBibTex) {
mode = "BibTex";
Copy link
Contributor

Choose a reason for hiding this comment

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

Must be "BibTeX"

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 😄

if (isBibTex) {
mode = "BibTex";
} else {
mode = "BibLaTex";
Copy link
Contributor

Choose a reason for hiding this comment

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

Must be "BibLaTeX"

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 😄

mode = "BibLaTex";
}

recommendButton = new JButton(Localization.lang("Recommend for %0", mode));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be "Recommended"

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 😄

DEFAULT_SAVE_ACTIONS = new FieldFormatterCleanups(false, defaultFormatters);

List<FieldFormatterCleanup> recommendBibTexFormatters = new ArrayList<>(defaultFormatters);
Copy link
Contributor

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.

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 😄

recommendBibTexFormatters.add(new FieldFormatterCleanup(FieldName.EDITOR, new UnicodeToLatexFormatter()));
RECOMMEND_BIBTEX_ACTIONS = new FieldFormatterCleanups(false, recommendBibTexFormatters);

List<FieldFormatterCleanup> recommendBibLatexFormatters = new ArrayList<>(defaultFormatters);
Copy link
Contributor

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

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 😄

@boceckts boceckts added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 13, 2016
* rename all_text_fields to all-text-fields
* remove ISBN and ISSN, MONTH, DATE, YEAR form all-text-fields
* 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));
Copy link
Member

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()));
Copy link
Member

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()));
Copy link
Member

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.

Copy link
Contributor Author

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);
Copy link
Member

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));
Copy link
Member

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) {
Copy link
Member

Choose a reason for hiding this comment

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

rename method to insertDefaultCleanupPreset

@koppor koppor added status: waiting-for-feedback The submitter or other users need to provide more information about the issue and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Oct 24, 2016
@koppor
Copy link
Member

koppor commented Oct 25, 2016 via email

@koppor
Copy link
Member

koppor commented Oct 25, 2016 via email

* 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()));
Copy link
Member

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)

@motokito
Copy link
Contributor Author

@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()));
Copy link
Member

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"....

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 😄

@@ -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();
Copy link
Member

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.

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 😄

* rename the recommended list
* change "isBibTex" to "isBibLatex"
@koppor
Copy link
Member

koppor commented Oct 27, 2016

Good enough for me. Follow ups:

  • remove empty lines before closing { and if-statements
  • unifiy mode string generation with panel

Reason for second one: I assume that

 if (isBibLaTeX) {
            mode = "BibLaTeX";

also appears somewhere else in the code.

grabbed_20161027-115448

@koppor koppor merged commit e5cfb05 into JabRef:master Oct 27, 2016
tobiasdiez pushed a commit that referenced this pull request Oct 27, 2016
* 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
@koppor koppor removed the status: waiting-for-feedback The submitter or other users need to provide more information about the issue label Jan 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants