-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Tag-Based Selection for Entry fields in Preferences #12592
base: main
Are you sure you want to change the base?
Conversation
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.
Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.
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.
Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.
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.
Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.
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.
Hi, welcome to JabRef and thanks for your contribution!
Below are some comments on first look.
<CheckBox fx:id="addModificationDate" | ||
text="%Add timestamp to modified entries (field "modificationdate")"/> |
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.
Was this line-break unintentional? Please revert.
} | ||
|
||
public void initialize() { | ||
this.viewModel = new EntryTabViewModel(preferences); | ||
|
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 revert these.
setupTagsField(); | ||
resolveStrings.selectedProperty().bindBidirectional(viewModel.resolveStringsProperty()); |
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.
Revert line break before line 71 and add one after 72.
timestampPreferences.setAddCreationDate(addCreationDateProperty.getValue()); | ||
timestampPreferences.setAddModificationDate(addModificationDateProperty.getValue()); | ||
fieldPreferences.getResolvableFields().clear(); | ||
fieldPreferences.getResolvableFields().addAll(resolveStringsForFieldsProperty.getValue()); |
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.
fieldPreferences.getResolvableFields().addAll(resolveStringsForFieldsProperty.getValue()); | |
fieldPreferences.getResolvableFields().addAll(resolveStringsForFieldsProperty.getValue().trim()); |
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.
The values are in list format, so can not trim them here.
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.
Use map to trim them individually then.
resolveStringsForFieldsProperty.getValue().stream()
.map(String::trim)
.forEach(fieldPreferences.getResolvableFields()::add);
fieldPreferences.getResolvableFields().clear(); | ||
fieldPreferences.getResolvableFields().addAll(resolveStringsForFieldsProperty.getValue()); | ||
fieldPreferences.getNonWrappableFields().clear(); | ||
fieldPreferences.getNonWrappableFields().addAll(nonWrappableFieldsProperty.getValue()); |
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.
fieldPreferences.getNonWrappableFields().addAll(nonWrappableFieldsProperty.getValue()); | |
fieldPreferences.getNonWrappableFields().addAll(nonWrappableFieldsProperty.getValue().trim()); |
private TextField keywordSeparator; | ||
@FXML | ||
private CheckBox resolveStrings; | ||
@FXML | ||
private TagsField<Field> resolveStringsForFields; | ||
@FXML | ||
private TagsField<Field> nonWrappableFields; |
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.
Separate these declarations into groups like before
|
||
@Override | ||
public Field fromString(String string) { | ||
return FieldFactory.parseField(string); |
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.
Why do we use parseField
instead of parseFieldList
now?
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.
As I was updating it according to the autocompletion module, and that uses parseFieldList.
nonWrappableFields.setShowSearchIcon(false); | ||
nonWrappableFields.setOnMouseClicked(event -> nonWrappableFields.getEditor().requestFocus()); | ||
nonWrappableFields.getEditor().getStyleClass().clear(); | ||
nonWrappableFields.getEditor().focusedProperty().addListener((observable, oldValue, newValue) -> nonWrappableFields.pseudoClassStateChanged(FOCUSED, newValue)); |
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.
nonWrappableFields.getEditor().focusedProperty().addListener((observable, oldValue, newValue) -> nonWrappableFields.pseudoClassStateChanged(FOCUSED, newValue)); | |
nonWrappableFields.getEditor().focusedProperty().addListener((_, _, newValue) -> nonWrappableFields.pseudoClassStateChanged(FOCUSED, newValue)); |
resolveStringsForFields.setShowSearchIcon(false); | ||
resolveStringsForFields.setOnMouseClicked(event -> resolveStringsForFields.getEditor().requestFocus()); | ||
resolveStringsForFields.getEditor().getStyleClass().clear(); | ||
resolveStringsForFields.getEditor().focusedProperty().addListener((observable, oldValue, newValue) -> resolveStringsForFields.pseudoClassStateChanged(FOCUSED, newValue)); |
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.
resolveStringsForFields.getEditor().focusedProperty().addListener((observable, oldValue, newValue) -> resolveStringsForFields.pseudoClassStateChanged(FOCUSED, newValue)); | |
resolveStringsForFields.getEditor().focusedProperty().addListener((_, _, newValue) -> resolveStringsForFields.pseudoClassStateChanged(FOCUSED, newValue)); |
<?import javafx.scene.control.*?> | ||
<?import javafx.scene.layout.*?> |
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.
Also, please don't use *
here. Import only what you're using.
@trag-bot didn't find any issues in the code! ✅✨ |
This PR Fixes #12550
This PR replaces the existing text-based field selection with a tag-based selection format, similar to the implementation in the Autocompletion settings.
Changes Made
Screenshots after Fix Applied
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)