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

Extract PushTo names into model #8005

Merged
merged 3 commits into from Aug 20, 2021
Merged

Extract PushTo names into model #8005

merged 3 commits into from Aug 20, 2021

Conversation

DominikVoigt
Copy link
Contributor

@DominikVoigt DominikVoigt commented Aug 20, 2021

This PR serves as a step to remove the dependence of the preferences on gui components.
In this PR some string constants are extracted from the gui component to inverse the depencendence.

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@@ -0,0 +1,12 @@
package org.jabref.model;

public class JabRefStringConstants {
Copy link
Member

@calixtus calixtus Aug 20, 2021

Choose a reason for hiding this comment

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

Maybe missing a private empty constructor, so this class can't be instantiated.
Codewise LGTM, although I really don't know if this is good practice. But to move forward and calm @koppor s temper...

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @calixtus not sure either. I would at least rename the class to PushToApplicationConstants and would have expected them in a supackge model/push/

Copy link
Member

Choose a reason for hiding this comment

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

Name of the class came up yesterday as we were thinking about collecting all the commonly used constant strings in jabref in that class.

Please add the private constructor, then we should merge, to start with the little things.

Add private constructor.
@Siedlerchr Siedlerchr merged commit 631f27c into main Aug 20, 2021
@Siedlerchr Siedlerchr deleted the introduce-string-constants branch August 20, 2021 16:33
@Siedlerchr
Copy link
Member

thx!

Siedlerchr added a commit that referenced this pull request Aug 20, 2021
* upstream/main: (110 commits)
  Extract PushTo names into model (#8005)
  Refactor processCitation in GrobidService to match processPdf (#8003)
  Improved progress indication for fulltext-index operations (#7981)
  Reordered Pdf-Importer priorities (#8001)
  Implement more pdf importers (#7947)
  Adding icon picker for group dialog issue#6142 (#7776)
  Fix possible NPE in exporter with empty charset (#7979)
  Fix icon color (#7994)
  Bump slf4j-api from 2.0.0-alpha2 to 2.0.0-alpha4 (#7991)
  Bump classgraph from 4.8.112 to 4.8.114 (#7990)
  Bump mariadb-java-client from 2.7.3 to 2.7.4 (#7992)
  Bump jsoup from 1.14.1 to 1.14.2 (#7993)
  New yaml issue template (#7983)
  [Bot] Update CSL styles (#7985)
  Reordered items in main table right-click menu (#7952)
  Fulltext Index: Only index local pdf files (#7980)
  Bump WyriHaximus/github-action-wait-for-status from 1.3 to 1.4 (#7973)
  Bump byte-buddy-parent from 1.11.9 to 1.11.12 (#7974)
  Bump classgraph from 4.8.110 to 4.8.112 (#7975)
  Bump checkstyle from 8.45 to 8.45.1 (#7978)
  ...

# Conflicts:
#	src/main/java/module-info.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants