Conversion of Preferences/TableColumnsTab to mvvm

[calixtus]

follow up to #5033 , concerns #4927 #3416
fixes #5164 #4752 #3354 #4459

This is going to be a huge one. I already put some work into, but I need to take a little break, So I present an early WIP of the reworked TableColumsTab in the Preferences. There are some issues in sight, eg. the checkModel is sime kind of special.

I realized, there is maybe something wrong with the serializeSpecialFields-Option. I was not able to find any use for this pref in the whole code.

Im trying not to break anything, so besides the conversion I will not change the logic of what is controlled here. I'll leave that to another person, after im done here. My goal is to include all the specialFields and the FileFields in the CheckListView, where you can sort, activate, deactivate, add custom fields (so called 'UnknownField') and remove them as you like.

Feel free to comment on it, give suggestions and ideas.

Update: We did some conceptual changes. See below.

---

• 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?)

[Siedlerchr]

I generally like the checkedListView approach, but I am not sure if we need all the file type columns. Maybe one generic (not sure if this is possible with our architecture)

Second, as we can reorder the columns in the maintable, it would be even better to add a righ click option there to add new columns instead of the preferences.
What do you think @JabRef/developers @MartinKarim

[calixtus]

I agree, adding and removing columns by a ContextMenu is probably more intuitive, and I must confess, I do not dislike this idea. One could also change the title or the type of the column by doubleclicking it or by dropdownMenu inside the ColumnTitle or something like that...
On the other hand, it can become confusing, if the list of options is growing to large and this would also break up the unity of how to change the preferences. So it must really be worth it. Ideas? Suggestions? Opinions?

I put the ExtraFileColumns in the CheckListView, to simplify the layout of the tab, as I found it very confusing with multiple listviews etc., because basically every single option (besides the radiobuttons) is creating a column in the maintable. So I think, the PrefsTab should reflect that. I wanted to change the checkboxes to simply enable the SpecialFields and FileFields as possible checkable options in the checkListView.

About the ExtraFileColums: i was wondering myself, why there are distinct columns for every single ExternalFileType, as there is already a column, that does what you described, called the fileColumn. If only one file is attached, it displays a symbol of that type, if multiple files are attached, a generic symbol. By rightclicking that symbol, you can access the attached files. So I was not sure, if those multiple ExtraFileColumns are there as an artifact of an older JabRefVersion, or an enhancement, I did not yet understand, or just some silly idea of earlier days. 😉 This ExtraFileColumsLogic can probably be removed, as they are superceeded by the single fileColumn, can't it? Opinions?

[Siedlerchr]

. I wanted to change the checkboxes to simply enable the SpecialFields and FileFields as possible checkable options in the checkListView.

Yep this is also an idea I had an mind.

This ExtraFileColumsLogic can probably be removed, as they are superceeded by the single fileColumn, can't it? Opinions?

I would remove it and just keep the generic file column

Renaming columns is a bit problematic I think, as they are bound to the field constants.

For the moment I would really say go with the single checked list view. We can then further discuss how to proceed with the maintable option contextmenu.

[calixtus]

This is driving me mad. I'm debugging this for hours.

1. After storing the correct new order of the tablecolumns in the preferences, AND updating the maintable, the maintable uses for half a second the new columnorder, but then refreshes and uses the old one.
2. Moving a column in the prefs down leaves unchecked items in the checkListView. Not sure if this is my error, or a bug in controlsFX.

@JabRef/developers Suggestions? Ideas?

If it's ok for you, im going to delete the extraFileColumns as @Siedlerchr suggested...

[calixtus]

Ad 1: I managed to locate the problem to the ChangeListener in PersistentVisualState of the MainTable. It somehow rewrites the preferences everytime a value is changed, so every change made in the preferences is rewritten, as soon as the columns of the MainTable are changed in the same session with the old values. My question is: As this is not directly a problem of the preferencesTab, but of the mainTable, should I go deep into it, fixing it (which would require some larger refactoring of the MainTable and the PersistentVisualState, or should I leave it, create in issue and concentrate on the other PreferencesTabs?
Ad 2: The checks are set correctly in the CheckViewModel, but they somehow won't be displayed in the ListView. Moving items upwards on the other hand works correctly. This is very confusing to me.

[calixtus]

Another solution would be to remove the up/down buttons in the preferences dialog and let the mainTable itself handle resizing and rearranging the columns (which it already does fine). This could be an early, workable and mergable solution. Someone could still add this functionality later, and I could move on to the next tab...
Opinions?

edit: Another possibility would be to forget about the whole CheckListView and to use a TableView with the name and a delete icon in the rows and a ComboBox to add items below like in the xmp-tab. This would of course not solve the first problem.

[calixtus]

I thought a while about the CheckListView. ControlsFX is somewhat nice, but it acutally causes more problems, than it solves. I'm going to change that to a tableView similiar to the XmpPrefsTab. But not today. 😄

[tobiasdiez]

Sorry for the late answer, was hiking the last few weeks. First of all thanks a lot for the work you put into the conversation of the preference tabs. Much appreciated.

The table columns tab is a huge mess and your changes are a huge improvement. I think there are a few things that can still be improved:

• There is no conceptual difference between normal fields (e.g. author) and the special columns (e.g. doi/url, file, arxiv). In the end, both kinds are represented as columns - just that the display is slightly different for some columns. Thus, I would prefer a gui solution that treats all kinds of columns on the same level. Proposal: remove of all the special checkboxes on the right and integrate them in the main list view. Maybe it is a good idea to highlight the special fields when adding a new column (e.g. by having a drop-down control that first lists the special fields and then the ordinary ones).

• I think the additional checkbox adds a bit too much complexity to the interface. What does it mean to have a field/column in the list view which is unchecked? Is it shown in the main interface or not? In a special way? I would only display the fields/columns that are really shown and list the other options only if the user adds a new column.

• Having the functionality to move fields up and down is relatively important I would say (because for some users drag & drop of columns isn't that natural). You can also implement them as buttons in the row of the listview (similar to how it is done in the Manage journal abbreviations or key bindings dialog).

• Right now the functionality of the special fields is tightly connected to display of these fields as columns. In my opinion this should be decoupled and the preference settings connected to the special fields should be moved to a different tab.

• Concerning the bug with the PersistentVisualState, it is ok if you only create a new issue for it at the moment. (Although fixing it would be better of course.)

[calixtus]

No problem, there is no hurry - summertime is vacation time.
Thanks for the suggestions. Im going to abandon the CheckListView, as it is problematic to handle and obviously a bit buggy, and the CheckBoxes. Instead I'm going to take a shot at the solution the journal abbreviations, with an actions-column.
About the special fields: I see, the right place for the contents of a special field to be shown, would be the entry-editor. Enabling them should therefore move either to the entryEditorTab oder the generalTab.

[calixtus]

Issue seems to exist already: #5164
I think I'm gonna work on it.

[Siedlerchr]

I would actually move this class to a new file and put it next to the other Fields.

[calixtus]

I am somewhat unsure about this. As this is just a wrapper class for the TableRowModel in the columnsList to draw the ExtraFileFieldColumns, I dont think it belongs into the model of Jabref. We already talked about removing these ExtraFileColumns completly, but after the discussion in #5244 I am very careful about removing any functionality...

[calixtus]

I created a util-class to store the ExtraFilePseudoField-class and a helper method which is going to be used also in PR #5265 . I hope this is a usable compromise.

[Siedlerchr]

overal looks good to me, just two minor things

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@89ebaf7). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #5185   +/-   ##
=========================================
  Coverage          ?   37.32%           
  Complexity        ?     6114           
=========================================
  Files             ?     1053           
  Lines             ?    45830           
  Branches          ?     5606           
=========================================
  Hits              ?    17106           
  Misses            ?    27401           
  Partials          ?     1323

Impacted Files	Coverage Δ	Complexity Δ
...ef/gui/preferences/PreferencesDialogViewModel.java	0% <ø> (ø)	0 <0> (?)
...va/org/jabref/gui/maintable/ColumnPreferences.java	0% <0%> (ø)	0 <0> (?)
...rc/main/java/org/jabref/gui/util/NoCheckModel.java	0% <0%> (ø)	0 <0> (?)
...bref/gui/preferences/TableColumnsTabViewModel.java	0% <0%> (ø)	0 <0> (?)
...rg/jabref/gui/preferences/TableColumnsTabView.java	0% <0%> (ø)	0 <0> (?)
.../main/java/org/jabref/gui/maintable/MainTable.java	0% <0%> (ø)	0 <0> (?)
.../jabref/gui/preferences/TableColumnsItemModel.java	0% <0%> (ø)	0 <0> (?)
...java/org/jabref/preferences/JabRefPreferences.java	28.11% <0%> (ø)	14 <0> (?)
...org/jabref/gui/preferences/PreferencesActions.java	0% <0%> (ø)	0 <0> (?)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89ebaf7...b368698. Read the comment docs.

[tobiasdiez]

For the next PR: you can get rid off these castings as follows:

• Add a generic type argument: AbstractPreferenceTabView<T extends PreferenceTabViewModel>
• Change the variable to protected T viewModel;
• Change declaration to TableColumnsTabView extends AbstractPreferenceTabView<TableColumnsTabViewModel>

[tobiasdiez]

The code looks good to me and the UI is definitely a huge improvement already. Thus I'll merge now. It would be nice however if the changes proposed in #5185 (comment) are not forgotten ;-)

Moreover, #5185 (comment) should give you few more minutes away from thesis ;-)