Skip to content

Change export to save#7518

Merged
Siedlerchr merged 4 commits into
JabRef:mainfrom
yinpeiqi:fix-for-issue-7517
Apr 5, 2021
Merged

Change export to save#7518
Siedlerchr merged 4 commits into
JabRef:mainfrom
yinpeiqi:fix-for-issue-7517

Conversation

@yinpeiqi

@yinpeiqi yinpeiqi commented Mar 12, 2021 •
edited by Siedlerchr
Loading

Copy link
Copy Markdown
Contributor

fix for 7517, change Export to Save
Fixes #7517

  • 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.

@mlep

mlep commented Mar 12, 2021

Copy link
Copy Markdown
Contributor

This is one of the fastest Issue --> PR I have seen!
Looking at your changes, I think the section title "Export sort order" needs to be modified too (and maybe twice: once for the preferences window, and once for the library window).

@yinpeiqi

yinpeiqi commented Mar 12, 2021

Copy link
Copy Markdown
Contributor Author

Okay I changed it too.

@Siedlerchr

Siedlerchr commented Mar 12, 2021

Copy link
Copy Markdown
Member

You probably need to adjust the localization in the l10n properties file https://devdocs.jabref.org/getting-into-the-code/code-howtos#using-localization-correctly

@yinpeiqi

yinpeiqi commented Mar 12, 2021

Copy link
Copy Markdown
Contributor Author

Only delete the unused part of l10n is enough, am I right?

@Siedlerchr Siedlerchr closed this Mar 13, 2021
@Siedlerchr Siedlerchr reopened this Mar 13, 2021
@Siedlerchr Siedlerchr changed the title Fix for issue 7517 Change export to save Mar 14, 2021
@calixtus

calixtus commented Mar 15, 2021

Copy link
Copy Markdown
Member

Please check your github configuration. The checks all stall and without checks it's way harder for us to test changes made to the codebase!

@koppor

koppor commented Mar 29, 2021

Copy link
Copy Markdown
Member

Please keep the distinction of "Export" and "Save". Please change to "neutral" wording to enable use of a common FX control:

grafik

(See also #7517 (comment))

@Siedlerchr

Siedlerchr commented Apr 3, 2021

Copy link
Copy Markdown
Member

@yinpeiqi It would be really nice if you could implement the changes as outlined in the screenshots

@yinpeiqi

yinpeiqi commented Apr 4, 2021

Copy link
Copy Markdown
Contributor Author

Is this change ok?

calixtus
calixtus reviewed Apr 4, 2021
View reviewed changes
Comment thread src/main/java/org/jabref/gui/commonfxcontrols/SaveOrderConfigPanel.java Outdated
Comment on lines 73 to 77
public void changeExportDescriptionToSave() {
exportInOriginalOrder.setText(Localization.lang("Save entries in their original order"));
exportInSpecifiedOrder.setText(Localization.lang("Save entries ordered as specified"));
exportInTableOrder.setText(Localization.lang("Save in current table sort order"));
exportInOriginalOrder.setText(Localization.lang("Keep original order"));
exportInSpecifiedOrder.setText(Localization.lang("Use specified order"));
exportInTableOrder.setText(Localization.lang("Use current table sort order"));
}

@calixtus calixtus Apr 4, 2021

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.

This method and the call to can be removed entirely.

@Siedlerchr Siedlerchr Apr 4, 2021

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.

Good catch! Otherwise it's good from my side,tested it locally

@yinpeiqi

yinpeiqi commented Apr 5, 2021

Copy link
Copy Markdown
Contributor Author

Okay, I remove it.

@calixtus

calixtus commented Apr 5, 2021

Copy link
Copy Markdown
Member

There is still a unused import, breaking a check style rule.

@yinpeiqi

yinpeiqi commented Apr 5, 2021

Copy link
Copy Markdown
Contributor Author

Well, this time I remove it.

calixtus
calixtus approved these changes Apr 5, 2021
View reviewed changes

@calixtus calixtus left a comment

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.

Looks good to me! Thanks.

@yinpeiqi

yinpeiqi commented Apr 5, 2021

Copy link
Copy Markdown
Contributor Author

You are welcome.

@Siedlerchr Siedlerchr merged commit 6837164 into JabRef:main Apr 5, 2021
@Siedlerchr

Siedlerchr commented Apr 5, 2021

Copy link
Copy Markdown
Member

Thanks again for your contribution!

Siedlerchr added a commit that referenced this pull request Apr 11, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into writebackupToTemp
5b2b64c 
* upstream/main:
  Main instead of master
  Custom DOI base address fix (#7569)
  Change export to save (#7518)
  Bump unoloader from 7.1.1 to 7.1.2 (#7609)
  Bump org.beryx.jlink from 2.23.5 to 2.23.6 (#7610)
  Bump com.adarshr.test-logger from 2.1.1 to 3.0.0 (#7611)
  Bump libreoffice from 7.1.1 to 7.1.2 (#7612)
  Squashed 'buildres/csl/csl-styles/' changes from e1acabe..bfa3b6d (#7603)
  Rename master to main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@calixtus calixtus calixtus approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

UI: "Export order" --> "Save order"

5 participants

@yinpeiqi @mlep @Siedlerchr @calixtus @koppor