Move generate key option to citation key preference tab#12436
Conversation
Recording.2025-01-31.mp4It's working fine. |
There was a problem hiding this comment.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
There was a problem hiding this comment.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
There was a problem hiding this comment.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
|
Good thing, this also refers to this user's post https://discourse.jabref.org/t/jabref-v5-15-is-not-using-the-merged-entry-random-changes-to-bibfile/5403/3?u=siedlerchr |
|
@priyanshu16095 you can keep my requested changes on hold. @calixtus may have some opinions on whether we want to shift or keep the preference in the |
There was a problem hiding this comment.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
|
Yes, my concern was that putting that pref option into keyGeneratorPreferences, this would breach the level of abstraction or hierarchy, because the pref option about if to use the keygenerator on import is different of how it is configured. Yet in the UI there is no place where this option would go otherwise. So my suggestion would be to leave the pref option codewise in the importerPreferences and put the ui option into the key generator tab. |
4e3f058 to
0e015c0
Compare
|
Done! I have moved the option in the UI while keeping the preference in Importer Preferences. |
subhramit
left a comment
There was a problem hiding this comment.
Quick comments. Also, @priyanshu16095, please no force-pushes!
|
Got it! I’ll avoid force-pushing. |
| public CitationKeyPatternTabViewModel(CitationKeyPatternPreferences keyPatternPreferences, ImporterPreferences importerPreferences) { | ||
| this.keyPatternPreferences = keyPatternPreferences; | ||
|
|
||
| // Defines key generation preference during import. |
There was a problem hiding this comment.
A javadoc comment at field level is preferable - javadoc is often used in relaying ambiguities/abnormalities/edge cases to the developer. Remove this, and put the lines:
- The preference for whether to use the keygenerator on import is different from how it is configured.
- For the UI, there is no better place to put the option than the Citation key generator tab, but shifting the preference to CitationKeypreferences would break the abstraction or hierarchy.
- Hence, we keep the preference in ImporterPrefernces, but in the UI, we initialize it here.
above line 40 (where the field is declared).
Take a look at how @link is used to link classes in javadoc.
There was a problem hiding this comment.
Thank you, I was not aware of this.
subhramit
left a comment
There was a problem hiding this comment.
I pushed a minor commit (b3d42f1) correcting the class name in the javadoc - IntelliJ will give you a hint whether the class is linked correctly or not by highlighting it in red.
I also shifted the keyPatternPreferences declaration above in order, as those are the primary preference options exposed via this view. The edge case comes after that.
Rest lgtm from my side code-wise. Await a second review.
|
Thank you, @subhramit , for your help at every step. |
|
@Siedlerchr @calixtus one of you can take a second look, i think we are near completion here |
|
Thanks for your work! |
Fixes #10849
This PR moves the "Generate new keys on import" option from the WebSearch tab to the CitationKey Preferences tab.
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)