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

Cleanup guiglobals #1409

Merged
merged 23 commits into from
May 23, 2016
Merged

Cleanup guiglobals #1409

merged 23 commits into from
May 23, 2016

Conversation

lenhard
Copy link
Member

@lenhard lenhard commented May 19, 2016

Implements #1408

Cleans up the GUIGlobals class and move LatexFieldFormatter to the bibtex package, which is now independent of the gui package.

As a final step, I would move the class net.sf.jabref.importer.fileformat.FieldContentParser to the bibtex package, which would make the package self-contained (apart from imports to the preferences and related classes) and would move the whole bibtex package into logic. Are there objections to this?

@lenhard
Copy link
Member Author

lenhard commented May 19, 2016

I restructured variable positioning as discussed in #1408 and turn MAX_BACK_HISTORY_SIZE, LINE_LENGTH, and INDENT into proper preferences.

@JabRef/developers What about turning the bibtex package into a subpackage of logic? Or should that rather stay a top-level package?

@lenhard lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 19, 2016
@simonharrer
Copy link
Contributor

The more stuff is in the logic package, the better.

@lenhard
Copy link
Member Author

lenhard commented May 19, 2016

Ok, from side this is good to go. I even was able to move some more classes into the model package.

@matthiasgeiger
Copy link
Member

LGTM

import net.sf.jabref.specialfields.SpecialFieldsUtils;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

public class JabRefPreferences {
// Character separating field names that are to be used in sequence as
// fallbacks for a single column (e.g. "author/editor" to use editor where
// author is not set):
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is placed wrongly, I think.

@tobiasdiez
Copy link
Member

tobiasdiez commented May 20, 2016

LGTM, just some minor remarks.

What I do not understand is the purpose of the bibtex package. Some of the classes seem like they do not have anything to do with writing or parsing bibtex and are rather general, for example the comparators. Maybe it would make sense to completely dissolve the bibtex package by moving the parsing logic to the import package and writing to export...in the end the bibtex format is just a special import-export format.

@lenhard
Copy link
Member Author

lenhard commented May 20, 2016

@tobiasdiez Thanks for your remarks, I will integrate them as well.

As for the bibtex package: Its existence is a historical thing and I do not know the exact motivation behind. It seems like "bundle parsing and writing of pure bibtex", but, with the addition of new importers and exporters, responsibilities shifted and mixed over time. The big advantage we have now is that the package moved to logic, which means that there is a guarantee that it is independent of GUI code and it can be re-used for new GUI stuff with relative safety. If we move it out of logic (to importers or exporters) we loose that advantage. Ultimately, the goal should be to get the GUI independent code from the import/export package into logic as well.

Out of topic: I just noticed the codecov report. How the hell did code coverage increase by a wooping 11.27% with this PR? I didn't even write new tests. This has to be an error.

@lenhard
Copy link
Member Author

lenhard commented May 20, 2016

I implemented the above comments and the PR is once again ready to go from my side.

One remark: COL_DEFINITION_FIELD_SEPARATOR cannot go to GUIGlobals, since this would create dependencies from logic to gui. Therefore, I moved it into COL_DEFINITION_FIELD_SEPARATOR to Globals. This is fair, I think, since the variables relates to our BibTeX sytnax and field content, which is independent of the GUI.

I still don`t get how the coverage can have increased that much.

@simonharrer simonharrer merged commit 1e644d3 into master May 23, 2016
@simonharrer simonharrer deleted the cleanup-guiglobals branch May 23, 2016 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants