-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
AutoCompletionReset #14538
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
base: main
Are you sure you want to change the base?
AutoCompletionReset #14538
Conversation
|
Hey @DawydowGerman! 👋 Thank you for contributing to JabRef! We have automated checks in place, based on which you will soon get feedback if any of them are failing. After all automated checks pass, a maintainer will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs. Please re-check our AI Usage Policy to ensure that your pull request is in line with it. It also contains links to our contribution guide in case of any other doubts related to our contribution workflow. |
|
@DawydowGerman it seems you rebased instead of merging.. To start with a clean commit we have following advice Rebase everything as one commit on main
|
|
@koppor, thank you for advice. I'll try to fix the issue. |
* Remove defaults map filling * Add getAutoCompletePreferencesFromBackingStore method * Add getAutoCompletePreferencesFromBackingStore call to getAutoCompletePreferences method * Add getAutoCompletePreferences calls to clear, importPreferences methods * Add private constructor, setAll, getDefault methods --------- Co-authored-by: Carl Christian Snethlage <[email protected]> Co-authored-by: Oliver Kopp <[email protected]>
4e1b6e7 to
077b7f7
Compare
jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java
Show resolved
Hide resolved
| .stream() | ||
| .map(Field::getName) | ||
| .collect(Collectors.joining(";")); | ||
| String completeFieldsFromPrefsNode = get(AUTOCOMPLETER_COMPLETE_FIELDS, completeFields); | ||
| Set<Field> completeFieldsSet = Stream.of(completeFieldsFromPrefsNode.split(";")) |
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.
Please modify getStringList method. Introduce parseStringList method or sthg, but do not introduce magic strings in high level code.
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.
Do you mean something like that:
// Add 2 methods in JabRefCliPreferences class
static Set<Field> convertStringToFieldSet(String toConvert) {
return Stream.of(toConvert.split(";"))
.map(String::trim)
.filter(s -> !s.isEmpty())
.map(String::toUpperCase)
.map(name -> {
try {
return StandardField.valueOf(name);
} catch (IllegalArgumentException e) {
return null;
}
})
.filter(Objects::nonNull)
.collect(Collectors.toCollection(HashSet::new));
}
public Set<Field> getFieldSet(String key, ObservableSet<Field> def) {
try {
return convertStringToFieldSet(get(key));
} catch (NullPointerException e) {
return new HashSet<>(def);
}
}
// in getDonationPreferencesFromBackingStore method remains getFieldSet call
// instead of that complex snippet (18 lines of parsing/converting)
return new AutoCompletePreferences(getBoolean(AUTO_COMPLETE, defaults.shouldAutoComplete()),
firstNameMode,
nameFormat,
getFieldSet(AUTOCOMPLETER_COMPLETE_FIELDS, defaults.getCompleteFields()));```
?
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.
IDK, we have to check:
- Is there any similar functionality already existing somewhere in JabRef. For instance, I found: org.jabref.model.entry.field.FieldFactory#parseFieldList - I found it whlie searching for ; in the class at hand. - I am not sure why the ordering is destroyed by your code. Shouldn't the ordering be kept.
- Please do NOT use
null. You can use Flatmatp.
calixtus
left a comment
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.
.
calixtus
left a comment
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.
Closes #14403
Addressed in this PR:
Steps to test
Mandatory checks
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)