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

Improve database writer #718

Merged
merged 13 commits into from
Feb 11, 2016
Merged

Conversation

tobiasdiez
Copy link
Member

The aim of this PR is to improve the database writer, in particular fix #621 and #669.
Also add tests to the writer and parser (and do some refactorings to make test writing easier).

Changes (to serialization):

  • Fix Ver 3.1 comment not written correctly. #621 and JabRef 3.1/3.2 writes bib file in a format that it will not read #669 by adding new lines after encoding and preamble. The idea is that everything is terminated by a new line and the parsed serialization only safes newlines which separate it from the previous entry.
  • Comments and preamble are serialized with capitalized first letter, i.e. @Comment instead of @comment and @Preamble instead of @PREAMBLE. This should follow the convention for entries.
  • There is no space anymore between @String and the bracket and after the bracket, i.e. @String{name = {string}} instead of @String { name = {string} }
  • Implemented #756: Add possibility to reformat all entries on save (under Preferences, File)

Refactorings:

  • Renamed FileActions to BibDatabaseWriter
  • Merged code for export and save in BibDatabaseWriter
  • Extracted SaveSettings class from FileActions to own file and extended it (& renamed to SavePreferences). Note that this class has some setters (against the recently established convention for preference classes) since there are quite a few cases where the values from the preferences have to be overriden.
  • Change usage of arrays to list in many places
  • Remove unused boolean parameters checkSearch, checkGroup
  • Moved writing of metadata and CustomEntryTypesManager to BibDatabaseWriter
  • Extracted and reused gui code for displaying and editing sort preferences (SaveOrderConfigDisplay)

@simonharrer
Copy link
Contributor

Could you rebase this on master as both pre-requisite PRs are already merged in. So this could be completed now.

@tobiasdiez
Copy link
Member Author

One question: currently the save methods take parameters boolean checkSearch, boolean checkGroup which specify that only entries satisfying the active search / group selection are exported or saved. As far as I can see, these boolean parameters are always set to false (which means all entries are exported). Should I remove both parameters?

@oscargus
Copy link
Contributor

oscargus commented Feb 2, 2016

I guess that if this is merged in first, it should be quite OK to rebase #762 (although many changes are redundant).

Regarding your question: it would be nice to be able to save like this, but I guess it is another of JabRefs almost finished/commented out and then removed features... Considering that one can save selected entries and it is quite easy to see which entries are in the active group/search selection, I'd say remove.

@tobiasdiez tobiasdiez changed the title [WIP] Improve database writer Improve database writer Feb 3, 2016
@tobiasdiez tobiasdiez added bug Confirmed bugs or reports that are very likely to be bugs testing status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Feb 3, 2016
@tobiasdiez
Copy link
Member Author

I think the code is now mature enough to be reviewed. I added some notes above in the first post. Sorry that this PR is rather big.

@@ -32,8 +32,6 @@ public BibDatabaseContext(BibDatabase database, MetaData metaData, Defaults defa
this.defaults = Objects.requireNonNull(defaults);
this.database = Objects.requireNonNull(database);
this.metaData = Objects.requireNonNull(metaData);

this.setMode(getMode());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you delete this? What happens if you load a bib file and save it afterwards - is there a comment about the type stored in the file then still?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the line because otherwise the biblatex mode would be written every time even if it is the default one. In addition, I found it irritating that a constructor changed values in the parameters passed to it (just using a metadata in a specific context shouldn't change it values).

But yes, the biblatex mode is correctly written. I added a test for it now.

@tobiasdiez
Copy link
Member Author

I also pushed it to the branch JabRef/databaseWriter so that a build should be available under http://builds.jabref.org/.

@simonharrer
Copy link
Contributor

Do we have the "only update if it changed" functionality for the preamble, strings and comments as well?

@tobiasdiez
Copy link
Member Author

For strings and the epilogue yes, for the preamble kind of (the text in the preamble is stored and exactly rewritten, but the preamble tag and newlines around it are not stored). For the metadata there is no such functionality.

@koppor
Copy link
Member

koppor commented Feb 8, 2016

@tobiasdiez Regarding the two parameters (#718 (comment)): Aren't they used for "Save selected as plain BibTeX..."?

@tobiasdiez
Copy link
Member Author

@koppor No it is not used for this function. "Save selected..." gets all the selected items and pass them to the save function. The code in question reads

if (selectedOnly) {
                session = FileActions.savePartOfDatabase(
                        panel.getBibDatabaseContext(),
                        file, Globals.prefs,
                        panel.getSelectedEntries(), encoding, FileActions.DatabaseSaveType.DEFAULT);
            } else {
                session = FileActions.saveDatabase(panel.getBibDatabaseContext(),
                        file, Globals.prefs, false, false, encoding, false);
            }

@simonharrer
Copy link
Contributor

Please rebase. I think this should be merged.

One suggestion regarding the SavePreferences: You could create a new instance after the call of a setter. This would result in the benefits of having final variables and it still allows to create derivations of this.

SavePreferences prefs = new SavePreferences(Globals.prefs);
SavePreferences otherInstance = prefs.withEncoding("utf8");
assert prefs != otherInstance && !prefs.equals(otherInstance)

Combine saveDatabase and savePartOfDatabase to one underlying method.
Introduce new class SavePreferences which controls how the database is
written. Move a few methods from the writer to another class (like
`getReader`) or from another place to the writer (like `writeMetaData`)
Extract display of order config from DatabasePropertiesDialog and
FileSortTab to special class which can be reused.
…ialization

Also adds a lot of tests for serialization (and a few to the parser)
@tobiasdiez
Copy link
Member Author

Rebased and implemented the withEncoding suggestion (i.e. removed all setters in the preferences...you really don't like them, right? 😄 )

simonharrer added a commit that referenced this pull request Feb 11, 2016
@simonharrer simonharrer merged commit 5b70e9c into JabRef:master Feb 11, 2016
@tobiasdiez tobiasdiez deleted the databaseWriter branch February 11, 2016 09:45
@jkomelj
Copy link

jkomelj commented Feb 27, 2016

In my case (JabRef 3.2 on 32- and 64-bit Windows the problem is not fixed. Original lines
% Encoding: windows-1250

@Preamble{{\providecommand{\noopsort}[1]{}}} (or @Preamble ...)

are always changed to
% Encoding: windows-1250@PREAMBLE{{\providecommand{\noopsort}[1]{}}}

after saving changed bib file.

Kind regards,
Janez Komelj

@tobiasdiez
Copy link
Member Author

@jkomelj You have to use the latest development version or wait for the release of 3.3 (probably in 1-2 weeks)

@jkomelj
Copy link

jkomelj commented Feb 27, 2016

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bugs or reports that are very likely to be bugs cleanup-ops status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ver 3.1 comment not written correctly.
5 participants