Add restart warning for auto complete settings#6564
Conversation
calixtus
left a comment
There was a problem hiding this comment.
Thank you for your contribution.
Restart warning looks good, besides two typos.
I'm not 100 % sure, but I believe the test is probably interfering with the permanent preferences of JabRef. @tobiasdiez @Siedlerchr ?
|
|
||
| @Test | ||
| public void storeAutoCompletePreferencesTest() throws NoSuchFieldException, IllegalAccessException { | ||
| JabRefPreferences preferencesService = JabRefPreferences.getInstance(); |
There was a problem hiding this comment.
While testing is normally important, I fear in this case you actually permanently set the preferences to true instead of testing the logic.
There was a problem hiding this comment.
The test actually does not make any sense. You are just testing that a boolean variable is set in the preferences. This is useless and totally unrelated to your changes.
if you want to write a test, you need to test the View Model to check if your changed are correctly applied. But that's more complicated.
Manual testing should be enough. In jabref we focus on writing tests for important logic or other parts and not just writing tests for the sake of coverage.
So please remove your test.
There was a problem hiding this comment.
Yeah, you are right. That test makes no sense for this issue. This was my early attempt while locating the code. It is really kind for you to point it out as well as two typos. Thanks. And I am still learning Github and its features.
|
|
||
| @Test | ||
| public void storeAutoCompletePreferencesTest() throws NoSuchFieldException, IllegalAccessException { | ||
| JabRefPreferences preferencesService = JabRefPreferences.getInstance(); |
There was a problem hiding this comment.
The test actually does not make any sense. You are just testing that a boolean variable is set in the preferences. This is useless and totally unrelated to your changes.
if you want to write a test, you need to test the View Model to check if your changed are correctly applied. But that's more complicated.
Manual testing should be enough. In jabref we focus on writing tests for important logic or other parts and not just writing tests for the sake of coverage.
So please remove your test.
Correct typos. Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
| firstNameMode = AutoCompleteFirstNameMode.ONLY_FULL; | ||
| } | ||
|
|
||
| if(initialAutoCompletePreferences.shouldAutoComplete()!=enableAutoCompleteProperty.getValue()){ |
There was a problem hiding this comment.
you have a lot of checkstyle issues here. Please setup the JabRef Code Style and make your life easier
https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace#using-jabrefs-code-style
There was a problem hiding this comment.
Thanks for your review.
calixtus
left a comment
There was a problem hiding this comment.
Please add the strings to the english localization resource file and somehow the two typos came back...
|
This is a good start. Since there was no activity since a few weeks and there are minor things to fix to get this ready, we will take over here. Thank you for having been part at the JabRef development. |
|
I took the liberty to finish this one in another PR, your work is preserved an we will give you credit in the next release of JabRef. Thank you for your work! |
To make auto complete settings come into effect, restart is needed. I add dialog box when someone change the auto complete settings, to remind him restart the software.
Fixes #6351