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

Fix issue #9306: Move "Open only one instance of JabRef" preference option to somewhere else #10602

Merged
merged 13 commits into from Oct 30, 2023

Conversation

DiamondMyK
Copy link
Contributor

@DiamondMyK DiamondMyK commented Oct 29, 2023

Closes #9306

Describe

I have moved the "Open only one instance of JabRef" preference option to a new location("General"). This change was made to improve the user experience and make the option more accessible.

Mandatory checks

  • 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 developer's documentation: The information is available and up to date.
  • Checked documentation: The information is available and up to date.

Screenshots

Before:
229559601-4312bfaf-0a38-4b09-9c9f-9caa6790318d

After:
222

Additional Note

I am a student, and this contribution is part of my assignment related to open-source technology. My teacher will be evaluating the completed work. I kindly request a timely review of this PR to meet the assignment deadline. Your assistance is greatly appreciated.

issue#9306.patch Outdated
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6e5a785389..b7136d7689 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
Copy link
Member

Choose a reason for hiding this comment

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

Pleaes delete this file or at least do not commmit it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this is my oversight, I will delete this file!

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

see my comments

@DiamondMyK
Copy link
Contributor Author

see my comments

Thank you very much for your advice! I have committed my changes. :)

@@ -127,10 +136,18 @@ public void initialize() {
backupDirectory.textProperty().bindBidirectional(viewModel.backupDirectoryProperty());
backupDirectory.disableProperty().bind(viewModel.createBackupProperty().not());

ActionFactory actionFactory1 = new ActionFactory(Globals.getKeyPrefs());
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this, as there is already an instance of ActionFactory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK!:)

CHANGELOG.md Outdated
@@ -16,6 +16,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv

### Changed

- We changed the location of the 'Open only one instance of JabRef' preference option.[#9306](https://github.com/JabRef/jabref/issues/9306)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- We changed the location of the 'Open only one instance of JabRef' preference option.[#9306](https://github.com/JabRef/jabref/issues/9306)
- We moved the location of the 'Open only one instance of JabRef' preference option from "Network" to "General" .[#9306](https://github.com/JabRef/jabref/issues/9306)

@@ -52,7 +52,7 @@

<CheckBox fx:id="fontOverride" text="%Override default font settings"
GridPane.rowIndex="4" GridPane.columnIndex="0"/>
<HBox alignment="CENTER_LEFT" spacing="4.0" disable="${!fontOverride.selected}"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why this was deleted? If not then revert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's my negligence. I'm sorry to cause you trouble!

Copy link
Member

Choose a reason for hiding this comment

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

No worries, you are here to learn something by contributing to a real open source project and it's part of our mission statement to guide students

PS: May I ask from which class/university etc you are?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, you are here to learn something by contributing to a real open source project and it's part of our mission statement to guide students

PS: May I ask from which class/university etc you are?

Thank you. I am a student from NEU of China. I found the test failed and I'm fixing it now.

@@ -6,3 +6,5 @@ springerNatureAPIKey=${springerNatureAPIKey}
astrophysicsDataSystemAPIKey=${astrophysicsDataSystemAPIKey}
ieeeAPIKey=${ieeeAPIKey}
biodiversityHeritageApiKey=${biodiversityHeritageApiKey}
Single\ instance=????
Copy link
Member

Choose a reason for hiding this comment

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

Ah, that is some issue with intellij. We need to refine our guides.
https://devdocs.jabref.org/code-howtos/localization.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a simple modification, but I don't know if I can pass the test. My knowledge is too shallow, I am new to open source, I really appreciate your help.

@@ -6,3 +6,5 @@ springerNatureAPIKey=${springerNatureAPIKey}
astrophysicsDataSystemAPIKey=${astrophysicsDataSystemAPIKey}
ieeeAPIKey=${ieeeAPIKey}
biodiversityHeritageApiKey=${biodiversityHeritageApiKey}
Single instance=????
Copy link
Member

Choose a reason for hiding this comment

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

The Problem is that IntelliJ suggests the wrong file:
You need to put these strings into src/main/resources/l10n/en.properties

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://devdocs.jabref.org/code-howtos/localization.html

Should I modify it in GeneralTab.class?

@Siedlerchr
Copy link
Member

If you click on "Details" you will the reason for the failing tests
grafik

@DiamondMyK
Copy link
Contributor Author

If you click on "Details" you will the reason for the failing tests grafik

I followed the instructions and the test failed. Can you give me some help?

@DiamondMyK
Copy link
Contributor Author

If you click on "Details" you will the reason for the failing tests grafik

I can't localize whatever changes I make, and I think I need your help! Thank you very much!

@Siedlerchr
Copy link
Member

From the test output:

LcalizationConsistencyTest > findObsoleteLocalizationKeys() FAILED
org.opentest4j.AssertionFailedError: Obsolete keys found in language properties file:

Listen for remote operation on port
This feature lets new files be opened or imported into an already running instance of JabRef instead of opening a new instance. For instance, this is useful when you open a file in JabRef from your web browser. Note that this will prevent you from running more than one instance of JabRef at a time.
1. CHECK IF THE KEY IS REALLY NOT USED ANYMORE
2. REMOVE THESE FROM THE ENGLISH LANGUAGE FILE 

@DiamondMyK
Copy link
Contributor Author

From the test output:

LcalizationConsistencyTest > findObsoleteLocalizationKeys() FAILED
org.opentest4j.AssertionFailedError: Obsolete keys found in language properties file:

Listen for remote operation on port
This feature lets new files be opened or imported into an already running instance of JabRef instead of opening a new instance. For instance, this is useful when you open a file in JabRef from your web browser. Note that this will prevent you from running more than one instance of JabRef at a time.
1. CHECK IF THE KEY IS REALLY NOT USED ANYMORE
2. REMOVE THESE FROM THE ENGLISH LANGUAGE FILE 

Unfortunately, the test failed, I will continue to improve.

@DiamondMyK
Copy link
Contributor Author

From the test output:

LcalizationConsistencyTest > findObsoleteLocalizationKeys() FAILED
org.opentest4j.AssertionFailedError: Obsolete keys found in language properties file:

Listen for remote operation on port
This feature lets new files be opened or imported into an already running instance of JabRef instead of opening a new instance. For instance, this is useful when you open a file in JabRef from your web browser. Note that this will prevent you from running more than one instance of JabRef at a time.
1. CHECK IF THE KEY IS REALLY NOT USED ANYMORE
2. REMOVE THESE FROM THE ENGLISH LANGUAGE FILE 

Thank you very much for your help! It passed the test. May I ask whether this modification result meets the expectation of issue?

@Siedlerchr
Copy link
Member

now all is green

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 29, 2023
@DiamondMyK
Copy link
Contributor Author

now all is green

Yes, we do! Thank you very much! :)

@DiamondMyK
Copy link
Contributor Author

DiamondMyK commented Oct 30, 2023

Thank you for your advice. I have made the requested changes to the PR and performed a self-check to ensure that other issues have been fixed. Now, I look forward to your reevaluation of my PR. Given that my homework is due within the next day, I kindly request your prompt review. Your timely attention to this matter would be greatly appreciated, as it will help me meet the deadline for my homework. If everything meets your expectations, could you please consider merging it? Thank you very much for your time and assistance, and I eagerly await your feedback.:)

@Siedlerchr
Copy link
Member

@DiamondMyK Tested it and just pushed one last commit, you only need one action factory. Creating a new object of ActionFactory just sets the KeybindingRepository so one instance can be reused

@Siedlerchr Siedlerchr added this pull request to the merge queue Oct 30, 2023
Merged via the queue into JabRef:main with commit 1945a5a Oct 30, 2023
17 checks passed
@@ -16,6 +16,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv

### Changed

- We move the location of the 'Open only one instance of JabRef' preference option from "Network" to "General" .[#9306](https://github.com/JabRef/jabref/issues/9306)
Copy link
Member

Choose a reason for hiding this comment

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

I fixed grammar and spaces in dc2b649.

Siedlerchr added a commit to xuanan20020/jabref that referenced this pull request Nov 1, 2023
* upstream/main:
  Fix CHANGELOG.md
  Bump commons-cli:commons-cli from 1.5.0 to 1.6.0 (JabRef#10607)
  Fix issue JabRef#9306: Move "Open only one instance of JabRef" preference option to somewhere else (JabRef#10602)
  Update README.md (JabRef#10604)
  Bump me.champeau.jmh from 0.7.1 to 0.7.2
  Bump org.beryx.jlink from 3.0.0 to 3.0.1
  Bump com.dlsc.gemsfx:gemsfx from 1.82.0 to 1.84.0
  Bump org.apache.logging.log4j:log4j-to-slf4j from 2.21.0 to 2.21.1

# Conflicts:
#	src/main/resources/l10n/JabRef_en.properties
Siedlerchr added a commit to guipmenezes/jabref that referenced this pull request Nov 1, 2023
* upstream/main: (419 commits)
  Fix CHANGELOG.md
  Bump commons-cli:commons-cli from 1.5.0 to 1.6.0 (JabRef#10607)
  Fix issue JabRef#9306: Move "Open only one instance of JabRef" preference option to somewhere else (JabRef#10602)
  Update README.md (JabRef#10604)
  Bump me.champeau.jmh from 0.7.1 to 0.7.2
  Bump org.beryx.jlink from 3.0.0 to 3.0.1
  Bump com.dlsc.gemsfx:gemsfx from 1.82.0 to 1.84.0
  Bump org.apache.logging.log4j:log4j-to-slf4j from 2.21.0 to 2.21.1
  Synchronize scrollbars in the change resolver dialog (JabRef#10587)
  Add button for a user to reset the cite command to the default value. (JabRef#10580)
  openrerwrite
  Update .github/PULL_REQUEST_TEMPLATE.md
  Update PULL_REQUEST_TEMPLATE.md
  Update .github/PULL_REQUEST_TEMPLATE.md
  Update PULL_REQUEST_TEMPLATE.md
  Replace "fixes" by "resolves"
  Change JavaDoc to annotation (JabRef#10571)
  Fix link (JabRef#10575)
  Enable collecting GitHub build artifacts for forks (JabRef#10574)
  Fix file field merging (JabRef#10573)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/fieldeditors/identifier/IdentifierEditor.java
#	src/main/java/org/jabref/logic/importer/WebFetchers.java
Siedlerchr added a commit that referenced this pull request Nov 4, 2023
* upstream/main: (1565 commits)
  Enable journal information fetcher directly in popup (#10598)
  Make generate button wider (#10588)
  make openRewrite stable again
  Rename cleanup_pr.yml to cleanup-pr.yml
  Changelog
  Fix failing fetcher tests (#10613)
  ShortDoi
  Fix CHANGELOG.md
  Bump commons-cli:commons-cli from 1.5.0 to 1.6.0 (#10607)
  Fix issue #9306: Move "Open only one instance of JabRef" preference option to somewhere else (#10602)
  Update README.md (#10604)
  Bump me.champeau.jmh from 0.7.1 to 0.7.2
  Bump org.beryx.jlink from 3.0.0 to 3.0.1
  Bump com.dlsc.gemsfx:gemsfx from 1.82.0 to 1.84.0
  Bump org.apache.logging.log4j:log4j-to-slf4j from 2.21.0 to 2.21.1
  Synchronize scrollbars in the change resolver dialog (#10587)
  Add button for a user to reset the cite command to the default value. (#10580)
  openrerwrite
  Update .github/PULL_REQUEST_TEMPLATE.md
  Update PULL_REQUEST_TEMPLATE.md
  ...

# Conflicts:
#	.devcontainer/devcontainer.json
#	build.gradle
#	src/main/java/org/jabref/gui/DefaultInjector.java
#	src/main/java/org/jabref/gui/libraryproperties/general/GeneralProperties.fxml
#	src/main/java/org/jabref/gui/libraryproperties/general/GeneralPropertiesView.java
#	src/main/java/org/jabref/gui/libraryproperties/general/GeneralPropertiesViewModel.java
#	src/main/java/org/jabref/logic/exporter/MetaDataSerializer.java
#	src/main/java/org/jabref/logic/importer/util/MetaDataParser.java
#	src/main/java/org/jabref/model/metadata/MetaData.java
#	src/main/java/org/jabref/preferences/JabRefPreferences.java
Siedlerchr added a commit to liyou969/jabref that referenced this pull request Nov 4, 2023
* upstream/main: (44 commits)
  change codecov (JabRef#10616)
  Add entry based on ISSN number JabRef#10124 (JabRef#10178)
  Enable journal information fetcher directly in popup (JabRef#10598)
  Make generate button wider (JabRef#10588)
  make openRewrite stable again
  Rename cleanup_pr.yml to cleanup-pr.yml
  Changelog
  Fix failing fetcher tests (JabRef#10613)
  ShortDoi
  Fix CHANGELOG.md
  Bump commons-cli:commons-cli from 1.5.0 to 1.6.0 (JabRef#10607)
  Fix issue JabRef#9306: Move "Open only one instance of JabRef" preference option to somewhere else (JabRef#10602)
  Update README.md (JabRef#10604)
  Bump me.champeau.jmh from 0.7.1 to 0.7.2
  Bump org.beryx.jlink from 3.0.0 to 3.0.1
  Bump com.dlsc.gemsfx:gemsfx from 1.82.0 to 1.84.0
  Bump org.apache.logging.log4j:log4j-to-slf4j from 2.21.0 to 2.21.1
  Synchronize scrollbars in the change resolver dialog (JabRef#10587)
  Add button for a user to reset the cite command to the default value. (JabRef#10580)
  openrerwrite
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move "Open only one instance of JabRef" preference option to somewhere else
4 participants