Move labelpattern #1626

lenhard · 2016-07-25T13:08:57Z

Minor refactoring and moving of classes from logic into model. The XXXLabelPattern classes really describe how we serialize and store label patterns (part of the structure of meta data). Hence, they belong into model and have already been almost independent of other classes. The util class with helpful functions regarding label patterns can stay in util.

simonharrer · 2016-07-25T13:59:06Z

src/main/java/net/sf/jabref/model/labelpattern/GlobalLabelPattern.java

+    }
+
+    public static void updateDefaultPattern() {
+        defaultLabelPattern = AbstractLabelPattern.split(JabRefPreferences.getInstance().get(JabRefPreferences.DEFAULT_LABEL_PATTERN));


Instead of relying on the preferences within this class, could the preference value just be passed through a parameter? Then this class would be independent of the preferences package.

See #1607. :-) Not sure what is the best merging order...

@simonharrer Good idea
@oscargus Oh no, now I have already rewritten GlobalLabelPattern :-(

However, it seems you did not really touch the class in #1607? From what I have seen and tested with the GUI, you can completely drop the static part of GlobalLabelPattern. It is just initialized in two places (the gui and the preferences) and both of these places have access to the gui, so you can provide the correct parameters right away.

Regarding merging order: I'd prefer mine to be merged first :P Anyway, let Simon decide!

…r be friends

simonharrer · 2016-07-25T19:18:29Z

LGTM

@oscargus how does this affect #1607 ?

oscargus · 2016-07-25T20:40:41Z

No idea. Probably a bit of resolving independent of who merges first. The feeling is that it is easier to merge the PR without moving of files first, but I'm not sure if that is true or if I just want less work. ;-)

oscargus · 2016-07-25T20:45:44Z

By the way, there is a preference setting "Enforce legal characters in BibTeX keys". Is that really needed?

The difference is the characters in "{}(),\\\"#~^'" but not in "{}(),\\\"" so "#~^'". I'm wondering if we upset many people by always enforcing legal characters?

lenhard · 2016-07-26T07:26:59Z

@oscargus: My changes were quite small and would be relatively easy to redo, so it's ok for me if you go first.

Regarding the BibTex keys: We have already upset at least one person in #1272. We talked about this issue on the devcall right now and arrived at the following decision:

The preference setting "Enforce legal characters in BibTeX keys" can be removed, hence we will only enforce "{}(),\\\"#~^'".
Key validation should be based on the mode (BibTeX, BibLaTeX). Especially BibLaTeX is more strict and there should be an additional validation that checks for the symbols that are not allowed in BibLaTeX

oscargus · 2016-07-26T10:54:35Z

OK! If anyone could have a look at the PR I'll merge it.

So basically, the (in the PR introduced) boolean argument to checkLegalKey should be replaced with isBibLatex? Is it clear what the set of symbols are for the two cases?

tobiasdiez · 2016-07-26T11:17:51Z

And we considered moving the check to the integrity dialog.

# Conflicts: # src/main/java/net/sf/jabref/gui/preftabs/LabelPatternPrefTab.java # src/main/java/net/sf/jabref/logic/labelpattern/LabelPatternUtil.java # src/main/java/net/sf/jabref/preferences/JabRefPreferences.java

lenhard · 2016-07-28T10:56:40Z

Should be good to go.

Just as a side note: What are the "default" buttons in Preferences-> BibTeX key generator supposed to do? For me, only the button for the default pattern works and actually resets something. The other default buttons basically just delete what I have entered into the corresponding text field. This behavior is independent of this PR.

oscargus · 2016-07-28T12:27:24Z

I think the default is that every type has the same key pattern, so it makes sense that the pattern is cleared when clicking Default.

oscargus · 2016-07-28T12:31:44Z

LGTM! 👍

matthiasgeiger · 2016-07-28T12:38:58Z

I'm not sure whether we discussed it in this context at the last devcall, but moving those classes will lead to another node in the registry for storing those preferences - if the storing mechanism is not adjusted.

lenhard · 2016-07-28T14:11:16Z

@matthiasgeiger: Are you sure? Didn't that already happen through #1635 by introducing the new LabelPatternPreferences? As far as I understand, the old label preferences get lost through this change, but will not so easily get lost in the future, since they are now normal preferences.

matthiasgeiger · 2016-07-28T14:20:28Z

Just tested it: v3.5 and current master reads and writes from/to HKEY_CURRENT_USER\SOFTWARE\JavaSoft\Prefs\net\sf\jabref\logic\labelpattern - your branch reads and writes from/to HKEY_CURRENT_USER\SOFTWARE\JavaSoft\Prefs\net\sf\jabref\model\labelpattern

matthiasgeiger · 2016-07-28T14:23:54Z

Caused by: https://github.com/JabRef/jabref/blob/master/src/main/java/net/sf/jabref/preferences/JabRefPreferences.java#L1151 and https://github.com/JabRef/jabref/blob/master/src/main/java/net/sf/jabref/preferences/JabRefPreferences.java#L1174
Maybe we should discuss again how to implement this better and provide migrations this time ;-)

lenhard · 2016-07-28T14:28:21Z

Migrations are overrated :-)

We should at least change that to

Preferences pre = Preferences.userNodeForPackage(JabRefMain.class);

to avoid future issues.

Although not part of this PR, let us keep this open and discuss at the next devcall on how to proceed.

Refs: #1257

matthiasgeiger · 2016-07-28T14:31:28Z

Another topic for the devcall: Can we rename this "LabelPattern..." stuff to "BibtexKeyPattern..." ? - I never understood why this is called "label pattern" 😉

lenhard · 2016-08-09T12:06:36Z

TODO in new PR after this one is merged:

Integrate label pattern preferences tree into global preferences tree
Rename label pattern to bibtex key pattern.
Possible migration of old key patterns

Conflicts: src/main/java/net/sf/jabref/JabRefMain.java

Conflicts: src/main/java/net/sf/jabref/MetaData.java

* 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

* Move classes describing the structure of label patterns into model * Remove unnecessar import * Fix checkstyle errors * Fix checkstyle errors in tests * Rewrite GlobalLabelPattern and remove static parts * Fix checkstyle error: The intellij checkstyle plugin and me will never be friends * Add getter for default label pattern * Remove unused imports

lenhard added 4 commits July 25, 2016 14:33

Move classes describing the structure of label patterns into model

0f659e5

Remove unnecessar import

d05b5d6

Fix checkstyle errors

ca8a261

Fix checkstyle errors in tests

25805ac

lenhard added component: cleanup-ops status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers architecture labels Jul 25, 2016

simonharrer reviewed Jul 25, 2016
View reviewed changes

lenhard added 2 commits July 25, 2016 16:52

Rewrite GlobalLabelPattern and remove static parts

939ad07

Fix checkstyle error: The intellij checkstyle plugin and me will neve…

a5016d2

…r be friends

Merge branch 'master' into move-labelpattern

7012d5e

# Conflicts: # src/main/java/net/sf/jabref/gui/preftabs/LabelPatternPrefTab.java # src/main/java/net/sf/jabref/logic/labelpattern/LabelPatternUtil.java # src/main/java/net/sf/jabref/preferences/JabRefPreferences.java

Add getter for default label pattern

2c5f1e8

lenhard added the status: devcall label Jul 28, 2016

oscargus mentioned this pull request Jul 28, 2016

Desired Architecture #1579

Closed

tobiasdiez added the dev: code-quality Issues related to code or architecture decisions label Aug 4, 2016

tobiasdiez removed the component: cleanup-ops label Aug 4, 2016

lenhard added 3 commits August 9, 2016 14:12

Merge branch 'master' into move-labelpattern

302417c

Conflicts: src/main/java/net/sf/jabref/JabRefMain.java

Remove unused imports

a6731a6

Merge branch 'master' into move-labelpattern

bff43a7

Conflicts: src/main/java/net/sf/jabref/MetaData.java

lenhard merged commit c004550 into master Aug 9, 2016

lenhard deleted the move-labelpattern branch August 9, 2016 12:39

matthiasgeiger mentioned this pull request Aug 9, 2016

Rename labelpattern to bibtexkeypattern and fix pref issue #1704

Merged

8 tasks

matthiasgeiger removed the status: devcall label Sep 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move labelpattern #1626

Move labelpattern #1626

lenhard commented Jul 25, 2016

simonharrer Jul 25, 2016

oscargus Jul 25, 2016

lenhard Jul 25, 2016 •

edited

Loading

simonharrer commented Jul 25, 2016

oscargus commented Jul 25, 2016

oscargus commented Jul 25, 2016 •

edited

Loading

lenhard commented Jul 26, 2016

oscargus commented Jul 26, 2016

tobiasdiez commented Jul 26, 2016

lenhard commented Jul 28, 2016

oscargus commented Jul 28, 2016

oscargus commented Jul 28, 2016

matthiasgeiger commented Jul 28, 2016

lenhard commented Jul 28, 2016

matthiasgeiger commented Jul 28, 2016

matthiasgeiger commented Jul 28, 2016 •

edited

Loading

lenhard commented Jul 28, 2016 •

edited

Loading

matthiasgeiger commented Jul 28, 2016

lenhard commented Aug 9, 2016

Move labelpattern #1626

Move labelpattern #1626

Conversation

lenhard commented Jul 25, 2016

simonharrer Jul 25, 2016

Choose a reason for hiding this comment

oscargus Jul 25, 2016

Choose a reason for hiding this comment

lenhard Jul 25, 2016 • edited Loading

Choose a reason for hiding this comment

simonharrer commented Jul 25, 2016

oscargus commented Jul 25, 2016

oscargus commented Jul 25, 2016 • edited Loading

lenhard commented Jul 26, 2016

oscargus commented Jul 26, 2016

tobiasdiez commented Jul 26, 2016

lenhard commented Jul 28, 2016

oscargus commented Jul 28, 2016

oscargus commented Jul 28, 2016

matthiasgeiger commented Jul 28, 2016

lenhard commented Jul 28, 2016

matthiasgeiger commented Jul 28, 2016

matthiasgeiger commented Jul 28, 2016 • edited Loading

lenhard commented Jul 28, 2016 • edited Loading

matthiasgeiger commented Jul 28, 2016

lenhard commented Aug 9, 2016

lenhard Jul 25, 2016 •

edited

Loading

oscargus commented Jul 25, 2016 •

edited

Loading

matthiasgeiger commented Jul 28, 2016 •

edited

Loading

lenhard commented Jul 28, 2016 •

edited

Loading