Skip to content

Some OO/LO cleanups#1927

Merged
tobiasdiez merged 1 commit into
JabRef:masterfrom
oscargus:oofixes
Sep 11, 2016
Merged

Some OO/LO cleanups#1927
tobiasdiez merged 1 commit into
JabRef:masterfrom
oscargus:oofixes

Conversation

@oscargus

@oscargus oscargus commented Sep 6, 2016

Copy link
Copy Markdown
Contributor

Removed deprecated calls, renamed a few variables, and restructured the code a bit.

@oscargus oscargus added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers component: libre-office dev: code-quality Issues related to code or architecture decisions labels Sep 6, 2016
clonedEntry.setId(IdGenerator.next());
// Insert a copy of the entry
resultDatabase.insertEntryWithDuplicationCheck(clonedEntry);
resultDatabase.insertEntry(clonedEntry);

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.

Just wondering: Is it save to remove the duplication check here? The documentation of insertEntryWithDuplicationCheck says use insertEntry and DuplicationChecker separately.

The comment also applies to the replacement below.

@lenhard

lenhard commented Sep 6, 2016

Copy link
Copy Markdown
Member

Apart from the clarification above: LGTM!

@oscargus

oscargus commented Sep 6, 2016

Copy link
Copy Markdown
Contributor Author

Well the duplication check just returns a boolean and as that is not checked here it shouldn't make any functional difference. If it was a good idea in the first place is another question though...

@tobiasdiez

Copy link
Copy Markdown
Member

LGTM 👍

@tobiasdiez tobiasdiez merged commit fdfbcac into JabRef:master Sep 11, 2016
Siedlerchr added a commit that referenced this pull request Sep 11, 2016
* 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
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: libre-office dev: code-quality Issues related to code or architecture decisions 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.

3 participants