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.

Sign up for GitHub

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

Jump to bottom

Removed Globals from BibDatabaseContext #1856

Merged
merged 2 commits into from
Aug 26, 2016
Merged

Removed Globals from BibDatabaseContext #1856

merged 2 commits into from
Aug 26, 2016

Conversation

oscargus
Copy link
Contributor

This almost worked. The penalty was that I had to (temporarily) introduce it in FileUtil.expandFilename where the arbitrary extension line makes it extremely cumbersome to get around it.

@oscargus oscargus added type: code-quality Issues related to code or architecture decisions architecture labels Aug 25, 2016
@oscargus oscargus force-pushed the noglobalsinbibdatabasecontext branch 2 times, most recently from e2c7de6 to a480dee Compare August 25, 2016 16:14
Siedlerchr
Siedlerchr reviewed Aug 25, 2016
View reviewed changes
src/main/java/net/sf/jabref/external/WriteXMPAction.java
.ifPresent(files::add));
entry.getFieldOptional(FieldName.PDF)
.ifPresent(
pdf -> FileUtil
Copy link
Member

Choose a reason for hiding this comment

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

The formatting here and below looks a bit weird...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of them was sorted out with the new code, but not the other...

@oscargus
Copy link
Contributor Author

I think I know how to get around this now. It seems like there are three possible directories "fileDirectory", "pdfDirectory", and "psDirectory", where only the first one is normally used and the other comes from the legacy pdf and ps fields. Now, it might be possible to hard-code in the preferences support for more extensions, but do we really need to support that or would it be OK to include a limitation to only those three?

@Siedlerchr
Copy link
Member

LGTM, jut two weird formatting things.
Regardng the FileUtil class. I will rework this., because it contains a lot of outdated code which can be replaced with Standard Java implementation. Maybe we could get rid of some prefs there.

Regarding the directories, there is also a discussion about the import/export dir preferences #1813
I don't think we need the legacy pdf/ps fields. Isn't there a cleanup job to move them?

@oscargus
Copy link
Contributor Author

There was an old issue where someone used them for something completely different, so maybe we shouldn't remove them. As long as it is only those three (or at least a well known subset of all possible extDirectory it should be OK. Now the problem is that one can use any extension and then the code tries to find a dedicated directory for that extension, which cannot be generated in any other way than editing the preference XML...

@oscargus
Copy link
Contributor Author

#496 I think. Although that doesn't discuss arbitrary extensions (from a quick browse).

@lenhard
Copy link
Member

lenhard commented Aug 26, 2016

I would also say that it is ok to remove support for the legacy file directories. And I do not really see why we should support arbitrary extension directories.

@oscargus
Copy link
Contributor Author

OK, dropping arbitrary extensions I'm fine with, but I will not remove support for the pdf and ps directories right now.

@oscargus oscargus force-pushed the noglobalsinbibdatabasecontext branch from a480dee to 9d7b26e Compare August 26, 2016 11:10
@oscargus oscargus force-pushed the noglobalsinbibdatabasecontext branch from 9d7b26e to b821a4e Compare August 26, 2016 11:12
@oscargus oscargus mentioned this pull request Aug 26, 2016
Closed
@oscargus
Copy link
Contributor Author

Now there are no more dependencies on Globals or logic in BibDatabaseContext. Still a dependency on shared though, but that I haven't looked into.

@oscargus oscargus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 26, 2016
@oscargus oscargus mentioned this pull request Aug 26, 2016
Closed
@oscargus oscargus merged commit 0d676a1 into JabRef:master Aug 26, 2016
@oscargus oscargus deleted the noglobalsinbibdatabasecontext branch August 26, 2016 17:15
ayanai1 pushed a commit to ayanai1/jabref that referenced this pull request Sep 5, 2016
@oscargus @ayanai1
Removed Globals from BibDatabaseContext (JabRef#1856)
288d2b9 
* Removed Globals from BibDatabaseContext

* Removed the temporarily introduced Globals dependency in logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions type: question
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oscargus @Siedlerchr @lenhard