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: make fields sorted by lexicographical order #7711

Merged
merged 13 commits into from
May 8, 2021
Merged

fix: make fields sorted by lexicographical order #7711

merged 13 commits into from
May 8, 2021

Conversation

dateri
Copy link
Contributor

@dateri dateri commented May 7, 2021

Fixes #7710

We fix that some fields are in random order (issue #7710). We use Treemap which is ordered to replace LinkedHashMap and added a comparison function to the treemap to ensure that fields are ordered by lexicographical order.

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

@Siedlerchr
Copy link
Member

Normally, this would be a suitable way. However, this is a special case as you can reorder the fields in the custom entry types dialog. Refs #6152
The TreeSet will now kill any individual order. What we only need is a sorting in the dropdown box. So you need to add a sorting in the GUI/ViewModel

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.

Do not change the order in FieldFactory
Edit:: Please change the PR title to a more meaningful name!

@dateri dateri changed the title fix 7710 fix make fields sorted by lexicographical order May 7, 2021
@dateri dateri changed the title fix make fields sorted by lexicographical order fix: make fields sorted by lexicographical order May 7, 2021
@dateri
Copy link
Contributor Author

dateri commented May 7, 2021

Thanks for guidance, I found they both use src/main/java/org/jabref/gui/commonfxcontrols/FieldFormatterCleanupsPanelViewModel.java, so I change it in the cleanupsPanel.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 7, 2021
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.

Yep, that's now better.
Just two minor improvements

CHANGELOG.md Outdated
@@ -550,6 +550,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue where the XMP Importer would incorrectly return an empty default entry when importing pdfs [#6577](https://github.com/JabRef/jabref/issues/6577)
- We fixed an issue where opening the menu 'Library properties' marked the library as modified [#6451](https://github.com/JabRef/jabref/issues/6451)
- We fixed an issue when importing resulted in an exception [#7343](https://github.com/JabRef/jabref/issues/7343)
- We fixed an issue Some fields are in random order [#7710](https://github.com/JabRef/jabref/issues/7710)
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 fixed an issue Some fields are in random order [#7710](https://github.com/JabRef/jabref/issues/7710)
- We fixed an issue where the field in the Field formatter dropdown selection were sorted in random order [#7710](https://github.com/JabRef/jabref/issues/7710)

@@ -28,6 +28,12 @@
private final ObjectProperty<Formatter> selectedFormatterProperty = new SimpleObjectProperty<>();

public FieldFormatterCleanupsPanelViewModel() {
availableFieldsProperty.sort((field1, field2) -> {
if (field1.getDisplayName().equals(field2.getDisplayName())) {
Copy link
Member

Choose a reason for hiding this comment

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

The compareTo already performs equal checks...
So you should be able to directly use sort(field1, field2) -> field1.getDisplayName().compareTo(field2.getDisplayName))

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Comparator.comparing should work as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you said really makes sense and I have modified them.

Copy link
Sponsor Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment

Choose a reason for hiding this comment

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

Looks good to me! One needed change and one nitpick.

(nitpick = I will approve even if you don't fix it, it is a suggestion and address it if you like it)

@@ -17,6 +17,8 @@
import org.jabref.model.entry.field.Field;
import org.jabref.model.entry.field.FieldFactory;

import java.util.Comparator;
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Please fix the reviewdog complaint

Copy link
Sponsor Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment

Choose a reason for hiding this comment

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

LTGM!

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.

Yeah, that is even a better solution!

@Siedlerchr Siedlerchr merged commit 4f398d8 into JabRef:main May 8, 2021
Siedlerchr added a commit that referenced this pull request May 15, 2021
* upstream/main: (71 commits)
  [Bot] Update CSL styles (#7735)
  Fix for issue 6966: open all files of multiple entries (#7709)
  Add simple unit tests (#7696)
  Add simple unit tests (#7543)
  Update check-outdated-dependencies.yml
  Added preset for new entry keybindings and reintroduced defaults (#7705)
  Select the entry which has smaller dictonary order when merge (#7708)
  Update CHANGELOG.md
  fix: make more fields, fomatters, ids and languages sorted by alphabetical order (#7717)
  Bump libreoffice from 7.1.2 to 7.1.3 (#7721)
  Bump unoloader from 7.1.2 to 7.1.3 (#7724)
  Bump org.beryx.jlink from 2.23.7 to 2.23.8 (#7723)
  Bump org.openjfx.javafxplugin from 0.0.9 to 0.0.10 (#7725)
  fix: make fields sorted by lexicographical order (#7711)
  Fix tests
  Refactoring existing unit tests (#7687)
  Refactoring and addition of unit tests (#7581)
  Refactor simple Unit Tests (#7571)
  Add simple unit tests (#7544)
  add and extend unit tests (#7685)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup action: some fields in random order
3 participants