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

Add disable/enable calculation of items in group #6233

Merged
merged 14 commits into from
Apr 5, 2020

Conversation

Gena928
Copy link
Contributor

@Gena928 Gena928 commented Apr 1, 2020

fixes #6042
This is my first Pull Request to a project (new Java developer).

Add: disable/enable calculation of items in group

I realized that GroupNodeViewModel.java class uses method "calculateNumberOfMatches" to get number of items. Looks like it uploads full database from disk every time you need to calculate No of items in group. Therefore, if you have 1 thousand groups, it will read the entire database 1 thousand times.

UI changes:

  • added new checkbox in Preferences/Groups - "Display quantity of items in group";
  • added new property DISPLAY_GROUP_QUANTITY to JabRefPreferences.java file;
  • connected new checkbox with JabRefPreferences.java. I used another preferences as a sample;

Logic changes:

  • I added link to JabRefPreferences in GroupTreeView class.
    If user wants to display groups quantity, a panel with quantity will be shown. Quantity is calculated when GroupNodeViewModel class is initialized, so I decided to wrap a panel into an "if" statement.

Tests passed.

@Siedlerchr Siedlerchr changed the title Resolve issue #6042 Add disable/enable calculation of items in group Apr 2, 2020
@Siedlerchr
Copy link
Member

Thanks for your contribution and thanks for investigating:

Looks like it uploads full database from disk every time you need to calculate No of items in group. Therefore, if you have 1 thousand groups, it will read the entire database 1 thousand times.

Could you add some details on this problem, so maybe we are able to fix the underlying issue as well

@Gena928
Copy link
Contributor Author

Gena928 commented Apr 2, 2020

Good day,

I was working on this issue: https://github.com/JabRef/jabref/issues/6042
It says JabRef starts slowly if you have a lot of groups. Proposed improvement: disable calculation of items quantity in group.

Kind regards,
Gennadiy

- Used this property in GroupsTabView
- DisplayGroupQuantity renamed into DisplayGroupCount in GroupsTabView & GroupsTabViewModel
- removed line from ...da.properties;
- added line to ...en.properties;
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.

Looks good to me. Thanks for your contribution. Just one last remark before we can merge.

BindingsHelper.includePseudoClassWhen(node, allSelected,
group.allSelectedEntriesMatchedProperty());
}
Text text = new Text();
Copy link
Member

Choose a reason for hiding this comment

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

As noted in the issue, the circle node should always be displayed because it is also used for the any/all selected status. Can you thus please add the if check only around the text here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done,
please see new GroupTreeeView.java. OK now?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for quick follow-up! Looks good indeed.

Gena928 and others added 2 commits April 5, 2020 13:46
Main task - disable calling "group.getHits()" method to avoid calculation of items in group.
@tobiasdiez tobiasdiez merged commit efc69be into JabRef:master Apr 5, 2020
tobiasdiez added a commit that referenced this pull request Apr 5, 2020
@tobiasdiez
Copy link
Member

Oh, I was a bit quick with merging. Just saw that the code only influences the display of the hit number. But the cost intensive process is the actual calculation of this number, which still happens.

Could you please open a follow-up PR which makes sure that the calculation is also not done. Thanks.

private void calculateNumberOfMatches() {
// We calculate the new hit value
// We could be more intelligent and try to figure out the new number of hits based on the entry change
// for example, a previously matched entry gets removed -> hits = hits - 1
BackgroundTask
.wrap(() -> groupNode.calculateNumberOfMatches(databaseContext.getDatabase()))
.onSuccess(hits::setValue)
.executeWith(taskExecutor);
}

@Gena928
Copy link
Contributor Author

Gena928 commented Apr 5, 2020

Hi,
sorry, my bad. I saw this last week, and forgot today in the morning. Would you mind, if I add PreferencesService to GroupNodeViewModel?
@Inject private PreferencesService preferencesService;

In this case my "if" statement goes from GroupTreeView to GroupNodeViewModel (calculateNumberOfMatches() method). Like this:

   private void calculateNumberOfMatches() {
       // We calculate the new hit value
       // We could be more intelligent and try to figure out the new number of hits based on the entry change
       // for example, a previously matched entry gets removed -> hits = hits - 1
       if (preferencesService.getDisplayGroupCount()) {
           BackgroundTask
                   .wrap(() -> groupNode.calculateNumberOfMatches(databaseContext.getDatabase()))
                   .onSuccess(hits::setValue)
                   .executeWith(taskExecutor);
       }
   }

OK?

@Gena928
Copy link
Contributor Author

Gena928 commented Apr 5, 2020

We can also remove CalculateNumberOfMatches() from GroupNodeViewModel constructor, and have 2 options:

Option No. 1

  • insert CalculateNumberOfMatches() into getHits() method, before returning a value to GroupTreeView;

Options No. 2

  • create a public method getHitsFromDatabase() (having CalculateNumberOfMatches inside) and call it from GroupTreeView, before calling getHits(). But only if count of if tems in groups is enabled.

@tobiasdiez
Copy link
Member

I think the solution you proposed in #6233 (comment) is perfect!

koppor pushed a commit that referenced this pull request Nov 15, 2022
7a10e3f Bluebook: Remove small-caps for case short & legislation (#6305)
ca4a92b Merge pull request #6244 from POBrien333/patch-1107
12743ad Create social-science-history.csl (#6233)
f7c1d57 Update american-chemical-society.csl (#6231)
aca6b23 ready: Update oil-shale.csl (#6225)
aadae55 Create pallas.csl (#6204)
cc7d016 Merge pull request #6253 from nschneid/patch-1
77fab39 Merge pull request #6282 from POBrien333/patch-1119
e2668eb Merge pull request #6263 from POBrien333/patch-1113
7f3244d move style to dependent folder
8584993 Create development-growth-differentiation.csl (#6226)
dfe71ff Create biotechnology-and-bioprocess-engineering.csl (#6227)
0d91742 Create sn-computer-science.csl (#6228)
a0d09b4 Create mots.csl (#6205)
47665e5 Update review-of-international-studies.csl
d573b8b Create computer-supported-cooperative-work.csl
cebec0e ACL: check if edition is ordinal before printing the word "edition"
03a0a39 Re-indent CSL styles
3c64906 fix self link
479c061 fix small issues after visual inspection
a356e72 Create gnosis-journal-of-gnostic-studies.csl

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 7a10e3f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposed enhancement: Make item count in groups panel an optional feature that can be turned off
3 participants