Functionality to modify CSL bibliography title and format #12784
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <?import javafx.scene.control.ButtonType?> | ||
| <?import javafx.scene.control.ComboBox?> | ||
| <?import javafx.scene.control.DialogPane?> | ||
| <?import javafx.scene.control.Label?> | ||
| <?import javafx.scene.control.TextField?> | ||
| <?import javafx.scene.layout.GridPane?> | ||
| <DialogPane xmlns:fx="http://javafx.com/fxml/1" | ||
| xmlns="http://javafx.com/javafx/8.0.121" fx:controller="org.jabref.gui.openoffice.ModifyCSLBibliographyTitleDialogView"> | ||
| <content> | ||
| <GridPane hgap="10.0" vgap="4.0"> | ||
| <Label text="%Bibliography title" GridPane.rowIndex="0" GridPane.columnIndex="0" /> | ||
| <TextField fx:id="titleField" prefWidth="300.0" GridPane.rowIndex="0" GridPane.columnIndex="1" /> | ||
| <Label text="%Header format" GridPane.rowIndex="1" GridPane.columnIndex="0" /> | ||
| <ComboBox fx:id="formats" prefWidth="300.0" GridPane.rowIndex="1" GridPane.columnIndex="1" /> | ||
| </GridPane> | ||
| </content> | ||
| <ButtonType fx:constant="OK"/> | ||
| </DialogPane> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| package org.jabref.gui.openoffice; | ||
|
|
||
| import javafx.fxml.FXML; | ||
| import javafx.scene.control.Button; | ||
| import javafx.scene.control.ButtonType; | ||
| import javafx.scene.control.ComboBox; | ||
| import javafx.scene.control.TextField; | ||
| import javafx.stage.Modality; | ||
|
|
||
| import org.jabref.gui.util.BaseDialog; | ||
| import org.jabref.gui.util.ViewModelListCellFactory; | ||
| import org.jabref.logic.l10n.Localization; | ||
| import org.jabref.logic.openoffice.OpenOfficePreferences; | ||
|
|
||
| import com.airhacks.afterburner.views.ViewLoader; | ||
|
|
||
| public class ModifyCSLBibliographyTitleDialogView extends BaseDialog<Void> { | ||
|
|
||
| @FXML private TextField titleField; | ||
| @FXML private ComboBox<String> formats; | ||
|
|
||
| private final ModifyCSLBibliographyTitleDialogViewModel viewModel; | ||
|
|
||
| public ModifyCSLBibliographyTitleDialogView(OpenOfficePreferences openOfficePreferences) { | ||
| this.viewModel = new ModifyCSLBibliographyTitleDialogViewModel(openOfficePreferences); | ||
|
|
||
| this.setTitle(Localization.lang("Modify bibliography title")); | ||
| this.initModality(Modality.NONE); | ||
| this.setResizable(false); | ||
|
|
||
| ViewLoader.view(this) | ||
| .load() | ||
| .setAsDialogPane(this); | ||
|
|
||
| Button okButton = (Button) getDialogPane().lookupButton(ButtonType.OK); | ||
| okButton.setOnAction(event -> { | ||
| viewModel.updateSettings(); | ||
| this.close(); | ||
| }); | ||
| } | ||
|
|
||
| @FXML | ||
| public void initialize() { | ||
| titleField.textProperty().bindBidirectional(viewModel.cslBibliographyTitleProperty()); | ||
|
|
||
| new ViewModelListCellFactory<String>() | ||
| .withText(format -> format) | ||
| .install(formats); | ||
| formats.itemsProperty().bind(viewModel.formatListProperty()); | ||
| formats.valueProperty().bindBidirectional(viewModel.cslBibliographySelectedHeaderFormatProperty()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package org.jabref.gui.openoffice; | ||
|
|
||
| import java.util.Arrays; | ||
|
|
||
| import javafx.beans.property.ReadOnlyListProperty; | ||
| import javafx.beans.property.ReadOnlyListWrapper; | ||
| import javafx.beans.property.SimpleStringProperty; | ||
| import javafx.beans.property.StringProperty; | ||
| import javafx.collections.FXCollections; | ||
|
|
||
| import org.jabref.logic.openoffice.OpenOfficePreferences; | ||
| import org.jabref.logic.openoffice.oocsltext.CSLFormatUtils; | ||
|
|
||
| public class ModifyCSLBibliographyTitleDialogViewModel { | ||
|
|
||
| private final StringProperty cslBibliographyTitle = new SimpleStringProperty(); | ||
| private final StringProperty cslBibliographySelectedHeaderFormat = new SimpleStringProperty(); | ||
| private final ReadOnlyListProperty<String> formatListProperty = | ||
| new ReadOnlyListWrapper<>(FXCollections.observableArrayList( | ||
| Arrays.stream(CSLFormatUtils.Format.values()).map(CSLFormatUtils.Format::getFormat).toList() | ||
| )); | ||
| private final OpenOfficePreferences openOfficePreferences; | ||
|
|
||
| public ModifyCSLBibliographyTitleDialogViewModel(OpenOfficePreferences openOfficePreferences) { | ||
| this.openOfficePreferences = openOfficePreferences; | ||
| this.cslBibliographyTitle.set(openOfficePreferences.getCslBibliographyTitle()); | ||
| this.cslBibliographySelectedHeaderFormat.set(openOfficePreferences.getCslBibliographyHeaderFormat()); | ||
|
|
||
| cslBibliographyTitle.bindBidirectional(openOfficePreferences.cslBibliographyTitleProperty()); | ||
| cslBibliographySelectedHeaderFormat.bindBidirectional(openOfficePreferences.cslBibliographyHeaderFormatProperty()); | ||
|
Comment on lines
+29
to
+30
Member
There was a problem hiding this comment. These direct bindings here are too sensitive - will capture and save updates regardless of whether a "save/ok" button is pressed, will be changing them in an upcoming PR. |
||
| } | ||
|
|
||
| public StringProperty cslBibliographyTitleProperty() { | ||
| return cslBibliographyTitle; | ||
| } | ||
|
|
||
| public StringProperty cslBibliographySelectedHeaderFormatProperty() { | ||
| return cslBibliographySelectedHeaderFormat; | ||
| } | ||
|
|
||
| public ReadOnlyListProperty<String> formatListProperty() { | ||
| return formatListProperty; | ||
| } | ||
|
|
||
| public void updateSettings() { | ||
| CSLFormatUtils.setBibliographyProperties(openOfficePreferences); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
|
|
||
| import org.jabref.logic.citationkeypattern.BracketedPattern; | ||
| import org.jabref.logic.citationstyle.CitationStyleOutputFormat; | ||
| import org.jabref.logic.openoffice.OpenOfficePreferences; | ||
| import org.jabref.model.database.BibDatabaseContext; | ||
| import org.jabref.model.entry.AuthorList; | ||
| import org.jabref.model.entry.BibEntry; | ||
|
|
@@ -25,15 +26,48 @@ | |
| */ | ||
| public class CSLFormatUtils { | ||
|
|
||
| public enum Format { | ||
|
Member
There was a problem hiding this comment. For which purpose this enum is used? I think styles can be chosen from this enum, but not limited to. Thus, I think a comment would look good here
Contributor
Author
There was a problem hiding this comment. Enum was initially used to pop up the dropdown list, as used in the codebase, to maintain consistency with existing practices.
Member
There was a problem hiding this comment. A simple list could be used too. An enum just seemed cleaner, and enums are generally used to denote "options" - so I didn't complain.
Member
There was a problem hiding this comment. @priyanshu16095 As @InAnYan suggested, in future if we let user enter their own text format, we will have to go out of this enum - but that will be some more work - and would need validation for being a valid format, otherwise error to user (else LO writing infrastructure will bleed), etc. |
||
| TITLE("Title"), | ||
| BODY_TEXT("Body Text"), | ||
| SUBTITLE("Subtitle"), | ||
| HEADING_1("Heading 1"), | ||
| HEADING_2("Heading 2"), | ||
| HEADING_3("Heading 3"), | ||
| HEADING_4("Heading 4"); | ||
|
|
||
| private final String format; | ||
|
|
||
| Format(String format) { | ||
| this.format = format; | ||
| } | ||
|
|
||
| public String getFormat() { | ||
| return format; | ||
| } | ||
| } | ||
|
|
||
| public static final CitationStyleOutputFormat OUTPUT_FORMAT = CitationStyleOutputFormat.HTML; | ||
|
|
||
| public static final String DEFAULT_BIBLIOGRAPHY_BODY_PARAGRAPH_FORMAT = "Body Text"; | ||
|
|
||
| private static String bibliographyTitle; | ||
| private static String bibliographyHeaderFormat; | ||
|
|
||
| private static final Pattern YEAR_IN_CITATION_PATTERN = Pattern.compile("(.)(.*), (\\d{4}.*)"); | ||
|
|
||
| public static String getBibliographyTitle() { | ||
| return bibliographyTitle; | ||
| } | ||
|
|
||
| public static String getBibliographyHeaderFormat() { | ||
| return bibliographyHeaderFormat; | ||
| } | ||
|
|
||
| public static void setBibliographyProperties(OpenOfficePreferences openOfficePreferences) { | ||
| bibliographyTitle = openOfficePreferences.getCslBibliographyTitle(); | ||
|
Member
There was a problem hiding this comment. This is not how we do it in JabRef. Why would you need to store these values in a static class? If someone needed to use
Member
There was a problem hiding this comment. This is a tricky one. The thing is - the values of these preferences are being taken and set from the UI. Here is some history for context:
I had to break the design when introducing Real time style switching just to have the last used style for comparison (if a CSL style): if (openOfficePreferences.getCurrentStyle() instanceof CitationStyle citationStyle) {
this.currentStyle = citationStyle;
}All other subsequent comparisions are done by storing state in the adapter itself. Now that you point it out, I realize that the preferences there could be cleaned up completely by passing the initial style instead of Like In this case I made Preferable - either break the design completely and directly use preferences in the adapter (as you suggest), or remove preferences completely from the adapter and do the same for these properties (seems weird, and you are right to question that this is not how we do it in JabRef - and that is because no other component in JabRef (that I know of) needs to interact with a continued running instance of an external application (in my knowledge), and thus doesn't need an adapter. I introduced it. Edit - I just realized this should go into an ADR.
Member
There was a problem hiding this comment. @InAnYan thought a lot over it, you're right. Short summary of original intent: 1. To be able to use these preferences statically without injection of Hence, I shift this to use them from |
||
| bibliographyHeaderFormat = openOfficePreferences.getCslBibliographyHeaderFormat(); | ||
| } | ||
|
Comment on lines
+66
to
+69
There was a problem hiding this comment. Method lacks JavaDoc documentation despite being a public API that manages important state. The method's behavior and side effects should be clearly documented. |
||
|
|
||
| /** | ||
| * Transforms provided HTML into a format that can be fully parsed and inserted into an OO document. | ||
| * Context: The HTML produced by {@link org.jabref.logic.citationstyle.CitationStyleGenerator#generateBibliography(List, String, CitationStyleOutputFormat, BibDatabaseContext, BibEntryTypesManager) generateBibliography} or {@link org.jabref.logic.citationstyle.CitationStyleGenerator#generateCitation(List, String, CitationStyleOutputFormat, BibDatabaseContext, BibEntryTypesManager) generateCitation} is not directly (completely) parsable by {@link OOTextIntoOO#write(XTextDocument, XTextCursor, OOText) write}. | ||
|
|
@@ -124,9 +158,9 @@ public static String generateAlphanumericInTextCitation(BibEntry entry, BibDatab | |
| String inTextCitation = generateAlphanumericCitation(List.of(entry), bibDatabaseContext); | ||
|
|
||
| String authorName = entry.getResolvedFieldOrAlias(StandardField.AUTHOR, bibDatabaseContext.getDatabase()) | ||
| .map(AuthorList::parse) | ||
| .map(list -> BracketedPattern.joinAuthorsOnLastName(list, 1, "", " et al.")) | ||
| .orElse(""); | ||
|
|
||
| return authorName + " " + inTextCitation; | ||
| } | ||
|
|
||
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 it should be looked up?
Can't it have an id and be used like
@FXML private Button okButton?Truly speaking, I haven't written dialogs in FXML. So maybe this is a common idiom?
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.
It has been a year for me as well since I have done JavaFX, but I only had some experience, so you are probably right. I also remember reading about the annotations in the docs - I can recall we follow it in JabRef.
@priyanshu16095 can you adjust using the fxml annotations and avoid the lookup? Just a small change, and would lead to neatness.
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.
Some background reading - https://devdocs.jabref.org/code-howtos/javafx.html#view---controller
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.
Can't it have an id and be used like @FXML private Button okButton?Yeah I did this in the
GUI for Consistency checkfeature.Uh oh!
There was an error while loading. Please reload this page.
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.
@FXML private Button okButtoncannot be used on a ButtonType, as in JavaFX DialogPane manages the button internally.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.
ah okay it won't work with the dialogpane, it's a standard added automatically as per the constant we specify in the fxml file (which was "CLOSE" before). you can leave this as it is then.
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.
Update (for learning) -
lookupButtoncan be avoided by usingsetResultConverter, example as present inStyleSelectDialogView.