JabRef / jabref

Graphical Java application for managing BibTeX and biblatex (.bib) databases
https://devdocs.jabref.org
MIT License
3.54k stars 2.47k forks source link

Problems during group renaming/changing in some cases #8824

Open claell opened 2 years ago

claell commented 2 years ago

Is your suggestion for improvement related to a problem? Please describe. When I rename a group, I am asked whether I want all old entries to that group. However, the old group name remains assigned to this group.

Describe the solution you'd like I had expected that the old group gets removed from the entries, at least an option for that should be provided.

Additional context

ThiloteE commented 2 years ago

Edit: I now CAN ~Cannot~ reproduce. See https://github.com/JabRef/jabref/issues/8824#issuecomment-1169216030

JabRef 5.7--2022-05-16--6a2332f Linux 5.4.0-110-generic amd64 Java 18.0.1 JavaFX unknown

~Maybe you have run into an index out of bounds issue and then the UI will depict weird stuff? I would suggest to restart JabRef whenever you run into an index out of bounds issue.~

claell commented 2 years ago

Hm, interesting. Not sure whether that is what happened. I'll do some other tests, then.

claell commented 2 years ago

Just tried again and couldn't reproduce, either. I think, I experienced this problem already before. So it was not the first time, I ran into it.

What do you mean with "the UI will depict weird stuff"? That did not happen to me. Possibly, I got an exception that I ignored. But I am not confident that it was an index out of bounds exception.

I'll leave this open for a few more days in case I run into this again. At least, it seems clear that this behavior is not normal (so no need for improvement).

ThiloteE commented 2 years ago

See e.g. issues #8012 and #8527 for "weird stuff"

In general: As far as I understand it, if there is an out of bounds syntax error (which are somehow connected to the bi-directional binding), the UI will either depict an old state as compared to what you entered or it will depict a new state, but on disc there is still old data. So you can't trust what is shown on the UI. If you get one, restart JabRef, the best thing you can do.

Of course, right now, we don't know if it was an index out of bounds issue. I was just speculating.

claell commented 2 years ago

From what you describe, that is not what happened to me.

I saw this looking at the .bib file itself. So the old category was still in there.

ThiloteE commented 2 years ago

refs #8826

ThiloteE commented 2 years ago

I assume you renamed groups by right clicking on a group and choosing "edit"?

claell commented 2 years ago

Yes, that is how I renamed the group.

ThiloteE commented 2 years ago

I can now reproduce:

  1. Create group.

    • Name group 1.
    • Choose "Collect by" with Explicit selection.
    • Add entry to this group. Resulting data in {}biblatex source tab: groups = {1},
  2. Edit group. Change "Collect by" to Searching for a keyword. Example: grafik Add old entries to this group. Resulting data in {}biblatex source tab: groups = {1, 2},

Desired result:

groups = {2},

ThiloteE commented 2 years ago

It also works the other way round:

Do the same thing, except

  1. first choose "Collect by" with Searching for a keyword
  2. then edit to "Collect by" with Explicit selection
ThiloteE commented 1 year ago

@DohaRamadan, since you seem to be interested in UI / groups issues, and you already have gained some experience, I suppose this issue might be of interest. This is a very annyoing one, as it is the root cause of very unintuitive experiences.

DohaRamadan commented 1 year ago

Thank you! i will check it out

ThiloteE commented 1 year ago

This is an easy issue at first glance (e.g. just remove the old group), but it is a little bit more complicated than the others you have worked on, since we need to take into account situations where the entry is part of multiple groups e.g. via "explicit selection". For example: image

Also, I hope we do not run into index out of bounds issues, if groups are automatically removed from entries.

ThiloteE commented 1 year ago

A very closely related, but different issue is #8836. Reading through this bug might help you to better understand how the groups feature works and it's current limitations.

DohaRamadan commented 1 year ago

I found out that there is another issue, If I created a group and assigned an entry to it, and then deleted this group, the entry's groups are not updated and you can still see that the deleted group is still present in the entry's groups. Is there an issue for this bug?

DohaRamadan commented 1 year ago

Also, I'm having trouble understanding the whole edit group logic, shouldn't entries be assigned to the new group and removed from the old group always? why is there a dialog to assign entries to the new group when it's by intuition should always be assigned to the new group, and the removePreviousAssignments variable is vague too?

ThiloteE commented 1 year ago

If I created a group and assigned an entry to it, and then deleted this group, the entry's groups are not updated and you can still see that the deleted group is still present in the entry's groups. Is there an issue for this bug?

I don't think there is a specific issue for this apart from this issue here.

ThiloteE commented 1 year ago

Also, I'm having trouble understanding the whole edit group logic, shouldn't entries be assigned to the new group and removed from the old group always? why is there a dialog to assign entries to the new group when it's by intuition should always be assigned to the new group, and the removePreviousAssignments variable is vague too?

If you ALWAYS remove it automatically from the group, you will also remove the entry from all other groups with this name.

So if you have a group hierarchy like this:

Bugs:

Birds:

and you delete the prio1 group from bugs, you will also delete the prio1 from birds, which is NOT what you would want. This could lead to unintentional dataloss and users will be confused too. We would fix one issue, but simply create another issue.

So we have to think hard how to solve this. The easy way out is to come up with a preference or clickable option during the ui popup that allows users to decide. The more complicated, but maybe better solution, would be to change the whole groups architecture.

ThiloteE commented 1 year ago

(PS, I am not good at coding. Hoping for other maintainers to comment on the coding stuff)

DohaRamadan commented 1 year ago

Also, I'm having trouble understanding the whole edit group logic, shouldn't entries be assigned to the new group and removed from the old group always? why is there a dialog to assign entries to the new group when it's by intuition should always be assigned to the new group, and the removePreviousAssignments variable is vague too?

If you ALWAYS remove it automatically from the group, you will also remove the entry from all other groups with this name.

So if you have a group hierarchy like this:

Bugs:

  • prio1
  • prio2

Birds:

  • prio1
  • prio2

and you delete the prio1 group from bugs, you will also delete the prio1 from birds, which is NOT what you would want. This could lead to unintentional dataloss and users will be confused too. We would fix one issue, but simply create another issue.

So we have to think hard how to solve this. The easy way out is to come up with a preference or clickable option during the ui popup that allows users to decide. The more complicated, but maybe better solution, would be to change the whole groups architecture.

I understand the problem better now, the group identifier is its name and that's the main problem in my opinion, there are 2 solutions to this 1- We impose a unique constraint on group names such that each group name must be unique. 2- We identify groups by a unique identifier (something like an integer called groupID) and when editing a group, we reference the old group and the new group by their groupID rather than their names. I think solution 2 is better and best practice but I need a second opinion here.

DohaRamadan commented 1 year ago

I want to work on this. Also, maintainers's insight on my previous comment would be highly appreciated.

tobiasdiez commented 1 year ago

@DohaRamadan This opens a very old box of the pandora, see https://github.com/JabRef/jabref/issues/1495. I tend to agree that id-based serialization is a good idea, although it invalidates some of the advantages outlined at https://github.com/JabRef/jabref/issues/629. With JabRef Online we will have the same issue to not be able to identify what group an entry actually belongs to.

@JabRef/developers please discuss this in the jabcon/next devcall

ThiloteE commented 1 year ago

@DohaRamadan just so you know, the next regular devcall is in four days (Monday evening, every two weeks).

koppor commented 1 year ago

Meta: We should write down a clean ADR (Folder: https://github.com/JabRef/jabref/tree/main/docs/decisions)

+1: For IDs.

ID stays same even if name changes.

ID technology:

We have to ensure that we do not simply rewrite the library, but ask for it. - Thus, https://github.com/JabRef/jabref/issues/10370 is a pre-condition of the solution of this issue. And that issue also has a pre-condition

tobiasdiez commented 1 year ago

In jabrefonline, I use cuid's.

koppor commented 1 year ago

Any pro/con list? I vote for CUID2, because if "Deterministic Length" in comparison to CUID1.

With length 10, CUID2 is short enough for me (remember the storage in plain text and that BibTeX users will check the file)

https://github.com/paralleldrive/cuid2#configuration

tobiasdiez commented 1 year ago

Mostly because prisma doesn't yet support cuid2 by default (https://github.com/prisma/prisma/issues/17102), but there are workarounds...so no hard requirement from my side.

claell commented 11 months ago

Nice to see some activity on this.

Reading the discussion, I was thinking, whether some hybrid approach with both IDs and names might be interesting.

For storage, that would then allow users to read the group names in the BibTeX file. Additionally, it might be helpful for copying between libraries. In such cases, the current approach of IDs would probably generate new groups. With a name attached, one could try to match groups with the same name and open a dialog where the user can manually select whether the pasted entry groups should be merged with existing ones (of the same name) or a new group should be created.

claell commented 11 months ago

For now, as it seems that the current implementation timeline for an ID-based system is pretty long (please correct me if this assumption is wrong): Might it be sensible to require unique group names for now as a quick solution that already fixes many problems?

ThiloteE commented 11 months ago

Yes, the timeline seems to be long, as for all problems here, nothing is really broken. I realized the other day, that our groups feature is actually a "keywords/tags/labels" feature, so we could theoretically rename groups to tags and then introduce a "real" groups feature.

For all problems here, there is a (lenghty and uncomfortable) workaround by adjusting user workflows and giving groups unique names, so in my opinion, the priority is "I am looking forward to and it would be very nice to have a real groups feature, which I personally find kinda important, but urgency is medium/low, as workarounds exist". Also, this feature competes with other issues that are arguably even more important (e.g. CSL styles, finishing the new Search, fixing performance, JabRef online, Fixing Grobid, Fixing ISBN fetcher, etc.)

claell commented 11 months ago

I think, we had the discussion about groups being more like keywords/tags/labels already some time ago :)

I'd argue that right now, there is quite something not working correctly (or as expected) for the group feature. It is not robust in any way.

But yes, if there are no resources for this, that's fine. I just thought about not blocking @DohaRamadan or others from improving the feature with postponing the point where it can be really improved unnecessarily into the future.