-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 O (Language and FileHistory) #9173
Changes from 6 commits
956c175
9722ee3
70a76c0
d5556ff
d068740
999121a
089108c
5c1b9a6
34c84cc
b3a1d47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,24 @@ | |
import java.util.Objects; | ||
import java.util.Optional; | ||
|
||
import org.jabref.logic.l10n.Localization; | ||
import org.jabref.model.metadata.MetaData; | ||
import org.jabref.preferences.PreferencesService; | ||
|
||
public class MetaDataDiff { | ||
public enum Difference { | ||
PROTECTED, | ||
GROUPS_ALTERED, | ||
ENCODING, | ||
SAVE_SORT_ORDER, | ||
KEY_PATTERNS, | ||
USER_FILE_DIRECTORY, | ||
LATEX_FILE_DIRECTORY, | ||
DEFAULT_KEY_PATTERN, | ||
SAVE_ACTIONS, | ||
MODE, | ||
GENERAL_FILE_DIRECTORY, | ||
CONTENT_SELECTOR | ||
} | ||
|
||
private final Optional<GroupDiff> groupDiff; | ||
private final MetaData originalMetaData; | ||
|
@@ -32,44 +45,44 @@ public static Optional<MetaDataDiff> compare(MetaData originalMetaData, MetaData | |
/** | ||
* @implNote Should be kept in sync with {@link MetaData#equals(Object)} | ||
*/ | ||
public List<String> getDifferences(PreferencesService preferences) { | ||
List<String> changes = new ArrayList<>(); | ||
public List<Difference> getDifferences(PreferencesService preferences) { | ||
List<Difference> changes = new ArrayList<>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest using an EnumSet, can be initialized with noneOf. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EnumSet is done, but it does not make sense to me to introduce a map to store the localized strings, as they would have to be initialized. But it is probably very rare that many metadata diffs happen the same time. So initializing all of the strings would be just a waste of memory and time. |
||
|
||
if (originalMetaData.isProtected() != newMetaData.isProtected()) { | ||
changes.add(Localization.lang("Library protection")); | ||
changes.add(Difference.PROTECTED); | ||
} | ||
if (!Objects.equals(originalMetaData.getGroups(), newMetaData.getGroups())) { | ||
changes.add(Localization.lang("Modified groups tree")); | ||
changes.add(Difference.GROUPS_ALTERED); | ||
} | ||
if (!Objects.equals(originalMetaData.getEncoding(), newMetaData.getEncoding())) { | ||
changes.add(Localization.lang("Library encoding")); | ||
changes.add(Difference.ENCODING); | ||
} | ||
if (!Objects.equals(originalMetaData.getSaveOrderConfig(), newMetaData.getSaveOrderConfig())) { | ||
changes.add(Localization.lang("Save sort order")); | ||
changes.add(Difference.SAVE_SORT_ORDER); | ||
} | ||
if (!Objects.equals(originalMetaData.getCiteKeyPattern(preferences.getGlobalCitationKeyPattern()), newMetaData.getCiteKeyPattern(preferences.getGlobalCitationKeyPattern()))) { | ||
changes.add(Localization.lang("Key patterns")); | ||
changes.add(Difference.KEY_PATTERNS); | ||
} | ||
if (!Objects.equals(originalMetaData.getUserFileDirectories(), newMetaData.getUserFileDirectories())) { | ||
changes.add(Localization.lang("User-specific file directory")); | ||
changes.add(Difference.USER_FILE_DIRECTORY); | ||
} | ||
if (!Objects.equals(originalMetaData.getLatexFileDirectories(), newMetaData.getLatexFileDirectories())) { | ||
changes.add(Localization.lang("LaTeX file directory")); | ||
changes.add(Difference.LATEX_FILE_DIRECTORY); | ||
} | ||
if (!Objects.equals(originalMetaData.getDefaultCiteKeyPattern(), newMetaData.getDefaultCiteKeyPattern())) { | ||
changes.add(Localization.lang("Default pattern")); | ||
changes.add(Difference.DEFAULT_KEY_PATTERN); | ||
} | ||
if (!Objects.equals(originalMetaData.getSaveActions(), newMetaData.getSaveActions())) { | ||
changes.add(Localization.lang("Save actions")); | ||
changes.add(Difference.SAVE_ACTIONS); | ||
} | ||
if (originalMetaData.getMode() != newMetaData.getMode()) { | ||
changes.add(Localization.lang("Library mode")); | ||
changes.add(Difference.MODE); | ||
} | ||
if (!Objects.equals(originalMetaData.getDefaultFileDirectory(), newMetaData.getDefaultFileDirectory())) { | ||
changes.add(Localization.lang("General file directory")); | ||
changes.add(Difference.GENERAL_FILE_DIRECTORY); | ||
} | ||
if (!Objects.equals(originalMetaData.getContentSelectors(), newMetaData.getContentSelectors())) { | ||
changes.add(Localization.lang("Content selectors")); | ||
changes.add(Difference.CONTENT_SELECTOR); | ||
} | ||
return changes; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,50 +1,61 @@ | ||
package org.jabref.logic.util.io; | ||
|
||
import java.nio.file.Path; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.Objects; | ||
|
||
import javafx.collections.FXCollections; | ||
import javafx.collections.ObservableList; | ||
import javafx.collections.ModifiableObservableListBase; | ||
|
||
public class FileHistory { | ||
public class FileHistory extends ModifiableObservableListBase<Path> { | ||
|
||
private static final int HISTORY_SIZE = 8; | ||
|
||
private final ObservableList<Path> history; | ||
private final List<Path> history; | ||
|
||
public FileHistory(List<Path> files) { | ||
history = FXCollections.observableList(Objects.requireNonNull(files)); | ||
private FileHistory(List<Path> list) { | ||
history = new ArrayList<>(list); | ||
} | ||
|
||
@Override | ||
public Path get(int index) { | ||
return history.get(index); | ||
} | ||
|
||
public int size() { | ||
return history.size(); | ||
} | ||
|
||
public boolean isEmpty() { | ||
return history.isEmpty(); | ||
@Override | ||
protected void doAdd(int index, Path element) { | ||
history.add(index, element); | ||
} | ||
|
||
@Override | ||
protected Path doSet(int index, Path element) { | ||
return history.set(index, element); | ||
} | ||
|
||
@Override | ||
protected Path doRemove(int index) { | ||
return history.remove(index); | ||
} | ||
|
||
/** | ||
* Adds the file to the top of the list. If it already is in the list, it is merely moved to the top. | ||
*/ | ||
public void newFile(Path file) { | ||
removeItem(file); | ||
history.add(0, file); | ||
this.add(0, file); | ||
while (size() > HISTORY_SIZE) { | ||
history.remove(HISTORY_SIZE); | ||
} | ||
} | ||
|
||
public Path getFileAt(int index) { | ||
return history.get(index); | ||
} | ||
|
||
public void removeItem(Path file) { | ||
history.remove(file); | ||
this.remove(file); | ||
} | ||
|
||
public ObservableList<Path> getHistory() { | ||
return history; | ||
public static FileHistory of(List<Path> list) { | ||
return new FileHistory(new ArrayList<>(list)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would extend the enum here and add the localization as second parameter so you don't need a switch statement and instead you can directly return the message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would contradict the whole meaning of this commit, to extract the call to the logic package from the model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there is something odd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correcting myself: to reduce the calls to the Localization package, to be able to move the 'mother' class to the model package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then introduce a map with enum keys and l10n values