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 "database" to "catalog" #9951

Closed
wants to merge 3 commits into from
Closed

Conversation

wy8881
Copy link
Contributor

@wy8881 wy8881 commented May 27, 2023

Fixes JabRef#615

Rename "database" to "catalog" for ul and I10n
1685187003556

Change the corresponding translation for some languages

1685187109363

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.

@wy8881 wy8881 changed the title Fix for issue 615 Change "database" to "catalog" May 27, 2023
@Siedlerchr
Copy link
Member

Hi you only need to edit the English translations, the rest is done via crowdin https://devdocs.jabref.org/code-howtos/localization.html

@ThiloteE ThiloteE added the SLR label May 27, 2023
@wy8881
Copy link
Contributor Author

wy8881 commented May 27, 2023

Hi you only need to edit the English translations, the rest is done via crowdin https://devdocs.jabref.org/code-howtos/localization.html

Hello, thanks for your advice. I checked this website and I am confused. It seems like I need to use the function Localization.lang() when there needs to be translated, but I can't find this function be used in ManageStudyDefinition.fxml.
I'm not sure whether it's fine just add a key-value pair in JabRef_en.properties and I'll try to run the test again. Sorry for not checking related documents in advance. I'm totally new to making contributions to an open-source project.

@Siedlerchr
Copy link
Member

Thanks, we will add a section on fxml, in the fxml files it's simply a % sign in front of the string

@wy8881 wy8881 marked this pull request as ready for review May 27, 2023 13:07
@wy8881
Copy link
Contributor Author

wy8881 commented May 27, 2023

Thanks for explaining it, I googled the sign % before and found nothing.

Siedlerchr
Siedlerchr previously approved these changes May 27, 2023
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 27, 2023
@calixtus
Copy link
Member

calixtus commented May 28, 2023

Hi, thanks for your interest in JabRef. Please note, that as long as you are not a native speaker, please remove all the language changes and leave it to crowdin to translate!

Also I think the maintainers should discuss such ui changes at least in the devcall. This is maybe some fundamental discussion, if every issue in the jabref or koppor repo including an ui change is already meant to be implemented without being talked about. In this case, as this is only a very small change in the SLR subsystem, it's probably not a big deal. But nevertheless it's something to be talked about.

@calixtus calixtus added status: changes required Pull requests that are not yet complete status: devcall ui labels May 28, 2023
src/main/resources/l10n/JabRef_ar.properties Outdated Show resolved Hide resolved
src/main/resources/l10n/JabRef_de.properties Outdated Show resolved Hide resolved
src/main/resources/l10n/JabRef_fr.properties Show resolved Hide resolved
src/main/resources/l10n/JabRef_ko.properties Show resolved Hide resolved
@calixtus calixtus removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 28, 2023
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

The PR is a good start. The intention, however, was to go through all the code in the slr code.

Example:

ManageStudyDefinitionView.java

I double checked the issue description - and it was also contained there. I think, you implemetned the first part. Please also implement the second part:

image

@@ -231,7 +231,7 @@
resizable="false"/>
<TableColumn
fx:id="databaseColumn"
Copy link
Member

Choose a reason for hiding this comment

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

This variable also needs to renamed

<ScrollPane
fitToWidth="true"
fitToHeight="true"
styleClass="slr-tab">
<VBox spacing="20.0">
<Label text="%Select Databases:"/>
<Label text="%Select Catalogs:"/>
<HBox alignment="CENTER_LEFT">
<TableView
fx:id="databaseTable"
Copy link
Member

Choose a reason for hiding this comment

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

This variable also needs to be renamed

@wy8881
Copy link
Contributor Author

wy8881 commented Jun 6, 2023

Hi, sorry for that. But I don't sure what is "in SLR code". Is that mean I also change the name of related function?

@koppor
Copy link
Member

koppor commented Jun 6, 2023

I try to unzip the following comment:

image

The "secret" is to search for "database" in the code:

image

Go through all classes in package org.jabref.gui.slr:

image

Finally, you need to modify all classes in package org.jabref.logic.crawler:

image

This excercise will teach you how to refactor code in a learning-by-doing way.

@wy8881
Copy link
Contributor Author

wy8881 commented Jun 6, 2023

Thanks for explanation, I will go and check those code step by step

@koppor
Copy link
Member

koppor commented Jun 8, 2023

The code changes were done in #9989. I think/hope, they are done all.

Siedlerchr pushed a commit that referenced this pull request Jun 8, 2023
…c.crawler (#9989)

* Fix for issue 615

* Add missing key pair English file

* Delete translation in non-english file

* We changed database to catalog in org.jabref.gui.slr and org.jabref.logic.crawler (Issue #9951)

* Made required changes

---------

Co-authored-by: wy8881 <wy7382@gmail.com>
Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>
@Siedlerchr Siedlerchr closed this Jun 8, 2023
@koppor
Copy link
Member

koppor commented Jun 8, 2023

The code changes were integerated in #9989 and merged in main. Thank you for your work and timely reaction on the comments!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SLR status: changes required Pull requests that are not yet complete ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SLR: Rename "database" to "catalog"
5 participants