Skip to content

Fixed #6601: Right click menu for main table header#7729

Closed
sj30001 wants to merge 26 commits into
JabRef:mainfrom
Qiming-Liu:issue6601
Closed

Fixed #6601: Right click menu for main table header#7729
sj30001 wants to merge 26 commits into
JabRef:mainfrom
Qiming-Liu:issue6601

Conversation

@sj30001

@sj30001 sj30001 commented May 11, 2021

Copy link
Copy Markdown

Fixes #6601

  • 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.

We've added right-click menu in JabRef main table header to achieved a show and hide function for different table columns. The right-click menu also gives the user a link to the preferences entry table.

Right Click menu:
right click menu

Hide and show table columns:
show and hide

Jump to preferences entry table:
link to preference

Multi-language support:
mulri language support

Bug Fixed:
1.Fix right-click not working properly on Mac OS
2.Fix show and hide status being refreshed after preference saved

@Siedlerchr

Copy link
Copy Markdown
Member

Thanks, overall this looks already good! I'm just not that happy with the approach for the visible status. I need to think about a better approach.
Meanwhile, please have a look at the reviewdog/checkstyle

@k3KAW8Pnf7mkmdSMPHz27

k3KAW8Pnf7mkmdSMPHz27 commented May 11, 2021

Copy link
Copy Markdown
Member

Perhaps use a BooleanProperty instead of a boolean and bind it to the MainTableColumn visibilityProperty() and the selectedProperty of the radio menu item?

@sj30001

sj30001 commented May 11, 2021

Copy link
Copy Markdown
Author

Thanks, we'll take a look at reviewlog

@calixtus

Copy link
Copy Markdown
Member

Right click menu is a good thing, but the visible property in this case not...
This creates a huge wtf moment for the users, who can now independently add/remove columns to the main table in the preferences dialog as well as in the right click menu with completely different methods.

Please remove the visible property handling here and try add/remove columns (maybe with a standard set of columns) in the list of main table columns. See TableTabViewModel.

@calixtus calixtus added the status: changes-required Pull requests that are not yet complete label May 12, 2021
@Siedlerchr

Copy link
Copy Markdown
Member

@sj30001 It would be really nice if you could add the required changes

@sj30001

sj30001 commented May 19, 2021

Copy link
Copy Markdown
Author

@sj30001 It would be really nice if you could add the required changes

Thanks, I'll discuss with our team and apply changes as required

puneetvalad and others added 2 commits May 19, 2021 13:21
…methods - getVisibleStatus() & setVisibleStatus(boolean visible)
Fixed Style Check Bug: Added extra space after method definition for …
@sj30001

sj30001 commented May 19, 2021

Copy link
Copy Markdown
Author

can now independently add/remove columns to the main table in the preferences dialog as well as in the right-click menu wit

Hi Calixtus,
Our team just had a meeting, and we are a little bit confused about the changes we need to do. The right-click menu will only set the columns visibility it won't add or remove any columns like what we can do in the preference window. If we use our right-click menu to hide a column it won't remove it completely (ie. no column will be deleted in the preference window), it can be shown again by using the right-click menu again. The visible property is there to make this show and hide function works. If we remove it I think we'll lose the functionality that issue 6601 wants us to solve.

Another confusion we have is with the auto check, we've managed to fix the style check problem reported by auto-check, but there is a MacOS installer fail check we don't know why it didn't pass. We've used a Mac machine to build the run the project with no problem. Do we need to worry about that? Thanks a lot for reading this, it will be great if you could give us a little hint on what we should do. Thanks again.
Steve

@Siedlerchr

Copy link
Copy Markdown
Member

You can ignore the mac installer. It's not working on forks because it requires access to secrets for notarization.

@calixtus

calixtus commented May 23, 2021

Copy link
Copy Markdown
Member

About the visibility property. Yes what you describe is from the user perspective the main problem: With your changes there are now two different ways to show/hide columns: By changing the columns in the preferences AND also by showing/hiding them in the right-click menu. This is confusing for the user, as the user expects only one place / one way to show and hide columns in the main table. So I would ask you, to show/hide the columns by adding or removing them from the columns list instead of changing the visibility property. Note that this is also in my opinion a question of performance, since hidden by visibility property columns are still computed. In a database with only about 100 entries, this may not make much difference, but in database of 10000 it does.

In the right-click-menu there should only be a list with commonly used columns, that can be added or removed, maybe also the recently used columns. If you have questions I'm happy to help.

Thank you for your efforts to make JabRef a better library management tool!

@sj30001

sj30001 commented May 23, 2021

Copy link
Copy Markdown
Author

About the visibility property. Yes what you describe is from the user perspective the main problem: With your changes there are now two different ways to show/hide columns: By changing the columns in the preferences AND also by showing/hiding them in the right-click menu. This is confusing for the user, as the user expects only one place / one way to show and hide columns in the main table. So I would ask you, to show/hide the columns by adding or removing them from the columns list instead of changing the visibility property. Note that this is also in my opinion a question of performance, since hidden by visibility property columns are still computed. In a database with only about 100 entries, this may not make much difference, but in database of 10000 it does.

In the right-click-menu there should only be a list with commonly used columns, that can be added or removed, maybe also the recently used columns. If you have questions I'm happy to help.

Thank you for your efforts to make JabRef a better library management tool!

Thanks, this clarifies our confusion. We'll make the necessary changes ASAP. Thanks again

@Qiming-Liu

Copy link
Copy Markdown

Hi, Thanks for the guidance.
We have encountered two problems in the implementation process and I would greatly appreciate it if you would help us with the same :

  1. How to get the standard columns list from maintable?
  2. How to create one standard column and refresh the header after the creation?

@calixtus

Copy link
Copy Markdown
Member

A standard column list can be made up, yet we only have the default values in JabRefPreferences. But these are probably only too few.
For creating columns and refreshing the table see the class TableTab in preferences/table package and JabRefFram::setUpAllTables for details.

@koppor

koppor commented Jul 4, 2021

Copy link
Copy Markdown
Member

Closing this PR due to inactivity 💤

Please comment on the PR if you intend to continue to work on the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: changes-required Pull requests that are not yet complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add right click menu for main table header

9 participants