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 (2) #6244

Closed
wants to merge 84 commits into from

Conversation

Gena928
Copy link
Contributor

@Gena928 Gena928 commented Apr 5, 2020

Fixes #6042

Additional code for issue 6042, based on comment from Tobiasdiez:
https://github.com/JabRef/jabref/pull/6233#issuecomment-609430247

Now GroupNodeViewModel will be responsible for checking user preferences. If user wants to see items count in group - it will be shown. Otherwise - no.

Now it's responsible for count if items in group (enable / disable, based on user preferences)
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Hi @Gena928, thanks for your quick follow-up and your effort.
I found a small problem about the injection of PreferencesService.
Also I would like to ask you to keep the checkboxes in the PR-description. They are just a little help for us to get a quick overview of the extent to which a PR is ready. I took the liberty to readd them by myself. 😉

if (preferencesService.getDisplayGroupCount()) {
text.textProperty().bind(group.getHits().asString());
}
text.textProperty().bind(group.getHits().asString());
Copy link
Member

Choose a reason for hiding this comment

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

Please leave this if check here, otherwise every group will have a 0 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.

Good day,
OK, adjusted in code. Will commit today, when we find solution about PreferencesService in GroupNodeViewModel (see comment from calixtus).

In case we can't use PreferencesService injection, I propose to create a property (displayItemsCount) and set this property from outside (from GroupTreeView) as True/False. True - need get count, False - NO need to get count of items.
Also I found method "onDatabaseChanged" - it fires when you delete/add new items to group. So we need "displayItemsCount" to restrict calculation in this case also.

By default displayItemsCount = False
@tobiasdiez tobiasdiez added the status: changes required Pull requests that are not yet complete label Apr 10, 2020
@koppor koppor added this to the v5.1 milestone Apr 14, 2020
@Hugoo98
Copy link

Hugoo98 commented Apr 21, 2020

@calixtus
Hello, I'm coming back to you regarding bug #6091. As we discussed on #6107, we agreed to start working on #6091 and download our draft in case of progress. Finally we managed to correct it with my colleagues, and now we don't know what the next step is, can you guide us please?
Thank you

@calixtus
Copy link
Member

Hi @Hugoo98 ,
you can now commit your changes to a new branch and push this branch via git to your origin repository. Then you should be able ot create a new pull request to JabRef on GitHubs Web-UI. Then we should be able to review and discuss your changes.

@calixtus
Copy link
Member

If you have general questions about JabRef development, please use the developers gitter chat, so we won't pollute this PR with our Off-Topic comments.

@tobiasdiez
Copy link
Member

@Gena928 did you already found the time to look at the issue with the preferences? Do you have questions concerning it?

@Gena928
Copy link
Contributor Author

Gena928 commented May 21, 2020

@tobiasdiez
looks like we have misunderstanding. Sorry, but I was sure I don't have any unclosed issues.
Which one?

@tobiasdiez
Copy link
Member

tobiasdiez commented May 21, 2020

You are right, I guess this is indeed a misunderstanding.
I was referring to

Will commit today, when we find solution about PreferencesService in GroupNodeViewModel (see comment from calixtus).

So this is then ready for review? The build currently fails and there are merge conflicts. Could you have a look at this? Thanks!

@Gena928
Copy link
Contributor Author

Gena928 commented May 21, 2020

@tobiasdiez
yes, it's ready for review. I'll look at the conflict.

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.

Good!

The changes are fine with me except one thing were I guess a git merge conflict resulted in unnecessary code duplication.

.executeWith(taskExecutor);
if (preferencesService.getDisplayGroupCount()) {
BackgroundTask
.wrap(() -> groupNode.calculateNumberOfMatches(databaseContext.getDatabase()))
Copy link
Member

Choose a reason for hiding this comment

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

I think this duplication of the code is a result from a merge conflict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like yes, this is a conflict.
the second BackgroundTask is not mine. My code is:

    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);
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I haven't worked with conflicts yet.
Looks like this is your correction as of 11.04.2020. Can I do something to resolve this GIT conflict?

Copy link
Member

Choose a reason for hiding this comment

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

No problem! I also often have problems with git ;-)

Here the solution is relatively simple. Just edit the code and remove the part that is duplicated / not correct. Then commit and push.

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 21, 2020
My method (calculateNumberOfMatches) is no longer valid and GroupNodeViewModel must use a new methods called "findMatches"
@Gena928
Copy link
Contributor Author

Gena928 commented May 22, 2020

@tobiasdiez
OK, looks like my old method is no longer valid, because it was refactored.
I updated updateMatchedEntries and now it's using new method - findMatches, according to your commit as of 28.04.2020.

otherwise it will be interpreted as path which obiously fails

Fixes JabRef#6507
tobiasdiez and others added 24 commits May 28, 2020 18:23
Removed BIB file directory from search when preferences has option unchecked
…cycastle-bcprov-jdk15on-1.65.01

Bump bcprov-jdk15on from 1.65 to 1.65.01
…sch.flexmark-flexmark-0.62.0

Bump flexmark from 0.61.32 to 0.62.0
…sch.flexmark-flexmark-ext-gfm-tasklist-0.62.0

Bump flexmark-ext-gfm-tasklist from 0.61.32 to 0.62.0
Bumps [flexmark-ext-gfm-strikethrough](https://github.com/vsch/flexmark-java) from 0.61.32 to 0.62.0.
- [Release notes](https://github.com/vsch/flexmark-java/releases)
- [Commits](https://github.com/vsch/flexmark-java/commits)

Signed-off-by: dependabot-preview[bot] <[email protected]>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Now it's responsible for count if items in group (enable / disable, based on user preferences)
In case we can't use PreferencesService injection, I propose to create a property (displayItemsCount) and set this property from outside (from GroupTreeView) as True/False. True - need get count, False - NO need to get count of items.
Also I found method "onDatabaseChanged" - it fires when you delete/add new items to group. So we need "displayItemsCount" to restrict calculation in this case also.

By default displayItemsCount = False
@calixtus
Copy link
Member

I originally wanted to merge current master into this branch and merge it, but I kinda messed up the git history in this one. Sorry about that. Took the commits to PR #6554 . Maybe someone more experienced with git (@koppor ) can take a quick look at it, and merge it then. 🙈

@koppor
Copy link
Member

koppor commented Jun 2, 2020

No clue. Good that you managed to create a "good PR" at #6554.

@koppor koppor closed this Jun 2, 2020
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
status: changes required Pull requests that are not yet complete 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.

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