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.

Sign up for GitHub

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

Jump to bottom

fix: make more fields, fomatters, ids and languages sorted by alphabetical order #7717

Merged
merged 17 commits into from
May 10, 2021
Merged

fix: make more fields, fomatters, ids and languages sorted by alphabetical order #7717

merged 17 commits into from
May 10, 2021

Conversation

dateri
Copy link
Contributor

@dateri dateri commented May 8, 2021

Fixes #7716
We fix that more fields, fomatters, ids and languages sorted by alphabetical order. Solving them by either adding sort to relative property or using a SortedList to wrapper the ObservableList.

  • 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
Siedlerchr reviewed May 8, 2021
View reviewed changes
src/main/java/org/jabref/gui/EntryTypeViewModel.java Outdated
@@ -60,6 +61,7 @@ public EntryTypeViewModel(PreferencesService preferences,
this.dialogService = dialogService;
this.stateManager = stateManager;
fetchers.addAll(WebFetchers.getIdBasedFetchers(preferences.getImportFormatPreferences()));
fetchers.sort(Comparator.comparing(IdBasedFetcher::getName));
Copy link
Member

Choose a reason for hiding this comment

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

As I said in a comment I don't think it's useful to sort by alphatical order here. DOI fetcher should always be on top because it's the most important one

Copy link
Member

Choose a reason for hiding this comment

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

So please revert that

Siedlerchr
Siedlerchr requested changes May 8, 2021
View reviewed changes
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.

Some minor improvements

src/main/java/org/jabref/gui/preferences/file/FileTabViewModel.java Outdated
@@ -84,6 +85,10 @@ public void setValues() {
secondarySortFieldsProperty.addAll(fieldNames);
tertiarySortFieldsProperty.addAll(fieldNames);

primarySortFieldsProperty.sort(Comparator.comparing(Field::getDisplayName));
Copy link
Member

Choose a reason for hiding this comment

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

Simple solution but not effective. You are essentially doing the same sort 3 times.
I would create: List<Field> fieldNames = new ArrayList<>(FieldFactory.getCommonFields())
Then sort it and add it to the properties

@Siedlerchr
Copy link
Member

Thanks for the quick follow up, lgtm to me now. For external contributors a second developer needs to review and approve

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 8, 2021
@koppor koppor merged commit ab74598 into JabRef:main May 10, 2021
@koppor
Copy link
Member

koppor commented May 10, 2021

I merged too early. Forgot the check for CHANGELOG.md. Fixed in 4282394.

@dateri Please remember the CHANGELOG.md for your next PR 🎉

@mlep
Copy link
Contributor

mlep commented May 11, 2021

@koppor I wanted to check on this, but in https://builds.jabref.org/main/, the files are dating back to May 8. A problem with the automatic build process?

@Siedlerchr
Copy link
Member

Happens from time to time, most often the mac build fails due to notarization somehow not working at apple's side.
you can then downlod the artifact from the run
https://github.com/JabRef/jabref/actions/runs/829332567

Siedlerchr added a commit that referenced this pull request May 15, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into writebackupToTemp
dfee77f 
* 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
Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@koppor koppor koppor approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some more fields in random order
4 participants
@dateri @Siedlerchr @koppor @mlep