Save sort order column of main table by Siedlerchr · Pull Request #4327 · JabRef/jabref

Siedlerchr · 2018-09-09T17:38:25Z

I tired to fix the above issue. But I have the problem that the column sort listener is never fired when the sort order is DESCENDING.

Edit// I also fixed an FX Threading error when opening a new library (the snackbar output was not onFX Thread)

@tobiasdiez Any idea if I did sth wrong oder have to set the value somewhere else?

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

tobiasdiez · 2018-09-09T19:14:47Z

Sorry no idea, would also need to debug this to see what's going on.

Siedlerchr · 2018-09-10T09:39:14Z

Hm, this is odd. I created a simple test application based on the code here https://code.makery.ch/blog/javafx-8-tableview-sorting-filtering/
and I experience the same behaviour

Siedlerchr · 2018-09-10T11:27:53Z

Apparently the SortOrder alone was not sufficient: https://stackoverflow.com/a/52255643/3450689
It works now

tobiasdiez

Nice! I've only a few remarks, mainly around code that apparently was added in previous tries and is now no longer needed.

tobiasdiez · 2018-09-10T16:15:17Z

+
+    public Optional<SortType> getSortTypeForColumn(String columnName) {
+
+        if (columnName.equals(columnSortOrder.get(0))) {


This parsing should happen in the JabRefPreferences class so that the constructor here admits a map: column > sort order

tobiasdiez · 2018-09-10T16:17:09Z


+        //Set sort order column from preferences, currently only single column suported
+        this.getColumns().forEach(col -> preferences.getColumnPreferences().getSortTypeForColumn(col.getText()).ifPresent(sortType -> {
+            this.getSortOrder().add(col);


Is this call this.getSortOrder().add(col); really necessary? I thought the TableView manages the sort order list on its own.

Yes, I found it out while testing. The sort oder defines the column which are considered to be sorting.
The sortType property only has an influence if the column is the sortOrder

tobiasdiez · 2018-09-10T16:18:22Z


        this.pane.getStylesheets().add(MainTable.class.getResource("MainTable.css").toExternalForm());

+        //Set sort order column from preferences, currently only single column suported


why is only one column supported? You already transverse all columns here and set the right sort order, or do I miss something?

As I learned from the SO answer, with shift it's possible to add multiple columns to the sort order.
However, currently only one column is stored, e.g. if you have both author DESCENDING and timestamp ASCENDING, it will only store the first one. Or do we have an elegant solution to store Key-Value Pairs in the preferences? e.g. a list of columns with sort type.

The standard way is to have two lists, one for the keys and one for the values. In the progress, it would be nice if you would add the corresponding wrapper methods (get/putMap) in JabRefPreferences for reuse in the future.

tobiasdiez · 2018-09-10T16:21:21Z

    private void updateColumnPreferences() {
        List<String> columnNames = new ArrayList<>();
        List<String> columnsWidths = new ArrayList<>();
+        List<String> columnSortOrders = new ArrayList<>();


This looks like it's not used...

tobiasdiez · 2018-09-10T16:21:53Z

        return get(ID_ENTRY_GENERATOR);
    }
+
+    public void setMainTableColumnSortOrder(String column, String sortType) {


Accept SortOrder and not a string as second argument.

tobiasdiez · 2018-09-10T16:22:15Z

+        putStringList(SORT_COLUMN, Arrays.asList(column, sortType));
+    }
+
+    public List<String> getMainTableColumnSortOrder() {


Siedlerchr · 2018-09-11T11:23:40Z

I will try if I can store the sortOrder as well.

* upstream/master: Fix that web search pane steals input (#4335) Fix entry editor field display (#4333) Convert update worker to BackgroundTask (#4334) Fix that "Rename and move file" throws file not found exception (#4317)

Siedlerchr · 2018-09-12T10:31:02Z

I now managed to save the whole sort oder for multiple columns.

tobiasdiez

I just stumbled upon it, but it appears that Maintable.setupComparatorChooser contained the old logic for storing the sort order. This (and related code like preference keys) probably can now be deleted.

* upstream/master: Fix export of corporate author to MSOffice (#4337) Added new keyboard shortcut "ctrl+E" for closing the entry editor. (#4222) (#4338) Remove GlobalFocusListener and old TextField entry editor (#4336)

remove old table prefs sort order fields also from prefs

tobiasdiez · 2018-09-13T10:18:16Z

            if (this.getBoolean(JabRefPreferences.EXPORT_IN_SPECIFIED_ORDER)) {
                saveOrder = this.loadExportSaveOrder();
-            } else {
-                saveOrder = this.loadTableSaveOrder();


I suspect this is still needed to allow the user to export entries in the current sort order.

But the preferences are no longer set and therefore removed. So I think this is similar to default order?

I still think the export sort order should be different from the table sort order and I will remove table sort order. But I currently see no easy way to convert the table column sort order to our schema

I'm fine with removing the "export according to table sorting" feature since it may quickly lead to unexpected exports (although I'm pretty confident that some users rely on this feature). But then please add a changelog entry and check if the documentation needs to be adopted accordingly.

I quickly had an idea on how to re implement this which works.

restore table sort oder config

tobiasdiez

Perfect!

Save sort order column of main table

ed6aeb0

add sort type listener for each column

f9f7d33

Siedlerchr changed the title ~~[WIP] Save sort order column of main table~~ Save sort order column of main table Sep 10, 2018

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 10, 2018

Siedlerchr added 2 commits September 10, 2018 13:46

fix checkstyle

2300b75

remove old method

bde05ce

tobiasdiez requested changes Sep 10, 2018

View reviewed changes

refactor code

6e9a564

Siedlerchr mentioned this pull request Sep 11, 2018

Jabref hangs when clicking on Optional fields Tab #4331

Closed

Siedlerchr added 3 commits September 12, 2018 10:39

store column sort oder for all columns

067896e

fix checkstyle

15c7c44

remove unused code

2a8296b

tobiasdiez reviewed Sep 12, 2018

View reviewed changes

Siedlerchr added 3 commits September 13, 2018 11:50

remove old comparator choser

9af6979

remove old table prefs sort order fields also from prefs

fix l10n

81cd54e

tobiasdiez reviewed Sep 13, 2018

View reviewed changes

fix l10n

4868593

restore table sort oder config

tobiasdiez approved these changes Sep 13, 2018

View reviewed changes

tobiasdiez merged commit ed7ea50 into master Sep 13, 2018

tobiasdiez deleted the storeColumnSortOrder branch September 13, 2018 11:51

Siedlerchr mentioned this pull request Nov 1, 2019

Reorderable columns in maintable for groups, URI, file and eprint #5544

Merged

6 tasks

calixtus mentioned this pull request Dec 10, 2019

Persistent column sortorder #5730

Merged

5 tasks


		public Optional<SortType> getSortTypeForColumn(String columnName) {

		if (columnName.equals(columnSortOrder.get(0))) {


		this.pane.getStylesheets().add(MainTable.class.getResource("MainTable.css").toExternalForm());

		//Set sort order column from preferences, currently only single column suported

Uh oh!

Conversation

Siedlerchr commented Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez commented Sep 9, 2018

Uh oh!

Siedlerchr commented Sep 10, 2018

Uh oh!

Siedlerchr commented Sep 10, 2018

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Sep 11, 2018

Uh oh!

Siedlerchr commented Sep 12, 2018

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Siedlerchr commented Sep 9, 2018 •

edited

Loading

tobiasdiez Sep 11, 2018 •

edited

Loading