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

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

Merged
merged 17 commits into from
Nov 10, 2019

Conversation

calixtus
Copy link
Member

@calixtus calixtus commented Oct 29, 2019

JabRef did not distinguish yet between columns in mainTable and fields. Thats why I had to create an ExtraFilePseudoField-Class in one of my recent PRs. But this was an ugly hack.
JabRef should store instead in the columnPreferences not the fields, but the columns e.g. "groups" or "special:rank" or "field:author". The result would be, that the user is able to have a clickable uri-column "linked_id" in one column, and in another "field:url" the uri printed out.
Also they should finally be reorderable.

Thoughts?


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

@calixtus calixtus changed the title Reorderable Columns in Maintable for Groups, URI, File and eprint [WIP] Reorderable columns in maintable for groups, URI, file and eprint Oct 30, 2019
@calixtus
Copy link
Member Author

calixtus commented Nov 1, 2019

This is ready to review, but there is probably much to talk about.
I believe JabRef should automatically adapt the new settings (by default it should parse a corrupt entry in the preferences as a normal field). Then you only have to add the groups and the linked_id column. If not, it can be fixed by resetting the preferences.

@calixtus calixtus marked this pull request as ready for review November 1, 2019 12:25
@calixtus calixtus changed the title [WIP] Reorderable columns in maintable for groups, URI, file and eprint Reorderable columns in maintable for groups, URI, file and eprint Nov 1, 2019

public static Type parse(String text) {
switch (text) {
case "groups": return GROUPS;
Copy link
Member

Choose a reason for hiding this comment

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

You can use Enum.valueOf() to parse enum keys stored in strings (see for example the group mode union/intersection) thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will look into this. A very first try did not work. I probably have to put the enum in an extra file.

Copy link
Member

Choose a reason for hiding this comment

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

You have to call .name() on the enum keys for storing.
like here:

    public void setGroupViewMode(GroupViewMode mode) {
        put(GROUP_INTERSECT_UNION_VIEW_MODE, mode.name());
    }

Copy link
Member Author

@calixtus calixtus Nov 2, 2019

Choose a reason for hiding this comment

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

valueOf did not really work, as it parses only the name of the constant, but the text is different from the constant name ("linked_id" instead of "LINKED_IDENTIFIER" and "field" instead of "NORMALFIELD"). But I found another solution (with a little inspiration of stackoverflow) which should be somewhat a generic approach.

@Siedlerchr
Copy link
Member

This is already a huge PR. Need to look more into detail about it. Could you maybe add a simple screen record or some screenshots how it works?
but it would also be cool if you could look into applying the saved sort order at startup (without decreasing performance that much as I had)
#4327

@calixtus
Copy link
Member Author

calixtus commented Nov 1, 2019

This is already a huge PR. Need to look more into detail about it. Could you maybe add a simple screen record or some screenshots how it works?
but it would also be cool if you could look into applying the saved sort order at startup (without decreasing performance that much as I had)
#4327

Hi, i know, this one is huge... again. I really was surprised myself, as it did not really went as easy as I thought it would in the first place. Modifiying the MainTableFactory was easy, but the preferences really showed, that I had to distinguish between fields and columns.

There is nothing I can really show by a screen record or screen shot, as nothing really changed on the optics, besides the integration of the "very special columns" in the preferences into the TableView. The PR is all about the hard wired columns "groups", "url", "files" and "extrafile" in the ColumnFactory. I had to modify the preferences to distinguish them from the other field-columns. They follow the scheme "groups", "linked_id" or "field:author", "special:readstatus" or "extrafile:pdf". They all should now behave all the same.

columns

(The column-headers just after startup. Note that I reordered the url column before restarting JabRef.)

prefs

(The EntryTableColumnsTab in the preferences. Note that the silly checkboxes on the right "Show groups column", "Show file column" and "Show URL column" as well as the radiobuttons for "Show DOI first" are gone integrated in the ComboBox on the bottom.)

I merged the icon for URL, URI, DOI and ePrint in one column. By clicking on the icon in that column a context menu pulls out presenting the links. The MainTableColumnModel was introduced to avoid the boilerplate of a TableColumn for use in the preferences.

It's now possible to have both kind of columns: One colum showing an icon, if clicked, opening the link "linked_id", and also a column showing just the text of the url "field:url".

I also looked into you PR #4327. I believe, my PR should integrate just fine with your modifications.

@calixtus
Copy link
Member Author

calixtus commented Nov 1, 2019

Sorry, it does affect the sort order. Im looking into this.

@Siedlerchr
Copy link
Member

Note that the sort order is not restored after opening JabRef. It is saved but not applied on startup. I had to disable it because it was a performance killer

@calixtus
Copy link
Member Author

calixtus commented Nov 1, 2019

I just tested the current master, sort order is somehow broken too.
To be sure we are on the same page: Order of the columns means the title column is to the right (or left) of the authors column and sort order means ascending or descending?
The first one is working persistent for all the columns with my PR, the latter is not working in the current master.

@calixtus
Copy link
Member Author

calixtus commented Nov 1, 2019

Note that the sort order is not restored after opening JabRef. It is saved but not applied on startup. I had to disable it because it was a performance killer

Aaah, i get it. Yes, ok

@Siedlerchr
Copy link
Member

Siedlerchr commented Nov 1, 2019

To be sure we are on the same page: Order of the columns means the title column is to the right (or left) of the authors column and sort order means ascending or descending?

Exactly. If you hold the shift key while pressing on the column title, you can define a primay, seconday and tertiary sort order (ascending/descending) group.

grafik

@calixtus
Copy link
Member Author

calixtus commented Nov 2, 2019

I took the liberty to abstract a magic string on my way: ';' is now Character STRINGLIST_DELIMITER.
Im thinking of integrating the column width into the MainTableColumnModel class, so I could get rid of the TableColumnsItemModel class and make the preferences list display directly the ColumnModel.

@koppor
Copy link
Member

koppor commented Nov 3, 2019

Refs #4919 and maybe #4224.

@calixtus
Copy link
Member Author

calixtus commented Nov 3, 2019

#4424 not yet, @Siedlerchr already asked me to give it a shot. I'm thinking of integrating the sort type into the ColumnModel and the sort order into the ColumnPreferences, so this bottleneck can be avoided.
Also the persistent ColumnPreferences should only be saved when exiting JabRef.

@calixtus calixtus changed the title Reorderable columns in maintable for groups, URI, file and eprint [WIP] Reorderable columns in maintable for groups, URI, file and eprint Nov 3, 2019
@calixtus
Copy link
Member Author

calixtus commented Nov 4, 2019

Well, I managed to get the storing and reloading of the sort criteria running and saw no greater perfomance drop. But I dont have any profiler running here, so its just a very subjective opinion. As this would include more refactoring, I would rather like to do this in another PR, as this is already almost too large.

I saw no quick and easy way to save the columnpreferences on exiting jabref.

So it's ready to review again.

@calixtus calixtus changed the title [WIP] Reorderable columns in maintable for groups, URI, file and eprint Reorderable columns in maintable for groups, URI, file and eprint Nov 4, 2019
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

This part of the code was still one of the most hacky and unorganized in JabRef's code base. It's much clearer now. Good job!

I've only a few minor remarks. It would be nice if you could also add a few tests, at least for the most complex operations (especially the preference migration as this is crucial).

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 5, 2019
Copy link
Member

@koppor koppor 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 comments. Can be merged as is.

@@ -2103,7 +2094,6 @@ List\ must\ not\ be\ empty.=List must not be empty.
Add\ field\ to\ filter\ list=Add field to filter list
Add\ formatter\ to\ list=Add formatter to list
Filter\ List=Filter List
New\ column=New column
Copy link
Member

Choose a reason for hiding this comment

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

Oh, this was never used?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was formerly the name for a new column created by an constructor which was never used, as new columns always had by design the name of a field.
This constructor was removed and new columns now always get created by a specified name.

@@ -1929,8 +1922,6 @@ Move\ panel\ down=Move panel down
Linked\ files=Linked files
Group\ view\ mode\ set\ to\ intersection=Group view mode set to intersection
Group\ view\ mode\ set\ to\ union=Group view mode set to union
Open\ %0\ URL\ (%1)=Open %0 URL (%1)
Open\ URL\ (%0)=Open URL (%0)
Copy link
Member

Choose a reason for hiding this comment

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

Oh, these two were never used?

Copy link
Member Author

Choose a reason for hiding this comment

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

The identifier icon column now collects every url, uri, eprint and doi. On click, the identifier icon column ("linked_id") opens a submenu, like the file icon column. The menuitems of the submenu present the whole link of the collected identifiers (uri etc.) printed out. The removed lines were part of the tooltip, which now presents all the collected links printed out in a very brief form ("URL = ...\nURI = ...\n" etc.).
The icon column is now independet from the field column ("field:url" or "field:doi" etc.) They can print out the stored text of the field in a column.


/**
* The former preferences default of columns was a simple list of strings ("author;title;year;..."). Since 5.0
* the preferences store the type of the column too, so that the formerly hardwired columns like the graphic groups
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the type of the column in the preferences. Is the comment still true?

Copy link
Member Author

Choose a reason for hiding this comment

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

The new form how columns are stored is divided in to parts divided by a colon. First is the column type, the second is, if existent, the qualifier. The new form is in example "groups;linked_id;field:author;field:title:special:readstatus;extrafile:chm".

String columnNames = preferences.get(JabRefPreferences.COLUMN_NAMES);

if (!columnNames.isEmpty() &&
!columnNames.contains(MainTableColumnModel.Type.NORMALFIELD.getName() // "field:"
Copy link
Member

Choose a reason for hiding this comment

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

Could you expand the comment "field:" or remove it?


public class SpecialFieldsPreferences {

public static final int COLUMN_RANKING_WIDTH = 5 * 16; // Width of Ranking Icon Column
Copy link
Member

Choose a reason for hiding this comment

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

Could you put the comment above the line?

@tobiasdiez tobiasdiez merged commit d1484f8 into JabRef:master Nov 10, 2019
@tobiasdiez
Copy link
Member

Thanks again for your work on this!

@calixtus
Copy link
Member Author

You were too quick for me, i was just working on the last remark @koppor made. But I can do this in a later PR. 😄

@calixtus calixtus deleted the maintable_columns_rework branch November 10, 2019 20:02
@koppor
Copy link
Member

koppor commented Nov 11, 2019

@calixtus: Just a follow-up PR. - Sorry that I did not see the type/name thing. Too little spaces there 😅. Refs koppor#232

@calixtus calixtus mentioned this pull request Dec 10, 2019
5 tasks
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.

4 participants