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.

Sign up for GitHub

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

Jump to bottom

Add restart warning for auto complete settings #6564

Closed
wants to merge 5 commits into from
Closed

Add restart warning for auto complete settings #6564

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d980ef
fix-6351
newtypes9 May 31, 2020
c725ef0
update
newtypes9 May 31, 2020
f3c3a00
Apply suggestions from code review
newtypes9 Jun 1, 2020
8864bd3
change request
newtypes9 Jun 3, 2020
f1611f2
change request
newtypes9 Jun 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/main/java/org/jabref/gui/preferences/EntryEditorTabViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.jabref.gui.autocompleter.AutoCompleteFirstNameMode;
import org.jabref.gui.autocompleter.AutoCompletePreferences;
import org.jabref.gui.entryeditor.EntryEditorPreferences;
import org.jabref.logic.l10n.Localization;
import org.jabref.model.entry.field.FieldFactory;
import org.jabref.preferences.PreferencesService;

Expand All @@ -37,6 +38,8 @@ public class EntryEditorTabViewModel implements PreferenceTabViewModel {
private final EntryEditorPreferences initialEntryEditorPreferences;
private final AutoCompletePreferences initialAutoCompletePreferences;

private List<String> restartWarnings = new ArrayList<>();

public EntryEditorTabViewModel(DialogService dialogService, PreferencesService preferencesService) {
this.dialogService = dialogService;
this.preferencesService = preferencesService;
Expand Down Expand Up @@ -106,7 +109,13 @@ public void storeSettings() {
} else if (firstNameModeFullProperty.getValue()) {
firstNameMode = AutoCompleteFirstNameMode.ONLY_FULL;
}

if(initialAutoCompletePreferences.shouldAutoComplete()!=enableAutoCompleteProperty.getValue()){
Copy link
Member

@Siedlerchr Siedlerchr Jun 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review.

if(enableAutoCompleteProperty.getValue()){
restartWarnings.add(Localization.lang("Auto complete enabled."));
}else{
restartWarnings.add(Localization.lang("Auto complete disabled."));
}
}
preferencesService.storeAutoCompletePreferences(new AutoCompletePreferences(
enableAutoCompleteProperty.getValue(),
firstNameMode,
Expand All @@ -122,7 +131,7 @@ public boolean validateSettings() {

@Override
public List<String> getRestartWarnings() {
return new ArrayList<>();
return restartWarnings;
}

public BooleanProperty openOnNewEntryProperty() {
Expand Down
31 changes: 31 additions & 0 deletions src/test/java/org/jabref/gui/preference/SaveAutoCompleteTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package org.jabref.gui.preference;


import org.jabref.gui.autocompleter.AutoCompleteFirstNameMode;
import org.jabref.gui.autocompleter.AutoCompletePreferences;
import org.jabref.model.entry.field.FieldFactory;
import org.jabref.preferences.JabRefPreferences;
import org.junit.Test;

import java.lang.reflect.Field;

import static org.junit.Assert.assertEquals;


public class SaveAutoCompleteTest {

@Test
public void storeAutoCompletePreferencesTest() throws NoSuchFieldException, IllegalAccessException {
JabRefPreferences preferencesService = JabRefPreferences.getInstance();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While testing is normally important, I fear in this case you actually permanently set the preferences to true instead of testing the logic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

preferencesService.storeAutoCompletePreferences(new AutoCompletePreferences(
true,
AutoCompleteFirstNameMode.BOTH,
AutoCompletePreferences.NameFormat.BOTH,
FieldFactory.parseFieldList(""),
preferencesService.getJournalAbbreviationPreferences()));

Field field = preferencesService.getClass().getDeclaredField("AUTO_COMPLETE");
field.setAccessible(true);
assertEquals(preferencesService.getBoolean((String) field.get(preferencesService)), true);
}
}