Improve entrytype customization #2331

matthiasgeiger · 2016-12-02T11:16:31Z

Goals:

Entry customization based on BibDatabaseMode
- working for custom entry types defined in bib DB
- storage is working for new customized entries (instead of creating three/four prefs Strings for each customized entry type in the form ("custom_type_1_name", "custom_type_1_REQ", ...) two distinct pref nodes (one for BibTeX one for BibLaTeX) for customized entry types are used which simply use the custom type name as key and the string serialization as value (--> parsing can be reused))
- loading is working for newly created customized entries
- write prefs migration (is performed on first startup of new version, existing prefs will be put as new custom types for defaultBibDatabaseMode, old prefs are not deleted)
enable testing with cleanup after test execution
- bibtex
- biblatex
various bug fixes and improvements, e.g., Customizing the entry types freezes Jabref #2318 custom entry types bug #772 Refactor entry type/custom entry type logic #366 Save custom entry types in bib file #365
- refactor/rewrite parts of EntryCustomizationDialog to improve performance (refs Customizing the entry types freezes Jabref #2318)
- fixes Reset preferences & Import preferences not working for custom entry types. #2261 (and also clearing of bibtexKeyPatterns using "reset preferences")
- custom entry types bug #772: Deleting custom type, restoring customized default type from exported prefs, no multiple import of customized entry types
- Refactor entry type/custom entry type logic #366: Check "Resetting overridden default entry types does not work, sometimes even crashes" - not reproducible in reworked state (ask @stefan-kolb whether still an issue) - related to Customizing the entry types freezes Jabref #2318? --> caused by running through all entries in all open DBs
- If a customized type is declared in a file it will be tried to import the declarations - if a customType already exists, a warning should be shown that it will be overwritten
  - before this PR (no import of already existing customizations):
  - now (more control):
- a customized standard type should not appear in the "custom" section of the "New entry" dialog, and not in the "custom entries" section of the "change entry type" menu
CHANGELOG

… entries - fixes #2261

…ialogs

matthiasgeiger · 2016-12-11T21:41:47Z

Okay... I think the current state is now a good improvement of the old status-quo.
At the moment the prefs migration and a dialog showing a warning if customized entries would be overridden is still missing.

Apart from that I would appreciate some feedback ;-)

lenhard

I just looked through the code, but did not test this in a running JabRef. There are a number of things that can be improved quality/architecture-wise, but none of these is a blocker. So please fix what you agree with.

lenhard · 2016-12-12T13:05:01Z

src/main/java/net/sf/jabref/gui/importer/actions/CheckForNewEntryTypesAction.java

-            sb.append(type).append(", ");
+    public void performAction(BasePanel panel, ParserResult parserResult) {
+
+        BibDatabaseMode mode = parserResult.getMetaData().getMode().orElse(Globals.prefs.getDefaultBibDatabaseMode());


To avoid writing that method chain multiple times in this class, maybe extract a helper method?

lenhard · 2016-12-12T13:05:52Z

src/main/java/net/sf/jabref/gui/importer/actions/CheckForNewEntryTypesAction.java

+        List<EntryType> newTypes = new ArrayList<>();
+        List<EntryType> differentCustomizations = new ArrayList<>();
+
+        for(EntryType customType : allCustomizedEntryTypes) {


please adhere to the formatting guidelines: for ( and below if (

lenhard · 2016-12-12T13:07:04Z

src/main/java/net/sf/jabref/gui/importer/actions/CheckForNewEntryTypesAction.java

+        JPanel checkboxPanel = new JPanel();
+        checkboxPanel.setLayout(new BoxLayout(checkboxPanel, BoxLayout.PAGE_AXIS));
+
+        JLabel customFoundLabel = new JLabel(Localization.lang("Custom entry types found in file")+".");


Also formatting problems here. I should be sufficient to auto-format the file.

lenhard · 2016-12-12T13:08:13Z

src/main/java/net/sf/jabref/gui/menus/ChangeEntryTypeMenu.java

@@ -57,19 +57,31 @@ private void populateChangeEntryTypeMenu(JMenu menu, BasePanel panel) {
            for (EntryType type : EntryTypes.getAllValues(BibDatabaseMode.BIBLATEX)) {
                menu.add(new ChangeTypeAction(type, panel));
            }
+
+            List<EntryType> customTypes = EntryTypes.getAllCustomTypes(BibDatabaseMode.BIBLATEX);


Would it be possible to extract the following code in a private method? It is duplicated below and the only difference is the mode.

I would prefer to leave it as it, as the helper method would require three params (BibDatabaseMode, Menu and BasePanel) for just a simple get, if, create...

lenhard · 2016-12-12T13:08:41Z

src/main/java/net/sf/jabref/gui/EntryTypeDialog.java

+        if (frame.getCurrentBasePanel().getBibDatabaseContext().isBiblatexMode()) {
+            panel.add(createEntryGroupPanel("BibLateX", BibLatexEntryTypes.ALL));
+
+            List<EntryType> customTypes = EntryTypes.getAllCustomTypes(BibDatabaseMode.BIBLATEX);


Would it be possible to extract the following code in a private method? It is duplicated below and the only difference is the mode.

Same as above - helper method would be overkill, imho

lenhard · 2016-12-12T13:11:02Z

src/main/java/net/sf/jabref/logic/CustomEntryTypesManager.java

 import net.sf.jabref.preferences.JabRefPreferences;

 public class CustomEntryTypesManager {

-    public static final List<EntryType> ALL = new ArrayList<>();
    /**
     * Load all custom entry types from preferences. This method is
     * called from JabRef when the program starts.
     */
    public static void loadCustomEntryTypes(JabRefPreferences prefs) {


Could you change this class to be independent of JabRefPreferences? This would get us one step closer to our desired architecture. For the load method, this is easy (just put in CustomEntryType instead of the preferences). For the saving it is a little more tricky and you will need to push code into the calling UI-class.

Mhmmm... This is basically just a UI-Helper class. loadCustomEntryTypes is called from the JabRefMain and the CLI Argument processer.

Storage is also used in the UI only...

So would it be possible and sensible to move it as is into the gui package?

Not entirely, unfortunately. See: https://github.com/JabRef/jabref/wiki/High-Level-Documentation
The CLI is not supposed to call the gui, so anything accessed from GUI and CLI should be in logic, but it should be independent of the preferences. You could move the save and leave the load for instance.

No we just need a new package net.sf.ui which could be called by gui and cli!

... just kidding 😉 I'll think about it...

lenhard · 2016-12-12T13:11:56Z

src/main/java/net/sf/jabref/model/EntryTypes.java

+    public static List<EntryType> getAllCustomTypes(BibDatabaseMode mode) {
+        Collection<EntryType> allTypes = getAllValues(mode);
+        if(mode == BibDatabaseMode.BIBTEX) {
+            return allTypes.stream().filter(entryType -> !BibtexEntryTypes.getType(entryType.getName()).isPresent())


Can you extract the almost duplicated lambda into a private helper method?

Sorry, but I cannot come up with an intelligent solution for such a helper method which is not basically the same as I already wrote here 😉

Sure, no problem. This is just a suggestion.

lenhard · 2016-12-12T13:12:22Z

src/main/java/net/sf/jabref/model/EntryTypes.java

+
+    public static List<EntryType> getAllModifiedStandardTypes(BibDatabaseMode mode) {
+        if (mode == BibDatabaseMode.BIBTEX) {
+            return BIBTEX.getAllValues().stream().filter(type -> type instanceof CustomEntryType)


Again, can the lambda be extracted into a helper method?

lenhard · 2016-12-12T13:13:02Z

src/main/java/net/sf/jabref/model/EntryTypes.java

+     */
+    private static boolean checkEqualityOfLists(List<String> fieldList1, List<String> fieldList2) {
+
+        if(fieldList1.size()!=fieldList2.size()) {


Please format this properly.

…ndard equals of List is possible

Conflicts: src/main/java/net/sf/jabref/model/EntryTypes.java src/main/java/net/sf/jabref/preferences/JabRefPreferences.java

matthiasgeiger · 2016-12-13T11:21:01Z

Apart from a CHANGELOG and some more tests this is good to go from my side.

matthiasgeiger · 2016-12-15T10:17:42Z

@mlep @frithnanth It is not yet decided whether we will include this improvement in 3.8 or not. However, there are three more translations which are added in this pull request. Could you provide me translations for these three strings? Then I'll add them manually.
Thanks!

Select_all_customized_types_to_be_stored_in_local_preferences=
Currently_unknown=
Different_Customization,_current_settings_will_be_overwritten=

frithnanth · 2016-12-15T12:07:02Z

On Thu, Dec 15, 2016 at 11:17 AM, Matthias Geiger ***@***.***> wrote: @mlep <https://github.com/mlep> @frithnanth <https://github.com/frithnanth> It is not yet decided whether we will include this improvement in 3.8 or not. However, there are three more translations which are added in this pull request. Could you provide me translations for these three strings? Then I'll add them manually. Thanks! Select_all_customized_types_to_be_stored_in_local_preferences= Select_all_customized_types_to_be_stored_in_local_

preferences=Seleziona_tutti_i_tipi_personalizzati_da_registrare_nelle_preferenze_locali

Currently_unknown= Currently_unknown=Attualmente_sconosciuto

That's the singular mode, the plural is "Attualmente_sconosciuti". But since Italian is profoundly evil, that's OK if referred to a masculine noun, the feminine forms are different, but I'm digressing :-)

Different_Customization,_current_settings_will_be_overwritten= Different_Customization,_current_settings_will_be_

overwritten=Personalizzazione_differente,_i_parametri_correnti_saranno_sovrascritti

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2331 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADyA2AnC2wjebXAMoi4YuB2CzYqky1Bgks5rIRPHgaJpZM4LCcrA> .

-- Fernando Santagata

matthiasgeiger · 2016-12-16T20:30:45Z

Thank you @frithnanth !

frithnanth · 2016-12-16T21:52:04Z

You're welcome!

…

On Fri, Dec 16, 2016 at 9:30 PM, Matthias Geiger ***@***.***> wrote: Thank you @frithnanth <https://github.com/frithnanth> ! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2331 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADyA2EhYM65slWyTwdWTJwH8EDTLmMC4ks5rIvT2gaJpZM4LCcrA> .

-- Fernando Santagata

koppor · 2017-08-18T19:49:10Z

How did we proceed regarding standard types? Is it a v4.0 issue? See koppor#248

matthiasgeiger force-pushed the improve-entrytype-customization branch from aa3c3b2 to 34f5f29 Compare December 4, 2016 21:50

matthiasgeiger added 6 commits December 5, 2016 14:20

entry type customization based on BibdatabaseMode

60db50a

Remove unnecessary help method in BibDatabaseMode

cbbdd67

use database mode of current DB

4548190

add new storage of custom entry types

ab10ab1

load and store databasemode specific lists of custom types

830f59c

rework to use database mode specific custom types

0d45001

matthiasgeiger force-pushed the improve-entrytype-customization branch from 46772ba to 0d45001 Compare December 5, 2016 13:20

matthiasgeiger added 6 commits December 5, 2016 15:12

reset of preferences now also clears bibtexKeyPatterns and customized…

d144954

… entries - fixes #2261

reenable ignored test

9d275bc

rewrite and add additional tests to EntryTypesTestBibtex

379a398

cleanup after tests

7dafccf

add convenience method to get IeeeTranEntryType by name

e189fd7

Filter customizations of standard types in NewEntry/ChangeEntryType d…

cd13456

…ialogs

matthiasgeiger added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 11, 2016

Improve customEntryType import dialog

441964d

This was referenced Dec 12, 2016

Refactor entry type/custom entry type logic #366

Closed

Save custom entry types in bib file #365

Closed

update localization

1cc3493

lenhard approved these changes Dec 12, 2016

View reviewed changes

matthiasgeiger added 9 commits December 12, 2016 21:11

minor cleanup and rename

6d7afc1

do not cycle through all entries but only through changed ones

4c6aa25

improve naming

5aae2f9

different order of fields should indicate different types - using sta…

92c5712

…ndard equals of List is possible

incorporate feedback

4ba2c1f

fix codacy

283401f

migrate customEntryType preferences

9678cba

Merge branch 'master' into improve-entrytype-customization

fbe70d7

Conflicts: src/main/java/net/sf/jabref/model/EntryTypes.java src/main/java/net/sf/jabref/preferences/JabRefPreferences.java

fix localization duplicates

999e493

matthiasgeiger mentioned this pull request Dec 13, 2016

Customizing the entry types freezes Jabref #2318

Closed

matthiasgeiger changed the title ~~[WIP] Improve entrytype customization~~ Improve entrytype customization Dec 13, 2016

matthiasgeiger added 4 commits December 13, 2016 14:01

test entrytypes parametrized

696bcfc

add CHANGELOG for custom type changes/fixes

8584a4d

increase coverage

e247a5d

refactor to adhere to architectural constraints

5e256f5

koppor merged commit 5e256f5 into master Dec 16, 2016

koppor deleted the improve-entrytype-customization branch December 16, 2016 15:52

lenhard mentioned this pull request Dec 16, 2016

custom entry types bug #772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve entrytype customization #2331

Improve entrytype customization #2331

matthiasgeiger commented Dec 2, 2016 •

edited

Loading

matthiasgeiger commented Dec 11, 2016

lenhard left a comment

lenhard Dec 12, 2016

lenhard Dec 12, 2016

lenhard Dec 12, 2016

lenhard Dec 12, 2016

matthiasgeiger Dec 12, 2016

lenhard Dec 12, 2016

matthiasgeiger Dec 12, 2016

lenhard Dec 12, 2016

matthiasgeiger Dec 12, 2016

lenhard Dec 13, 2016

matthiasgeiger Dec 13, 2016

lenhard Dec 12, 2016

matthiasgeiger Dec 12, 2016

lenhard Dec 13, 2016

lenhard Dec 12, 2016

lenhard Dec 12, 2016

matthiasgeiger commented Dec 13, 2016

matthiasgeiger commented Dec 15, 2016

frithnanth commented Dec 15, 2016 via email

matthiasgeiger commented Dec 16, 2016

frithnanth commented Dec 16, 2016 via email

koppor commented Aug 18, 2017

Improve entrytype customization #2331

Improve entrytype customization #2331

Conversation

matthiasgeiger commented Dec 2, 2016 • edited Loading

matthiasgeiger commented Dec 11, 2016

lenhard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasgeiger commented Dec 13, 2016

matthiasgeiger commented Dec 15, 2016

frithnanth commented Dec 15, 2016 via email

matthiasgeiger commented Dec 16, 2016

frithnanth commented Dec 16, 2016 via email

koppor commented Aug 18, 2017

matthiasgeiger commented Dec 2, 2016 •

edited

Loading