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

Container for group item count still visible if display count is off #9972

Closed
mschroen opened this issue Jun 4, 2023 · 12 comments · Fixed by #9980 or #10534
Closed

Container for group item count still visible if display count is off #9972

mschroen opened this issue Jun 4, 2023 · 12 comments · Fixed by #9980 or #10534
Assignees

Comments

@mschroen
Copy link
Sponsor

mschroen commented Jun 4, 2023

Cosmetic bug:
When the display of group item counts is deactivated in the settings, the enclosing darkgrey rectangles are still visible in the group panel.

grafik

@ThiloteE ThiloteE added the groups label Jun 4, 2023
@koppor koppor added the ui label Jun 6, 2023
@BeeJay28
Copy link
Contributor

BeeJay28 commented Jun 6, 2023

Hello, can I work on this issue?

@AEgit
Copy link

AEgit commented Oct 17, 2023

@Siedlerchr See #9980 (comment) - I think the respective changes need to be reverted. Fixing this cosmetic bug actually has removed some functionality from JabRef.

@ThiloteE
Copy link
Member

Have you tried turning "display count of groups" on with never versions of JabRef? Does it still have a large impact on performance?

@AEgit
Copy link

AEgit commented Oct 17, 2023

JabRef 5.11--2023-10-17--8ede31a
Windows 10 10.0 amd64
Java 21-internal
JavaFX 20.0.2+3

Just tried it again. The "Display count of items in groups" performance issue is improved I think (at least from what I recall - I had disabled it for ages, since originally the performance problems it caused were atrocious), but is still clearly noticeable. When I load my database with >24k entries with the count turned off, the CPU load drops after a few sec to 0-5%. With the count of items turned on it stays on 70% for at least a minute and shoots up every time I assign an item to a new group.

@ThiloteE
Copy link
Member

ThiloteE commented Oct 17, 2023

Could you create a separate issue for the performance problems with count of groups? Just so that we can track it more easily. Also, i think removing the container for group items is legit, as it looks very awkward.

Maybe it is possible to have another new more beautiful way that would show that an entry is within a group, when the item count is disabled. I would create a new issue for that.

@AEgit
Copy link

AEgit commented Oct 17, 2023

The performance issue with the counts of groups is known - I created an issue 3 years ago and the workaround was, indeed, to turn of the count of items in groups:
#6042

Funnily enough I already mentioned there, that the background box should not be removed, exactly because of the problem that we encounter now ^^:

However, you shouldn't get rid of the background box surrounding the group item account. This box changes colour according to whether an item selected in the main table is part of a group or not, so this information should be available. In the old JabRef version this was achieved by having the name of a group underlined, which contained a selected item.

As for your suggestion - I would say yes and no. Yes, in the sense that there could be better ways to display group membership (e.g. by underlining the group like in JabRef 3.8.2 or - if you want it to be more fancy - by creating a background box for each group and colouring that according to group membership). This way also the counting of groups items would not be associated with displaying group membership - two distinct features.
No, however, as long as this alternative solution is not implemented. Currently with this implementation it is simply not possible any more to get a visual cue immediately whether an item belongs to a group or not if the item count is turned off. This is clearly a loss of functionality, and, if I say so, a rather big one for me. On the other hand, displaying the grey rectangle, is - as mentioned originally by the thread starter just a "cosmetic bug" at best. It is not even an inconvenience, it just does not look that great (to be honest, I was never really bothered). So as long, as an alternative solution is not implemented, I would heavily suggest to revert the change that was made. I don't think it is worthwhile giving up a major functionality just for beauty's sake ^^

@AEgit
Copy link

AEgit commented Oct 21, 2023

@koppor @Siedlerchr Can this change simply be reverted or should I open a new issue (see #9972 (comment); #9980 (comment))?

@Siedlerchr Siedlerchr reopened this Oct 21, 2023
@Siedlerchr
Copy link
Member

Following @AEgit proposal we should just keep the indicator as a visual indicator

@AEgit
Copy link

AEgit commented Oct 21, 2023

Cheers!

Siedlerchr added a commit that referenced this issue Oct 21, 2023
@koppor
Copy link
Member

koppor commented Oct 21, 2023

Screenshot of marked containment:

image

@koppor
Copy link
Member

koppor commented Oct 21, 2023

We currently need this indicators. Thus, the solution is NOT to just remove them. There needs to be another solution to highlight containment. Maybe a colored background of all groups the entry is belonging to?

@AEgit
Copy link

AEgit commented Oct 22, 2023

JabRef 5.12--2023-10-22--b1abe02
Windows 10 10.0 amd64
Java 21.0.1
JavaFX 20.0.2+3

I can confirm that by reverting the original change the "coloured group indication" feature, which was lost with the originally implemented change (see #9972 (comment); #9972 (comment)), has returned. Well done!

I notice, that for the group "All entries" the rectangle is not coloured no matter which item/article you select - I think this behaviour was different in previous development versions (and the new behaviour may be unexpected given that all items should be part of this group?), but it is not a real problem, since all items should be part of the "All entries" group anyway. So whether that is indicated by colour or not, should make no difference to the user.

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