Skip to content

Removed external dependency in logic#1934

Merged
lenhard merged 2 commits into
JabRef:masterfrom
oscargus:noexternalinlogic
Sep 10, 2016
Merged

Removed external dependency in logic#1934
lenhard merged 2 commits into
JabRef:masterfrom
oscargus:noexternalinlogic

Conversation

@oscargus

@oscargus oscargus commented Sep 7, 2016

Copy link
Copy Markdown
Contributor

An alternative to #1924 : hard coded PS and PDF as file type names.

Still some methods that can be removed in ExternalFileType (the original thing done in #1924 .

I created three gui packages:

  • externalfiletypes for things related to ExternalFileType
  • externalfiles for things (primarily) related to the actual files
  • filelist for things related to FileList
  • Change in CHANGELOG.md described

@oscargus oscargus added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers architecture labels Sep 7, 2016
tobiasdiez
tobiasdiez reviewed Sep 8, 2016
View reviewed changes
Comment thread src/main/java/net/sf/jabref/gui/importer/ImportInspectionDialog.java Outdated
JDialog diag = new JDialog(ImportInspectionDialog.this, true);
JabRefExecutorService.INSTANCE.execute(
net.sf.jabref.external.AutoSetLinks.autoSetLinks(entry, localModel, bibDatabaseContext, e -> {
net.sf.jabref.gui.externalfiles.AutoSetLinks.autoSetLinks(entry, localModel, bibDatabaseContext, e -> {

@tobiasdiez tobiasdiez Sep 8, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use package import

@oscargus oscargus Sep 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed.

@oscargus oscargus Sep 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There was a local class with the same name. I renamed that.

@tobiasdiez

tobiasdiez commented Sep 8, 2016

Copy link
Copy Markdown
Member

LGTM 👍
I like this solution. Personally, I wouldn't separate externalfiletypes and externalfile, but this is probably a matter of taste.

@lenhard

lenhard commented Sep 8, 2016

Copy link
Copy Markdown
Member

LGTM as well! And I am in favor of the separation.

Just for clarification before merging: Do you still intend to move ExternalFileType and ExternalFileTypes into one of the other packages? Or, since the external dependencies on the classes are removed now, will you keep them in gui?

Both ways would be fine in a sense. If those classes are exclusively used in gui, it is fine to keep them there, even if they are could be refactored in a gui-independent fashion.

Before merging the build has to succeed of course.

@oscargus

oscargus commented Sep 8, 2016

Copy link
Copy Markdown
Contributor Author

The plan is to keep it as is as it solves the problem.

One may indeed think of different solutions regarding the amount of packages and there is no clear border, especially considering filelist.

Regarding the builds: clearly I picked the wrong field in ExternalFileType... Will fix it.

@oscargus

oscargus commented Sep 8, 2016

Copy link
Copy Markdown
Contributor Author

Correct field. Bad at reading...

@oscargus

oscargus commented Sep 8, 2016

Copy link
Copy Markdown
Contributor Author

Not sure why the Circle CI-tests are not executed, but the things are fixed now anyway.

@tobiasdiez tobiasdiez closed this Sep 10, 2016
@tobiasdiez tobiasdiez reopened this Sep 10, 2016
@tobiasdiez

tobiasdiez commented Sep 10, 2016

Copy link
Copy Markdown
Member

I closed and reopened the PR to give travis another chance to build the branch.
So can we merge this?

@lenhard

lenhard commented Sep 10, 2016

Copy link
Copy Markdown
Member

Good idea and it worked! The build succeeded, so we can merge it.

@lenhard lenhard merged commit 71a215d into JabRef:master Sep 10, 2016
@tobiasdiez tobiasdiez mentioned this pull request Sep 11, 2016
Closed
Siedlerchr added a commit that referenced this pull request Sep 11, 2016
@Siedlerchr
Merge branch 'master' into xmlunit
9be2619 
* master:
  Remove obsolete wrapper task
  Added error dialog when setting invalid main file directory (#1921)
  Add filteringCharset = 'UTF-8' (#1945)
  Include https://github.com/grimes2
  Searchbar across all bib files instead each having its own (#1549)
  Some OO/LO cleanups (#1927)
  Update link
  Removed external dependency in logic (#1934)
Siedlerchr added a commit to Siedlerchr/jabref that referenced this pull request Sep 11, 2016
@Siedlerchr
Merge branch 'master' into cleanupWarnings
21d39fe 
* master:
  Remove obsolete wrapper task
  Added error dialog when setting invalid main file directory (JabRef#1921)
  Add filteringCharset = 'UTF-8' (JabRef#1945)
  Include https://github.com/grimes2
  Searchbar across all bib files instead each having its own (JabRef#1549)
  Some OO/LO cleanups (JabRef#1927)
  Update link
  Removed external dependency in logic (JabRef#1934)
zesaro pushed a commit to zesaro/jabref that referenced this pull request Nov 22, 2016
@oscargus @zesaro
Removed external dependency in logic (JabRef#1934)
b321930 
* Removed external dependency in logic

* Fixed comments and move one class
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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@oscargus @tobiasdiez @lenhard