Skip to content

Got rid of unused preferences#1672

Merged
oscargus merged 2 commits into
JabRef:masterfrom
oscargus:removevaluedelimiters
Aug 9, 2016
Merged

Got rid of unused preferences#1672
oscargus merged 2 commits into
JabRef:masterfrom
oscargus:removevaluedelimiters

Conversation

@oscargus

@oscargus oscargus commented Aug 3, 2016

Copy link
Copy Markdown
Contributor

And marked some which can not be changed from within JabRef.

  • Change in CHANGELOG.md described
  • Manually tested changed features in running JabRef

@oscargus oscargus added component: cleanup-ops status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Aug 3, 2016
@tobiasdiez tobiasdiez added dev: code-quality Issues related to code or architecture decisions and removed component: cleanup-ops labels Aug 4, 2016
@oscargus oscargus force-pushed the removevaluedelimiters branch from 00d53a9 to 99b775c Compare August 4, 2016 16:46
@oscargus

oscargus commented Aug 5, 2016

Copy link
Copy Markdown
Contributor Author

Should we try to collect the preference strings we remove and provide a function to erase those from the preferences? The string can then be kept for a few years and finally retired for good.

@oscargus oscargus force-pushed the removevaluedelimiters branch from 99b775c to dd926db Compare August 8, 2016 12:12
@lenhard

lenhard commented Aug 9, 2016

Copy link
Copy Markdown
Member

How did you find out which preferences are unused? Did the IDE mark them as unused?

There is no need to manually cleanup the registry from old preferences. Hardly any software actually does that and we are just talking about a few key value pairs.

stefan-kolb
stefan-kolb reviewed Aug 9, 2016
View reviewed changes
Comment thread src/main/java/net/sf/jabref/JabRefGUI.java

private void openWindow() {
// Perform checks and changes for users with a preference set from an older JabRef version.
PreferencesMigrations.replaceAbstractField();

@stefan-kolb stefan-kolb Aug 9, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did you remove this? Because it's too old?

@oscargus oscargus Aug 9, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because the code checks if GENERAL_FIELDS contains abstract and in that case removes if from GENERAL_FIELDS.

The thing is that the only place GENERAL_FIELDS is used in the current master is for setting a default value...

// The general fields stuff is made obsolete by the CUSTOM_TAB_... entries.
        defaults.put(GENERAL_FIELDS, "crossref;keywords;file;doi;url;urldate;"
                + "pdf;comment;owner");

@oscargus

oscargus commented Aug 9, 2016

Copy link
Copy Markdown
Contributor Author

I searched (almost) every string constant in JabRefPreferences and checked how they were used (with some thinking as clearly the count is not always a good indication). For most of the removed ones I also search for the actual string, just to confirm that someone had hardcoded the string instead. All four "features" are also quite straightforward to test and see if they are working and if the preference makes any difference (hint: no). Typically these were only used in the preferences and the preference dialog.

The ones marked as not changable are simply used in (typically) two places, once to set the default value and then they are used in one line of code (and that is not the preference dialog).

@lenhard

lenhard commented Aug 9, 2016

Copy link
Copy Markdown
Member

Ok, since this is somewhat hard to review by looking at the code, we'll trust you here. Please merge and rebase!

@oscargus

oscargus commented Aug 9, 2016

Copy link
Copy Markdown
Contributor Author

I can summarize:

  • JabRefPreferences.GROUPS_VISIBLE_ROWS not needed since group pane now always maximizes the height.
  • JabRefPreferences.GENERAL_FIELDS not used (so the migration is not needed)
  • JabRefPreferences.DISABLE_ON_MULTIPLE_SELECTION never used in the code (so entry editor is never disabled on multiple selection)
  • JabRefPreferences.VALUE_DELIMITERS (2) JabRef always use {} for storing fields (the comboBox was not in the layout, but in the code)
  • JabRefPreferences.DEFAULT_AUTO_SORT No default auto sort any more (as this preference is not used except in the preference dialog)
  • JabRefPreferences.CTRL_CLICK Ctrl + left click will never ever bring up the right-click menu (as this preference is not used except in the preference dialog)
  • JabRefPreferences.DIALOG_WARNING_FOR_DUPLICATE_KEY and JabRefPreferences.DIALOG_WARNING_FOR_EMPTY_KEY there will never be any automatic warnings when entering an empty or duplicate key

@stefan-kolb

stefan-kolb commented Aug 9, 2016

Copy link
Copy Markdown
Member

Just go ahead and merge this 😄 👍

@oscargus oscargus force-pushed the removevaluedelimiters branch from dd926db to 75b66de Compare August 9, 2016 13:31
@oscargus oscargus merged commit 12f5e29 into JabRef:master Aug 9, 2016
@oscargus oscargus deleted the removevaluedelimiters branch August 9, 2016 13:35
Siedlerchr added a commit to Siedlerchr/jabref that referenced this pull request Aug 9, 2016
@Siedlerchr
Merge branch 'master' into fileChooserRefactor
56ff174 
* master: (22 commits)
  Do not cite entries without a key in OpenOffice/LibreOffice (JabRef#1682) (JabRef#1683)
  Got rid of unused preferences (JabRef#1672)
  Move labelpattern (JabRef#1626)
  Implementation of shared database support (full system). (JabRef#1451)
  Removed bst from architecture tests JabRef#1699
  Update PULL_REQUEST_TEMPLATE.md
  French localization: Menu: Translation of an empty string
  French localization: Jabref_fr: empty strings translated
  Updated string similarity version (JabRef#1698)
  Minify description of release process
  Announce switch from GPL to MIT in CONTRIBUTING.md and README.md
  Some cleanups to initialize empty MetaData at fewer positions (JabRef#1696)
  Automatic group names are converted from LaTeX to Unicode (JabRef#1684)
  Update ISSUE_TEMPLATE.md (JabRef#1686)
  Removed (false) NPE issue reported by FindBugs
  More Swedish translations (JabRef#1691)
  Updated wiremock version (JabRef#1690)
  Some minor code cleanups (JabRef#1689)
  Removed dependencies of Globals.prefs in some tests (JabRef#1688)
  Lookup BibEntry from ISBN and merge information (JabRef#1621)
  ...

# Conflicts:
#	src/main/java/net/sf/jabref/gui/BasePanel.java
#	src/main/java/net/sf/jabref/importer/ImportMenuItem.java
#	src/main/resources/l10n/JabRef_da.properties
#	src/main/resources/l10n/JabRef_de.properties
#	src/main/resources/l10n/JabRef_en.properties
#	src/main/resources/l10n/JabRef_es.properties
#	src/main/resources/l10n/JabRef_fa.properties
#	src/main/resources/l10n/JabRef_fr.properties
#	src/main/resources/l10n/JabRef_in.properties
#	src/main/resources/l10n/JabRef_it.properties
#	src/main/resources/l10n/JabRef_ja.properties
#	src/main/resources/l10n/JabRef_nl.properties
#	src/main/resources/l10n/JabRef_no.properties
#	src/main/resources/l10n/JabRef_pt_BR.properties
#	src/main/resources/l10n/JabRef_ru.properties
#	src/main/resources/l10n/JabRef_sv.properties
#	src/main/resources/l10n/JabRef_tr.properties
#	src/main/resources/l10n/JabRef_vi.properties
#	src/main/resources/l10n/JabRef_zh.properties
@oscargus oscargus mentioned this pull request Aug 11, 2016
Closed
ayanai1 pushed a commit to ayanai1/jabref that referenced this pull request Sep 5, 2016
@oscargus @ayanai1
Got rid of unused preferences (JabRef#1672)
9f1724c 
* Fully removed support for changing field start/end character

* Removed unused preferences
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

dev: code-quality Issues related to code or architecture decisions status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@oscargus @lenhard @stefan-kolb @tobiasdiez