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

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

Closed
wants to merge 26 commits into from
Closed

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

wants to merge 26 commits into from

Conversation

sj30001
Copy link

@sj30001 sj30001 commented May 11, 2021

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
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
Copy link
Member

k3KAW8Pnf7mkmdSMPHz27 commented May 11, 2021

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

@sj30001
Copy link
Author

sj30001 commented May 11, 2021

Thanks, we'll take a look at reviewlog

@calixtus
Copy link
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
Member

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

@sj30001
Copy link
Author

sj30001 commented May 19, 2021

@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
Copy link
Author

sj30001 commented May 19, 2021

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
Member

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

@calixtus
Copy link
Member

calixtus commented May 23, 2021

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
Copy link
Author

sj30001 commented May 23, 2021

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

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
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
Copy link
Member

koppor commented Jul 4, 2021

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