Fix citation keys unintentionally being overwritten on import #7443
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes JabRef#7420 If 'Overwrite existing keys' in 'Preferences' > 'Citation key patterns' is unchecked, 'CitatonKeyGenerator' only overwrites the default citationKey if it is blank. If it is not blank, the default citationKey is not overwritten.
|
Thanks a lot for your PR. I think using the setting 'Citation key patterns' > 'Overwrite existing keys' might be confusing. This is used to warn/prevent users if they try to generate a key using the "Generate key" button in the entry editor, although the entry already has a key. The use case is that after generating a new key, old latex documents stop compiling and some users would like to prevent this. As you see, this has nothing to do with the import. Maybe it's better to add a checkbox in the import dialog. Another idea would be to let the users specify a cleanup preset in the preferences that is applied after an import. The latter would then also resolve #1018. @LucasF-42 @JabRef/developers what do you prefer? |
|
Oh, I've completely missed the mark then, sorry about that. Just for my understanding in general: If the Bibtex-Source-String: is being imported after a web search, default behaviour would be to overwrite the I'm afraid I'm not able to fully comprehend the "cleanup preferences"-solution you've mentioned, so I'm definitely not fit to voice an opinion here. I've just got a first, very localised look at JabRef while attempting to fix the mentioned issue. Not sure about the protocol - would you like me to close this PR and await your decision on this topic? |
I like this option! I think it would address a lot of the confusion regarding when the citation key generator is used and what behavior it has.
I don't think there is such a thing as missing the mark when it comes to JabRef, when something leads to an improvement I think everything is up for discussion and on the mark 😛
I'd argue that this is the case.
I interpret it as essentially running a "cleanup action" on the imported entries, perhaps using different preferences but otherwise same as the |
|
From my point of view as I proposed in #7420 (comment) it needs to the check needs to be added to the import handler before the key generator is called |
Am I reading too much into your comment in that you are aligned with the "add a checkbox in the import dialog" option? In my mind, the logic is that a citation key generator object is created based on preferences ( |
|
Indeed, there is no such thing as missing the mark in our cosy JabRef world. There are two things here that we shouldn't confuse:
Although from a code point of view they are the same, for a user they are quite different and one could easily imagine that a user might want to prevent overwriting manually set keys but still would like to have automatically generated keys upon import. I think, the best way forward would be to introduce a new preference setting "generate (new?) key on import". That could go in the "General" tab below the other import-related preferences (entry owner + time stamp). Or if you feel comfortable enough with the code, create a new preference tab "Import" where you put all of these 3 preferences options. Maybe @calixtus has more input about the structure of the preferences. |
|
Thank you all for the explanations/comments, I feel like I've got a better idea about the subject. I'd have to take a closer look at the project again to see whether I feel like I'd be able to implement the "create a new 'Import tab' in preferences"-solution. I was wondering though, wouldn't the option to decide whether or not to generate a key for a given import in the import dialogue be advantageous as well (on top of the new entry in preferences)? This way a user wouldn't have to change their preferences when importing an entry for which they'd like to go against said preference. (As long as I'm not confusing things again) |
|
I agree that such an option in the import dialog might be handy from time to time. However, this also makes the import dialog more complicated, so I'm not sure if it's a good idea to add it there. Maybe first try the solution via the preferences, and then see if the users would like to have an option during the import as well? |
|
@LucasF-42 It would be really nice if you could go with the simple solution first. That would be a huge benefit already for many users! |
|
It seems I'm unable to accomplish even that, I'm afraid.
The 'feature' as such seems to be working as well, but it's messing with something (or more like: breaking something) and I have been unable to understand what exactly it is.
These exceptions are raised once you have toggled the checkbox in the "Import settings" at least once in a session, as soon as you delete the last remaining entry within a library.
Didn't want to push the branch in it's current (broken) state. |
|
Thanks for your continued work on this. Looks already pretty good. Concerning the error message, could you copy & paste the complete stacktrace. Moreover, it would be good to have the code with your changes at hand. Could you maybe push them to a different branch? |
Commit does not work as intended yet (is buggy)
|
Here's a link to the new branch containing the code on my JabRef fork (hope that's how you wanted me to do that). stack trace details
|
|
Thanks! After looking at your code, I think, the issue is not coming from your changes. It looks like there is an issue somewhere in the entry list code, and you just found a way to trigger them. In fact such issues have been reported before, we just didn't had a way to reproduce them. So I would propose you push your changes to this PR, we merge and then have a look at the exception later. |
tobiasdiez
added
the
status: changes required
|
I kinda forgot about merging/pushing, sorry about that. If further changes/improvements are required/preferred, let me know. |
Siedlerchr
added
status: ready-for-review
|
Looks very good, although I maybe make some little rewordings before merging this to fit to the name conventions of JabRef! Thank you! |
calixtus
approved these changes
Mar 15, 2021
Siedlerchr
added a commit
that referenced
this pull request
Mar 28, 2021
* upstream/master: (191 commits) Fix for issue 7416: font size of the preferences dialog does not update with the rest of the GUI. (#7509) Fix school/instituation is printed twice (#7574) Dsiable notarisation until we hae an account for JabRef e.V. (#7572) Fix citation keys unintentionally being overwritten on import (#7443) Fix AuthentificationPlugin not declared in mergedModule (#7570) Suggestions for changes in caching latex free authors (#7301) Add simple Unit Tests (#7542) Fix drag and drop into empty library (#7555) Bump richtextfx from 0.10.4 to 0.10.6 (#7563) Bump pdfbox from 2.0.22 to 2.0.23 (#7561) Bump org.eclipse.jgit (#7560) Bump fontbox from 2.0.22 to 2.0.23 (#7562) Bump guava from 30.1-jre to 30.1.1-jre (#7564) Bump xmpbox from 2.0.22 to 2.0.23 (#7565) Bump hmarr/auto-approve-action from v2.0.0 to v2.1.0 (#7566) Add gource (#7193) UI: Fix for group icon (#7552) Fix for issue 6487: Opening BibTex file (doubleclick) from Folder with spaces not working (#7551) add ability to insert arxivId (#7549) Fixed missing trigger for linked file operations (#7548) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7420
Hello JabRef-Team,
I've attempted to create a fix for issue 7420. I'm not sure if it's done as expected though, please let me know if I need to change/update anything.
The 'fixed' version behaves like this:
If 'Preferences' > 'Citation key patterns' > 'Overwrite existing keys' is unchecked,
generateKey()inCitationKeyGenerator:citationKeywith an newly generated one only, if it is blankcitationKeyis not overwritten anymore (and therefore stays the same), if it is not blankBehaviour remains unchanged if 'Overwrite existing keys' is checked.
(Let me know if I misunderstood how this feature was supposed to work in general and I'll adapt the behaviour accordingly)
Fixes #7420