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

Fixed error when removing entries from group when bibtex source tab is selected (#8012) #8711

Closed
wants to merge 5 commits into from

Conversation

Yancy10-1
Copy link

@Yancy10-1 Yancy10-1 commented Apr 22, 2022

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.
fix.8012.mp4

#8012

@calixtus calixtus changed the title Right clicking a group and choosing "remove selected entries from this group" leads to error when {}Bibtex source is selected #8012 Fixed error when removing entries from group when bibtex source tab is selected (#8012) Apr 22, 2022
@calixtus
Copy link
Member

Could you please fix the checkstyle errors pointed out by our reviewdog? Thanks.

@ThiloteE
Copy link
Member

image

@Yancy10-1
Copy link
Author

@ThiloteE I fixed the style in that field. You can see it doesn't have any more, but I can not pass the checkstyle test.

@ThiloteE
Copy link
Member

  1. The error is gone! Was not able to reproduce out of index error, which is great!
  2. This unfortunately apparently caused a new problem: Editing fields in {} biblatex source tab is now not possible anymore!

Sorry, this pull request needs changes :/

@ThiloteE ThiloteE added status: changes required Pull requests that are not yet complete [outdated] type: bug Confirmed bugs or reports that are very likely to be bugs component: groups component: entry-editor labels Apr 24, 2022
@Yancy10-1
Copy link
Author

@ThiloteE I checked the problem, but it can run well.

editTheBibTexsource.mp4

@ThiloteE
Copy link
Member

I might have stumbled upon another out of index error, which might have nothing to do with your pull-request, but I am not sure.

Give me a few minutes, I will create an issue with video.

@ThiloteE
Copy link
Member

ThiloteE commented Apr 25, 2022

#8719 might explain the issues I had, not sure though.

Maybe you could play around using this library?

test-#8673-encoding.bib.txt

Edit: Don't focus on solving this one. Albeit it would be nice if this one were solved, this one is probably not related to your pull-request, because I can trigger it in JabRef 5.6 AND with your implementation, therefore the cause is not your pull request.

@ThiloteE
Copy link
Member

ThiloteE commented Apr 27, 2022

Dear Yancy, I did two tests with your version of JabRef:

The first one showed that you did very well. I tried hard triggering another out of bounds error, and only at the end was able to trigger one, but your implementation seemed to work definitely a lot better than JabRef 5.6! See here:

2022-04-27.23-41-26-1.MNWE.Issue.8711.removing.first.entry.from.groups.index.out.of.bounds.mp4

After I triggered the index out of bounds, I restarted JabRef and continued testing. This time I again came across the problem I initially meant and mentioned in #8711 (comment). See here:

2022-04-27.23-54-00.Mnwe2.Issue.8711.Removing.whole.fields.-.Change.Not.Detected-1.mp4

I hope this will help you in some way 😶

Edit:

I would like to mention that

  1. Don't be discouraged. We never knew the error as shown in the first video existed, because removing entries from groups would ALWAYS lead to an error, when in the bibtex source tab, regardless at which position in the main table the entry was. Now, with your implementation, it only happens to the first entry on top of the main table. Your pull request therefore would be an improvement to the status quo in this regard.

  2. I cannot reproduce the problem, as depicted in the second video, in JabRef 5.6, so this is probably something triggered by your Pull-request.

@Yancy10-1
Copy link
Author

@ThiloteE I will try my best to fix it!

@ThiloteE
Copy link
Member

ThiloteE commented May 15, 2022

On gitter, @LIM0000 has posted some stuff related to deleting (removing) groups. Maybe it could be relevant here as well.

Hi teams, I am working on JabRef/jabref#8390 and have figured out a solution to fix this issue.

Solution (please refer to image below):
I have planned to run a loop to remove selected groups instead of just remove the only group that fire setOnAction().
Issue with this solution:
If a group has been deleted, the iterator will skip the next element due to that element is now at that deleted element index. Refer (https://stackoverflow.com/questions/17852412/foreach-skips-an-element-when-a-previous-element-is-deleted)
Really appreciate if someone could provide insight to this issue, thanks!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: entry-editor component: groups [outdated] type: bug Confirmed bugs or reports that are very likely to be bugs status: changes required Pull requests that are not yet complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants