Refactoring: completey typed metadata by tobiasdiez · Pull Request #2112 · JabRef/jabref

tobiasdiez · 2016-10-03T20:41:56Z

The metadata class is completely rewritten and now only uses typed fields instead of a map of serialized strings. Serialization and parsing now happens in the dedicated MetadataSerializer/parser classes.
For this also some of the cleanup classes had to be moved to the model package.

Final big refactoring PR from my side 😄

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

Siedlerchr · 2016-10-03T21:04:36Z

I think your problem with the failing test couuld be due to the renaming of the DBMSSynchronizerTest.java
There was a case change of the test name. At least I had a similar problem a while ago on Win.

lenhard

Overall this looks good :-) I would have expected us to take much much longer for arriving at a globals-free and gui-free meta data class.

Most of my remarks are just requests for clarification and I have a few suggestions for improvement.

lenhard · 2016-10-04T13:53:31Z

-                }
-                // Remember that we've handled this one:
-                handledOnDisk.add(key);
+    private void scanMetaData(MetaData inMemory, MetaData onTmp, MetaData onDisk) {


With your new implementation here, we lose information, don't we? Previously the actual key of different data made it into the change event, now we just get the information that the meta data as whole have changed. Am I right? And might this be a problem?

Yes the detailed information about which key has changed is not contained in the event. Of course this information could be easily extracted from the two metadata by comparing each contained meta item separately. I didn't found this information too useful and thus just left a todo in the dialog https://github.com/JabRef/jabref/pull/2112/files#diff-423409b75a40e426ad5123cabbe295e6R33.

lenhard · 2016-10-04T13:55:09Z

 /**
 *
 */
 class MetaDataChange extends Change {


See above. The changes displayed here depend of course on what is available. Why is the type of change no longer of interest?

lenhard · 2016-10-04T14:00:11Z

                groupsRoot.addNewGroup(newGroup, panel.getUndoManager());
                panel.markBaseChanged();
                frame.output(Localization.lang("Created group \"%0\".", newGroup.getName()));
-                panel.getBibDatabaseContext().getMetaData().postChange();


Why are all the change notifications in this class no longer needed?

The metadata class now directly subscribes to changes in the group tree and relays these change events https://github.com/JabRef/jabref/pull/2112/files#diff-20cd356a80ee8ffb10c848005c89b949R74

lenhard · 2016-10-04T14:06:18Z

     * Writes all data in the format <key, serialized data>.
     */
-    public static Map<String, String> getSerializedStringMap(MetaData metaData) {
+    public static Map<String, String> getSerializedStringMap(MetaData metaData,


How about refactoring this method a little bit? After all, it consists of various blocks, where different types of meta data contents are put into the final list. Each such block could be extract into a beautifully named private method :-)

I like that one sees directly in one method all the metadata plus their key. Instead of moving everything to separate methods, I moved the main serialization to a different method so that the code now looks cleaner and is better to understand.

lenhard · 2016-10-04T14:12:52Z

@@ -0,0 +1,122 @@
+package net.sf.jabref.model.cleanup;


I have a suggestion regarding this new package: How about moving it into model.metadata? After all, the classes in the package are about how the cleanup information is stored in the meta data. What do you think?

Mhhh.... none of the other metadata items are currently under the metadata namespace:
import net.sf.jabref.model.cleanup.FieldFormatterCleanups;
import net.sf.jabref.model.bibtexkeypattern.AbstractBibtexKeyPattern;
import net.sf.jabref.model.database.BibDatabaseMode;
import net.sf.jabref.model.groups.GroupTreeNode;

I think it is more consistent to have a model.cleanup or also move the rest to model.metadata(.groups/.bibtexkeypattern...)

Ok, at some point we might think of structuring the model package a little more, but for now it is ok, I think.

@calixtus and @koppor currently wonder why this class was moved to model. All implementing classes are in the logic package:

lenhard · 2016-10-04T14:16:45Z

-        if (bibtexKeyPattern != null) {
-            return bibtexKeyPattern;
-        }
+    public AbstractBibtexKeyPattern getCiteKeyPattern(GlobalBibtexKeyPattern globalPattern) {


Since you are doing this for the other methods: Do a null check of globalPattern.

Good observation :) Done.

lenhard · 2016-10-04T14:21:01Z

    public Optional<BibDatabaseMode> getMode() {
-        List<String> data = getData(DATABASE_TYPE);
-        if ((data == null) || data.isEmpty()) {
-            return Optional.empty();


Why is the Optional.empty no longer a desired return type here?

If mode is null, then still an empty optional is returned (Optional.ofNullable(mode))

lenhard · 2016-10-04T14:23:07Z

    public void markAsProtected() {
-        putData(PROTECTED_FLAG_META, Collections.singletonList("true"));
+        isProtected = true;
+        postChange();


So the change notification I asked for earlier have been moved into the meta data. Am I right?

Yes every setSomething method now has to post that the metadata changed. Previously this was done in the putData method.

lenhard · 2016-10-04T14:25:39Z

 1 KeywordGroup:DynamicGroup\;0\;author\;Churchill\;0\;0\;;
 }

-@Comment{jabref-meta: groupsversion:3;}


Is it ok to just remove that? Wouldn't it be better to have it in and verify that it is just ignored by the new parser?

We have a test integrationTestGroupTree https://github.com/JabRef/jabref/pull/2112/files#diff-837597ff5fafbe4e71d5733d490e3454R1479 which still have the ̀groupsversion:3 string. But since we no longer write the the groupsversion information, the roundtrip tests for complex.bib fail. This is why I removed it.

tobiasdiez · 2016-10-04T16:24:49Z

Thanks for the review. I addressed all your remarks and hopefully the build now runs through without problems.

Siedlerchr · 2016-10-04T20:50:57Z

Regarding the failing tests: When you log in to Travis-CI and check the build, you can access the raw log.
In your case it showed:


Formatting ./build/test-results/databaseTest/TEST-net.sf.jabref.shared.DBMSSynchronizerTest.xml
=====================================================
Testsuite: net.sf.jabref.shared.DBMSSynchronizerTest
Tests run: 16, Failures: 2, Errors: 0, Time elapsed: 2.522 sec
--------- ----------- ---------
Testcase: testMetaDataChangedEventListener[Test with PostgreSQL database system] took 0.209 FAILURE java.lang.AssertionError: expected:<{databaseType=bibtex;}> but was:<{}> java.lang.AssertionError
java.lang.AssertionError: expected:<{databaseType=bibtex;}> but was:<{}>

    at net.sf.jabref.shared.DBMSSynchronizerTest.testMetaDataChangedEventListener(DBMSSynchronizerTest.java:134)

Siedlerchr · 2016-10-04T20:54:25Z

There is also an error which seems to be related to your changes:


Oct 04, 2016 6:04:34 PM com.google.common.eventbus.EventBus$LoggingHandler handleException
SEVERE: Exception thrown by subscriber method listen(net.sf.jabref.model.metadata.event.MetaDataChangedEvent) on subscriber net.sf.jabref.shared.DBMSSynchronizer@5d506d49 when dispatching event: net.sf.jabref.model.metadata.event.MetaDataChangedEvent@34a71a73
java.lang.NullPointerException
    at net.sf.jabref.shared.DBMSSynchronizer.listen(DBMSSynchronizer.java:121)

* upstream/master: (102 commits) Removed unused test code (#2140) Fix main table bug when creating a duplicate (#2135) Remove explicit author and add SPDX-License-Identifier Remove GPL from README.md and CONTRIBUTING.md fix preview update (#2125) Remove some UnicodeToLatex uses (#2132) Fix mixup in french/farsi localization FetcherException should extend JabRefException Fix exception when opening preference dialog (#2127) Unify ParserException and ParseException (#2124) Small refactoring in Importer package (#2053) Implement Datepicker "none"-button (#2122) revert change from 816d30c Change title/tooltip of source panel in biblatex mode (#2120) Refactoring: completey typed metadata and add detailed travis output (#2112) RTFchars fix (#2097) Fix NPE in Medline fetcher on missing ISSN (#2113) Ctrl-s parsing error message (#2114) Fix bad web search error messages (#2034) parse error freeze (#2106) ... # Conflicts: # src/main/java/net/sf/jabref/collab/FileUpdateMonitor.java # src/main/java/net/sf/jabref/gui/externalfiles/DownloadExternalFile.java # src/main/java/net/sf/jabref/gui/externalfiles/DroppedFileHandler.java # src/main/java/net/sf/jabref/gui/externalfiles/MoveFileAction.java # src/main/java/net/sf/jabref/logic/cleanup/RenamePdfCleanup.java # src/main/java/net/sf/jabref/logic/exporter/FileSaveSession.java # src/main/java/net/sf/jabref/logic/util/io/FileUtil.java # src/main/java/net/sf/jabref/preferences/JabRefPreferences.java

obraliar · 2016-11-01T13:36:38Z

@tobiasdiez Since this merge the synchronization of metadata for shared databases is totally broken. Could you please investigate why this happened? There is also a new issue #2219.

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

@obraliar: Looking at the code, there have been changes in src/main/java/net/sf/jabref/shared/DBMSSynchronizer.java, so my best guess would be those changes. Could you have a look at the changes to this class? Maybe some meta data are not carried over properly due to the typing.

Lacking an actual database for testing, I cannot provide more help unfortunately. It seems that we need more tests for the database code, as travis did not find this problem.

tobiasdiez · 2016-11-01T14:18:46Z

Sorry I have no db to test the sync (and no intellij at the moment either). I have to admit, I kind of trusted that our db-tests would catch possible problems.

…abRef#2112) * Rewrite MetaData * Move some cleanup classes to model * Restoe test bib * Fix failing tests * Cleanup code in MetaDataSerializer * Add null check * Make database test compile again * Fix import order * Reorder metadata * Fix database tests * Show test results after failure * Fix path * Really fix path * Maybe now the path is correct * I mean now... * set permission for after-failure.sh * Comment out echo * Fix NPE exception due to Globals.prefs * Remove unused import

tobiasdiez added 4 commits October 3, 2016 21:23

Rewrite MetaData

1661459

Move some cleanup classes to model

41dd958

Restoe test bib

e8367d8

Fix failing tests

efe10cb

tobiasdiez added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers dev: code-quality Issues related to code or architecture decisions labels Oct 3, 2016

lenhard suggested changes Oct 4, 2016

View reviewed changes

tobiasdiez added 5 commits October 4, 2016 18:06

Cleanup code in MetaDataSerializer

aeea973

Add null check

01922dc

Make database test compile again

150a078

Fix import order

dc950fc

Reorder metadata

4751cc4

tobiasdiez added 7 commits October 4, 2016 18:32

Fix database tests

754ad5a

Show test results after failure

021f25f

Fix path

a62fd70

Really fix path

525eaab

Maybe now the path is correct

d34ec54

I mean now...

a77fe47

set permission for after-failure.sh

8cbefc0

tobiasdiez closed this Oct 4, 2016

tobiasdiez reopened this Oct 4, 2016

Comment out echo

81f1d0a

tobiasdiez closed this Oct 4, 2016

tobiasdiez reopened this Oct 4, 2016

tobiasdiez added 2 commits October 5, 2016 09:56

Fix NPE exception due to Globals.prefs

cfa56b0

Remove unused import

d6e66e7

tobiasdiez merged commit 7d84274 into JabRef:master Oct 5, 2016

tobiasdiez deleted the metadataRefactTyped branch October 5, 2016 10:44

Uh oh!

Conversation

tobiasdiez commented Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Siedlerchr commented Oct 3, 2016

Uh oh!

lenhard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez commented Oct 4, 2016

Uh oh!

Siedlerchr commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Siedlerchr commented Oct 4, 2016

Uh oh!

obraliar commented Nov 1, 2016

Uh oh!

lenhard commented Nov 1, 2016

Uh oh!

tobiasdiez commented Nov 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tobiasdiez commented Oct 3, 2016 •

edited

Loading

tobiasdiez Oct 4, 2016 •

edited

Loading

Siedlerchr commented Oct 4, 2016 •

edited

Loading