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

Persistent column sortorder #5730

Merged
merged 10 commits into from
Dec 16, 2019
Merged

Persistent column sortorder #5730

merged 10 commits into from
Dec 16, 2019

Conversation

calixtus
Copy link
Member

@calixtus calixtus commented Dec 10, 2019 •
edited
Loading

This PR should make the column sort order persistent. @Siedlerchr asked me to give it a shot, and to see if it's somehow quicker. I tried to stay close to the JavaFX implementation of the the column sort order and somewhat close to the mvvm pattern. I don't think it has much impact on the performance, compared to the current master, but i'm not equipped with a profiler to test it. So it's just my subjective opinion. 😁 Maybe someone else likes to test it?

fixes #4800 fixes #4224
refs #5713 and #5368
follow-up to #4327 and #5544

The next step would be to implement sortorder by library, which would allow some more exotic use cases for jabref. I remember someone had problems with the sort order when using jabref as a library for some genetics stuff, but i wasn't able to find it in the issues...

So, let's talk about this.

Screenshot is omitted, as there is nothing to see here.

Siedlerchr
Siedlerchr reviewed Dec 10, 2019
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.

Thanks, looks good so far. Have to test it later

src/main/java/org/jabref/migrations/PreferencesMigrations.java Outdated Show resolved Hide resolved
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 11, 2019
Siedlerchr
Siedlerchr requested changes Dec 11, 2019
View reviewed changes
src/main/java/org/jabref/gui/maintable/MainTableColumnModel.java Outdated
@@ -35,6 +38,8 @@
NORMALFIELD("field"),
SPECIALFIELD("special", Localization.lang("Special"));

public static List<Type> ICON_COLUMNS = Arrays.asList(EXTRAFILE,FILES,GROUPS,LINKED_IDENTIFIER);
Copy link
Member

Choose a reason for hiding this comment

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

Why not an EnumSet? EnumSet.allOf() https://www.baeldung.com/java-enumset

Copy link
Member Author

@calixtus calixtus Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm just using this as a constant (i know, I forgot 'final' here) to spare the subsequent checks on each single column type, if it is an icon column. I implemented this in the first time as an additional argument of the single Types constructor (boolean isIconColumn), but it would have brought way more complexity. So this way someone later, who wants to add another column type can simply add it in this array (if needed). It does not really make a difference if this is an Array or an EnumSet.

@Siedlerchr
Copy link
Member

I tried your branch and for me it doesn't work. I changed sort order for year to descending but after a restart it's still ascending

@calixtus
Copy link
Member Author

That's somehow odd... Just tried it again and it worked for me...
Can you try to reset the preferences, restart JabRef and test it again?

A shortcut for quicker testing: After restarting JabRef the mechanics of column sorting should be also the same in restarting JabRef as in opening and saving the prefs dialog.

@Siedlerchr
Copy link
Member

Yes, works fine after resetting the preferences 👍
Codewise also looks good. I let have @tobiasdiez the final say on this ;)

@calixtus
Copy link
Member Author

Great. Did you notice any performance drops?

@Siedlerchr
Copy link
Member

So far not, Sorting alone in a huge database with over 6,400 entries and many groups takes a long time so this is definitely not related to the storage of the order.

@Siedlerchr Siedlerchr mentioned this pull request Dec 14, 2019
Closed
@Siedlerchr
Copy link
Member

I am so free to merge this. ;)

@Siedlerchr Siedlerchr merged commit b2c4061 into JabRef:master Dec 16, 2019
@bernhard-kleine bernhard-kleine mentioned this pull request Dec 16, 2019
Closed
@calixtus calixtus deleted the persistent_column_sortorder branch December 16, 2019 18:21
Siedlerchr added a commit that referenced this pull request Dec 17, 2019
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into change-upload-bac…
9061f37 
…k-to-script

* upstream/master:
  Bump mockito-core from 3.2.0 to 3.2.4 (#5760)
  Bump classgraph from 4.8.58 to 4.8.59 (#5761)
  Improve dependency update rules
  Update jpackage to build 27 (#5758)
  Persistent column sortorder (#5730)
  Fix medline fetcher/importer when using installer (#5752)
  New Crowdin translations (#5751)
  Bump byte-buddy-parent from 1.10.4 to 1.10.5 (#5750)
  Fix checkstyle
  Fix filename
Siedlerchr added a commit that referenced this pull request Dec 20, 2019
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into changemergesdialog
ab48a98 
# By Tobias Diez (11) and others
# Via GitHub (1) and Tobias Diez (1)
* upstream/master: (29 commits)
  Improve things arround change detection (#5770)
  Revert "Update to most recent journal abbreviation list" (#5769)
  Various fixes to the dark theme (#5764)
  Bump mockito-core from 3.2.0 to 3.2.4 (#5760)
  Bump classgraph from 4.8.58 to 4.8.59 (#5761)
  Improve dependency update rules
  Update jpackage to build 27 (#5758)
  Persistent column sortorder (#5730)
  Fix medline fetcher/importer when using installer (#5752)
  New Crowdin translations (#5751)
  Bump byte-buddy-parent from 1.10.4 to 1.10.5 (#5750)
  Fix checkstyle
  Fix filename
  Update to most recent journal abbreviation list
  Remove obsolete string
  Revert "Switch back to development"
  Switch back to development
  Next development cycle
  Release 5.0-beta (#5684)
  Fix multiple entries allowed in crossref (issue #5284) (#5724)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java
#	src/main/java/org/jabref/gui/collab/EntryChangeViewModel.java
@AEgit AEgit mentioned this pull request Jan 30, 2020
Closed
@koppor koppor mentioned this pull request Apr 15, 2020
Closed
koppor pushed a commit that referenced this pull request Dec 1, 2021
Squashed 'buildres/csl/csl-styles/' changes from 3a6a0a7..3bb4b5f
6328352 
3bb4b5f infoclio.ch styles for German: remove non-breaking space delimiters (#5754)
adf28db Create journal-of-health-care-for-the-poor-and-underserved.csl (#5752)
0713a8e Update chinese-gb7714-2005-numeric.csl (#5737)
1cd3754 Update china-national-standard-gb-t-7714-2015-author-date.csl (#5746)
c2536b7 Update china-national-standard-gb-t-7714-2015-numeric.csl (#5745)
f8c1392  Create steel-research-international.csl (#5720)
21fe1f5 Create asian-myrmecology.csl (#5718)
91e9e2b Update harvard-university-of-the-west-of-england.csl (#5734)
dd453d1 fix minor erros in polar-research.csl (#5730)
038a8f5 Remove group around no-date cluster (#5731)
0710b51 remove et-al from bibtex.csl (#5728)
bbd703d Add editorial-director to universite-laval-departement-des-sciences-historiques.csl (#5727)
58ea430 Create german-journal-of-agricultural-economics.csl (#5717)
0654e16 Create scandinavian-journal-of-information-systems.csl (#5716)
ce2d537 Update journal-of-computer-applications-in-archaeology.csl (#5715)
755d3d3 Create human-rights-law-review.csl (#5626)
0feda94 Create journal-of-intercultural-studies.csl (#5709)
ae4756d Update acta-universitatis-agriculturae-sueciae.csl (#5713)
323d9ac Update mohr-siebeck-recht.csl (#5559)
15530a8 Bch corr (#5712)
094a1af Create forschungsjournal-soziale-bewegungen-fjsb.csl (#5699)
cb91566 initialize authors and editors (#5714)
2d5cfff Create cancer-biomarkers.csl (#5703)
5e264d5 Update multidisciplinary-digital-publishing-institute.csl (#5708)
46e961f Create klinische-padiatrie.csl (#5711)
e81e877 Create bulletin-archeologique-des-ecoles-francaises-a-l-etranger.csl (#5704)
0029c5a Create polar-research.csl 🧊 (#5702)
7db1361 Update vancouver-imperial-college-london.csl (#5641)
b953e9f Update iso690-author-date-fr-no-abstract.csl (#5706)
91eda8c Update thieme-german.csl (#5710)
ebe0787 Update harvard-imperial-college-london.csl (#5643)
2d4db76 Fix UNESCO IIEP in text
436cbf4 Create revue-archeologique-de-narbonnaise.csl (#5688)
5150bcf Create journal-of-computer-assisted-tomography.csl (#5690)
dd6f050 Create anti-trafficking-review.csl (#5658)
08e622f Create the-angle-orthodontist.csl (#5685)
c6a1907 journal-of-palm-oil-research.csl fix several errors (#5686)
6cbe29d Create bern-university-of-applied-sciences-school-of-agricultural-for… (#5684)
f590dc1 Update biomed-central.csl (#5701)
1efce81 Update turabian-author-date.csl (#5695)
12dbba5 Create tyndale-bulletin (#5673)
b0746db Create Engineered Regeneration (#5682)
e38b953 wikipedia citation template (#5662)
5e7f731 Create early-music-history.csl (#5679)
86443f3 Create zeitschrift-fur-politik.csl (#5676)
68f1996 Create annals-of-work-exposures-and-health.csl (#5666)
1ba9dc6 Create brazilian-journal-of-psychiatry.csl (#5672)
438f92c fix error for speech in ama styles (#5693)
7a0c2d3 set initialize-with-hyphen to false (#5689)
3bd2765 Update emu-austral-ornithology.csl (#5671)
31492b2 fix various errors in natura-croatica.csl (#5687)
94d6b23 Update iso690-author-date-cs.csl (#5677)
5d017da minor update on the "Haute école de gestion de Genève - ISO 690" style (#5665)
2cad8f6 add ibid/subsequent to comparative-politics.csl (#5669)
de0b116 Create taylor-and-francis-vancouver-national-library-of-medicine.csl (#5650)
ed87f99 Update bulletin-de-correspondance-hellenique.csl (#5663)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 3bb4b5f
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

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
2 participants
@calixtus @Siedlerchr