Add disable/enable calculation of items in group by Gena928 · Pull Request #6233 · JabRef/jabref

Gena928 · 2020-04-01T20:50:43Z

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.

Corrected resources file after not passing the test

Siedlerchr · 2020-04-02T10:04:27Z

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 · 2020-04-02T15:20:37Z

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;

tobiasdiez

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

tobiasdiez · 2020-04-04T19:26:55Z

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


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.

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

Thanks for quick follow-up! Looks good indeed.

Main task - disable calling "group.getHits()" method to avoid calculation of items in group.

This reverts commit efc69be.

tobiasdiez · 2020-04-05T13:22:10Z

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.

jabref/src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java

Lines 228 to 236 in b5d5ed0

    
           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 · 2020-04-05T14:10:55Z

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 · 2020-04-05T14:55:06Z

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 · 2020-04-05T15:01:30Z

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

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

Gena928 added 9 commits March 30, 2020 22:20

Init changes

834c65f

Merge branch 'master' of https://github.com/JabRef/jabref

bc03c03

Init changes

6f5b718

Восстановлено исходное состояние мастера

f25ec45

Просто коммит

6b7ce6d

Merge branch 'master' of https://github.com/JabRef/jabref

7d87baa

Merge branch 'master' of https://github.com/JabRef/jabref

7df02e4

Issue #6042 fixed

2862f2a

Issue #6042 fixed

181f1e9

Corrected resources file after not passing the test

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

Siedlerchr requested changes Apr 2, 2020

View reviewed changes

Comment thread src/main/java/org/jabref/gui/groups/GroupTreeView.java Outdated

Siedlerchr requested a review from tobiasdiez April 2, 2020 16:40

Siedlerchr reviewed Apr 3, 2020

View reviewed changes

Comment thread src/main/resources/l10n/JabRef_da.properties Outdated

Gena928 added 2 commits April 3, 2020 11:23

- Added new property to PreferencesService and JabRefPreferences

cc4d449

- Used this property in GroupsTabView - DisplayGroupQuantity renamed into DisplayGroupCount in GroupsTabView & GroupsTabViewModel

- removed line from JabRef_en.properties

09cded7

Siedlerchr reviewed Apr 3, 2020

View reviewed changes

Comment thread src/main/resources/l10n/JabRef_da.properties Outdated

Updated Properties file

cdc18a9

- removed line from ...da.properties; - added line to ...en.properties;

Siedlerchr approved these changes Apr 3, 2020

View reviewed changes

tobiasdiez requested changes Apr 4, 2020

View reviewed changes

Gena928 and others added 2 commits April 5, 2020 13:46

Changed GroupTreeView according to comments from Tobiasdiez

ea19b70

Main task - disable calling "group.getHits()" method to avoid calculation of items in group.

Remove empty line

bfbffe0

tobiasdiez merged commit efc69be into JabRef:master Apr 5, 2020

tobiasdiez added a commit that referenced this pull request Apr 5, 2020

Revert "Add disable/enable calculation of items in group (#6233)"

c176cc5

This reverts commit efc69be.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add disable/enable calculation of items in group#6233

Add disable/enable calculation of items in group#6233
tobiasdiez merged 14 commits into
JabRef:masterfrom
Gena928:6042

Gena928 commented Apr 1, 2020

Uh oh!

Siedlerchr commented Apr 2, 2020

Uh oh!

Gena928 commented Apr 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobiasdiez left a comment

Uh oh!

tobiasdiez Apr 4, 2020

Uh oh!

Gena928 Apr 5, 2020

Uh oh!

tobiasdiez Apr 5, 2020

Uh oh!

tobiasdiez commented Apr 5, 2020

Uh oh!

Gena928 commented Apr 5, 2020

Uh oh!

Gena928 commented Apr 5, 2020

Uh oh!

tobiasdiez commented Apr 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Gena928 commented Apr 1, 2020

Uh oh!

Siedlerchr commented Apr 2, 2020

Uh oh!

Gena928 commented Apr 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Apr 4, 2020

Choose a reason for hiding this comment

Uh oh!

Gena928 Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

tobiasdiez commented Apr 5, 2020

Uh oh!

Gena928 commented Apr 5, 2020

Uh oh!

Gena928 commented Apr 5, 2020

Uh oh!

tobiasdiez commented Apr 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants