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

Observable Preferences U #9619

Merged
merged 2 commits into from
Mar 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ private String abbreviate(BibDatabaseContext databaseContext, List<BibEntry> ent
UndoableAbbreviator undoableAbbreviator = new UndoableAbbreviator(
Globals.journalAbbreviationRepository,
abbreviationType,
journalAbbreviationPreferences.useAMSFJournalFieldForAbbrevAndUnabbrev());
journalAbbreviationPreferences.shouldUseFJournalField());

NamedCompound ce = new NamedCompound(Localization.lang("Abbreviate journal names"));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,11 @@ public JournalAbbreviationsTab() {

@FXML
private void initialize() {
viewModel = new JournalAbbreviationsTabViewModel(preferencesService, dialogService, taskExecutor, abbreviationRepository);
viewModel = new JournalAbbreviationsTabViewModel(
preferencesService.getJournalAbbreviationPreferences(),
dialogService,
taskExecutor,
abbreviationRepository);

filteredAbbreviations = new FilteredList<>(viewModel.abbreviationsProperty());

Expand Down Expand Up @@ -127,9 +131,8 @@ private void setBindings() {
loadingLabel.visibleProperty().bind(viewModel.isLoadingProperty());
progressIndicator.visibleProperty().bind(viewModel.isLoadingProperty());

searchBox.textProperty().addListener((observable, previousText, searchTerm) -> {
filteredAbbreviations.setPredicate(abbreviation -> searchTerm.isEmpty() || abbreviation.containsCaseIndependent(searchTerm));
});
searchBox.textProperty().addListener((observable, previousText, searchTerm) ->
filteredAbbreviations.setPredicate(abbreviation -> searchTerm.isEmpty() || abbreviation.containsCaseIndependent(searchTerm)));

useFJournal.selectedProperty().bindBidirectional(viewModel.useFJournalProperty());
}
Expand All @@ -145,9 +148,7 @@ private void setAnimations() {
new KeyFrame(Duration.seconds(0), new KeyValue(flashingColor, Color.TRANSPARENT, Interpolator.LINEAR)),
new KeyFrame(Duration.seconds(0.25), new KeyValue(flashingColor, Color.RED, Interpolator.LINEAR)),
new KeyFrame(Duration.seconds(0.25), new KeyValue(searchBox.textProperty(), "", Interpolator.DISCRETE)),
new KeyFrame(Duration.seconds(0.25), (ActionEvent event) -> {
addAbbreviationActions();
}),
new KeyFrame(Duration.seconds(0.25), (ActionEvent event) -> addAbbreviationActions()),
new KeyFrame(Duration.seconds(0.5), new KeyValue(flashingColor, Color.TRANSPARENT, Interpolator.LINEAR))
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import org.jabref.logic.journals.JournalAbbreviationRepository;
import org.jabref.logic.l10n.Localization;
import org.jabref.logic.util.StandardFileType;
import org.jabref.preferences.PreferencesService;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -51,23 +50,21 @@ public class JournalAbbreviationsTabViewModel implements PreferenceTabViewModel
private final SimpleBooleanProperty isAbbreviationEditableAndRemovable = new SimpleBooleanProperty(false);
private final SimpleBooleanProperty useFJournal = new SimpleBooleanProperty(true);

private final PreferencesService preferences;
private final DialogService dialogService;
private final TaskExecutor taskExecutor;

private final JournalAbbreviationPreferences abbreviationsPreferences;
private final JournalAbbreviationRepository journalAbbreviationRepository;
private boolean shouldWriteLists;

public JournalAbbreviationsTabViewModel(PreferencesService preferences,
public JournalAbbreviationsTabViewModel(JournalAbbreviationPreferences abbreviationsPreferences,
DialogService dialogService,
TaskExecutor taskExecutor,
JournalAbbreviationRepository journalAbbreviationRepository) {
this.preferences = Objects.requireNonNull(preferences);
this.dialogService = Objects.requireNonNull(dialogService);
this.taskExecutor = Objects.requireNonNull(taskExecutor);
this.journalAbbreviationRepository = Objects.requireNonNull(journalAbbreviationRepository);
this.abbreviationsPreferences = preferences.getJournalAbbreviationPreferences();
this.abbreviationsPreferences = abbreviationsPreferences;

abbreviationsCount.bind(abbreviations.sizeProperty());
currentAbbreviation.addListener((observable, oldValue, newValue) -> {
Expand Down Expand Up @@ -336,19 +333,16 @@ public void storeSettings() {
.map(path -> path.getAbsolutePath().get().toAbsolutePath().toString())
.collect(Collectors.toList());

preferences.storeJournalAbbreviationPreferences(new JournalAbbreviationPreferences(
journalStringList,
abbreviationsPreferences.getDefaultEncoding(),
useFJournal.getValue()
));
abbreviationsPreferences.setExternalJournalLists(journalStringList);
abbreviationsPreferences.setUseFJournalField(useFJournal.get());

if (shouldWriteLists) {
saveJournalAbbreviationFiles();
shouldWriteLists = false;
}
})
.onSuccess((success) -> Globals.journalAbbreviationRepository =
JournalAbbreviationLoader.loadRepository(preferences.getJournalAbbreviationPreferences()))
JournalAbbreviationLoader.loadRepository(abbreviationsPreferences))
.onFailure(exception -> LOGGER.error("Failed to store journal preferences.", exception))
.executeWith(taskExecutor);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package org.jabref.logic.journals;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Collection;
Expand Down Expand Up @@ -62,6 +61,6 @@ public static JournalAbbreviationRepository loadRepository(JournalAbbreviationPr
}

public static JournalAbbreviationRepository loadBuiltInRepository() {
return loadRepository(new JournalAbbreviationPreferences(Collections.emptyList(), StandardCharsets.UTF_8, true));
return loadRepository(new JournalAbbreviationPreferences(Collections.emptyList(), true));
}
}
Original file line number Diff line number Diff line change
@@ -1,37 +1,41 @@
package org.jabref.logic.journals;

import java.nio.charset.Charset;
import java.util.List;

import javafx.beans.property.BooleanProperty;
import javafx.beans.property.SimpleBooleanProperty;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;

public class JournalAbbreviationPreferences {

private final Charset defaultEncoding;
private List<String> externalJournalLists;
private boolean useFJournalField;
private final ObservableList<String> externalJournalLists;
private final BooleanProperty useFJournalField;

public JournalAbbreviationPreferences(List<String> externalJournalLists, Charset defaultEncoding, boolean useFJournalField) {
this.externalJournalLists = externalJournalLists;
this.defaultEncoding = defaultEncoding;
this.useFJournalField = useFJournalField;
public JournalAbbreviationPreferences(List<String> externalJournalLists,
boolean useFJournalField) {
this.externalJournalLists = FXCollections.observableArrayList(externalJournalLists);
this.useFJournalField = new SimpleBooleanProperty(useFJournalField);
}

public List<String> getExternalJournalLists() {
public ObservableList<String> getExternalJournalLists() {
return externalJournalLists;
}

public void setExternalJournalLists(List<String> externalJournalLists) {
this.externalJournalLists = externalJournalLists;
public void setExternalJournalLists(List<String> list) {
externalJournalLists.clear();
externalJournalLists.addAll(list);
}

public Charset getDefaultEncoding() {
return defaultEncoding;
public boolean shouldUseFJournalField() {
return useFJournalField.get();
}

public boolean useAMSFJournalFieldForAbbrevAndUnabbrev() {
public BooleanProperty useFJournalFieldProperty() {
return useFJournalField;
}

public void setUseAMSFJournalFieldForAbbrevAndUnabbrev(boolean useFJournalField) {
this.useFJournalField = useFJournalField;
public void setUseFJournalField(boolean useFJournalField) {
this.useFJournalField.set(useFJournalField);
}
}
21 changes: 15 additions & 6 deletions src/main/java/org/jabref/preferences/JabRefPreferences.java
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,7 @@ public class JabRefPreferences implements PreferencesService {
private MainTablePreferences mainTablePreferences;
private ColumnPreferences mainTableColumnPreferences;
private ColumnPreferences searchDialogColumnPreferences;
private JournalAbbreviationPreferences journalAbbreviationPreferences;

// The constructor is made private to enforce this as a singleton class:
private JabRefPreferences() {
Expand Down Expand Up @@ -1113,12 +1114,20 @@ public LayoutFormatterPreferences getLayoutFormatterPreferences(JournalAbbreviat

@Override
public JournalAbbreviationPreferences getJournalAbbreviationPreferences() {
return new JournalAbbreviationPreferences(getStringList(EXTERNAL_JOURNAL_LISTS), StandardCharsets.UTF_8, getBoolean(USE_AMS_FJOURNAL));
}
if (Objects.nonNull(journalAbbreviationPreferences)) {
return journalAbbreviationPreferences;
}

@Override
public void storeJournalAbbreviationPreferences(JournalAbbreviationPreferences abbreviationsPreferences) {
putStringList(EXTERNAL_JOURNAL_LISTS, abbreviationsPreferences.getExternalJournalLists());
journalAbbreviationPreferences = new JournalAbbreviationPreferences(
getStringList(EXTERNAL_JOURNAL_LISTS),
getBoolean(USE_AMS_FJOURNAL));

journalAbbreviationPreferences.getExternalJournalLists().addListener((InvalidationListener) change ->
putStringList(EXTERNAL_JOURNAL_LISTS, journalAbbreviationPreferences.getExternalJournalLists()));
EasyBind.listen(journalAbbreviationPreferences.useFJournalFieldProperty(),
(obs, oldValue, newValue) -> putBoolean(USE_AMS_FJOURNAL, newValue));

return journalAbbreviationPreferences;
}

@Override
Expand Down Expand Up @@ -2520,7 +2529,7 @@ public GuiPreferences getGuiPreferences() {
EasyBind.listen(guiPreferences.lastSelectedIdBasedFetcherProperty(), (obs, oldValue, newValue) -> put(ID_ENTRY_GENERATOR, newValue));
EasyBind.listen(guiPreferences.mergeDiffModeProperty(), (obs, oldValue, newValue) -> put(MERGE_ENTRIES_DIFF_MODE, newValue.name()));
EasyBind.listen(guiPreferences.sidePaneWidthProperty(), (obs, oldValue, newValue) -> putDouble(SIDE_PANE_WIDTH, newValue.doubleValue()));
EasyBind.listen(guiPreferences.mergeShowChangedFieldOnlyProperty(), (obs, oldValue, newValue) -> putBoolean(MERGE_SHOW_ONLY_CHANGED_FIELDS, newValue.booleanValue()));
EasyBind.listen(guiPreferences.mergeShowChangedFieldOnlyProperty(), (obs, oldValue, newValue) -> putBoolean(MERGE_SHOW_ONLY_CHANGED_FIELDS, newValue));

return guiPreferences;
}
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/org/jabref/preferences/PreferencesService.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ public interface PreferencesService {

KeyBindingRepository getKeyBindingRepository();

void storeJournalAbbreviationPreferences(JournalAbbreviationPreferences abbreviationsPreferences);

FilePreferences getFilePreferences();

FieldWriterPreferences getFieldWriterPreferences();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import org.jabref.logic.journals.JournalAbbreviationLoader;
import org.jabref.logic.journals.JournalAbbreviationPreferences;
import org.jabref.logic.journals.JournalAbbreviationRepository;
import org.jabref.preferences.PreferencesService;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -62,7 +61,6 @@ class JournalAbbreviationsViewModelTabTest {
private JournalAbbreviationsTabViewModel viewModel;
private Path emptyTestFile;
private Path tempFolder;
private PreferencesService preferencesService;
private final JournalAbbreviationRepository repository = JournalAbbreviationLoader.loadBuiltInRepository();
private DialogService dialogService;

Expand Down Expand Up @@ -179,14 +177,12 @@ public static Stream<TestData> provideTestFiles() {
@BeforeEach
void setUpViewModel(@TempDir Path tempFolder) throws Exception {
JournalAbbreviationPreferences abbreviationPreferences = mock(JournalAbbreviationPreferences.class);
preferencesService = mock(PreferencesService.class);
when(preferencesService.getJournalAbbreviationPreferences()).thenReturn(abbreviationPreferences);

dialogService = mock(DialogService.class);
this.tempFolder = tempFolder;

TaskExecutor taskExecutor = new CurrentThreadTaskExecutor();
viewModel = new JournalAbbreviationsTabViewModel(preferencesService, dialogService, taskExecutor, repository);
viewModel = new JournalAbbreviationsTabViewModel(abbreviationPreferences, dialogService, taskExecutor, repository);

emptyTestFile = createTestFile(new CsvFileNameAndContent("emptyTestFile.csv", ""));
}
Expand Down Expand Up @@ -515,13 +511,6 @@ void testSaveAbbreviationsToFilesCreatesNewFilesWithWrittenAbbreviations(TestDat
assertEquals(testData.finalContentsOfFile3, actual);
}

@ParameterizedTest
@MethodSource("provideTestFiles")
void testSaveExternalFilesListToPreferences(TestData testData) throws IOException {
addFourTestFileToViewModelAndPreferences(testData);
verify(preferencesService).storeJournalAbbreviationPreferences(any());
}

Comment on lines -518 to -524
Copy link
Member

Choose a reason for hiding this comment

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

From reading the deleted code only, I don't get why it is not replaced by some other testing code -
Shouldn't the preferences then stored be in the abbreviationPreferences?

Copy link
Member Author

@calixtus calixtus Feb 16, 2023

Choose a reason for hiding this comment

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

Thing is, it does not make sense to test here if the private putBoolean method is called. This should be a test in JabRefPreferencesTests or JournalAbbreviationPreferencesTest, but this is somewhat trivial and I assume the behavior of observers is testet within javafx already.

If you really want to test the view model we should certainly not test with any() as an argument.

With other words: the test, as it is, is useless imho.

/**
* Select the last abbreviation in the list of abbreviations
*/
Expand Down