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

Change instances of 'Search Selected' to 'Search Pre-configured' #11871

Merged
merged 5 commits into from
Oct 1, 2024

Conversation

plvzfq-rit
Copy link
Contributor

@plvzfq-rit plvzfq-rit commented Oct 1, 2024

Closes #11866

image

Instances of 'Search Selected' has been changed to 'Search Pre-configured' in the following files:

  • CompositeSearchBasedFetcher.java in line 21
  • JabRef_en.properties in line 128
  • WebFetchers.java in line 241
  • WebSearchTab.fxml in line 30

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: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Instances of 'Search Selected' has been changed to 'Search Pre-configured' in the following files:
- CompositeSearchBasedFetcher.java in line 21
- JabRef_en.properties in line 128
- WebFetchers.java in line 241
- WebSearchTab.fxml in line 30
Update CHANGELOG.md to reflect Web Search Preferences UI changes from 'Search Selected' to 'Search Pre-configured'
Siedlerchr
Siedlerchr previously approved these changes Oct 1, 2024
@Siedlerchr
Copy link
Member

can you lowercase "pre-configured" please?

@plvzfq-rit
Copy link
Contributor Author

Sure+ Though I'm worried I might have broken something (as shown in the tests)

@plvzfq-rit
Copy link
Contributor Author

Perhaps something might have been broken since I changed the "Search Selected" to "Select pre-configured" in the FETCHER_NAME field in CompositeSearchBasedFetcher.java?

@plvzfq-rit
Copy link
Contributor Author

For now, I think I'll try reverting the Fetcher logic back to what it was before so that it still runs.

Revert 'Search pre-configured' back to 'Search Selected' to stop errors
Revert 'Search Selected' back to 'Search pre-configured' since not doing so does not update dropdown menu
@plvzfq-rit
Copy link
Contributor Author

I had reverted the name changes back since not doing so made the dropdown menu display 'Search Selected' instead of 'Search pre-configured.' I do apologize for the hullabaloo;;; Do let me know how to deal with the failed tests though; I am not quite sure how to deal with them and it might affect this branch's ability to merge with the main branch.

@plvzfq-rit plvzfq-rit marked this pull request as ready for review October 1, 2024 14:13
@@ -18,7 +18,7 @@

public class CompositeSearchBasedFetcher implements SearchBasedFetcher {

public static final String FETCHER_NAME = "Search pre-configured";
Copy link
Member

Choose a reason for hiding this comment

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

Why do you revert 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.

Not doing so made the dropdown list display "Search Selected" (the old value) instead of "Search pre-configured"

The dropdown in question (from the latest revert):
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though if it was with the revert before that, I was trying to look into the errors that showed up. Though I guess they were quite moot now that I know they're optional

@Siedlerchr
Copy link
Member

If you click on Details on the right and check the failures, you will see that the failures are not related to your changes
In addition, Fetcher tests are marked as optional (they depend on external sources and can fail from time to time)
grafik

@koppor
Copy link
Member

koppor commented Oct 1, 2024

Sure+ Though I'm worried I might have broken something (as shown in the tests)

The fetcher tests run as soon something is changed in the fetchers. Since they are remote services, something might fail - not because of us, but because of the network or the external server. You experience that now.

- 'src/main/java/org/jabref/logic/importer/fetcher/**'

We execute fetcher tests with the gradle task "fetcherTest"

run: ./gradlew fetcherTest

As follow-up, maybe, you could write some text to https://devdocs.jabref.org/code-howtos/fetchers.html - and link the new heading from https://devdocs.jabref.org/code-howtos/testing.html? Some lines are OK.

@plvzfq-rit
Copy link
Contributor Author

Some changes to those devdocs then with regard to fetcherTests? Alright, I'll see what I could do+

@Siedlerchr Siedlerchr added this pull request to the merge queue Oct 1, 2024
Merged via the queue into JabRef:main with commit 3fe4852 Oct 1, 2024
23 of 24 checks passed
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.

Rename "Search selected" to "Search pre-configured"
3 participants