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.

Sign up for GitHub

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

Add setting: always add "Cited on pages" text to JStyles. #11732

Merged
merged 16 commits into from
Sep 13, 2024
Merged

Add setting: always add "Cited on pages" text to JStyles. #11732

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
68834e9
Add setting to always add "Cited on pages" text in JStyle Citations.
heyitsdross Sep 8, 2024
7243dda
Add setting: always add "Cited on pages" text to JStyles.
heyitsdross Sep 8, 2024
a5a902b
Add changelog.
heyitsdross Sep 8, 2024
cda2669
Add change to Changelog.
heyitsdross Sep 8, 2024
160721d
Fix translation key. Ensure checkstyle conformity.
heyitsdross Sep 8, 2024
a4f05b1
Merge branch 'main' into feature/11691-add-always-add-cited-on-pages-…
subhramit Sep 8, 2024
3e507b8
Merge branch 'main' into feature/11691-add-always-add-cited-on-pages-…
heyitsdross Sep 11, 2024
09d9647
Correct option text, position, & hiding behavior.
heyitsdross Sep 11, 2024
576eb92
Cited on pages... text now appears.
heyitsdross Sep 11, 2024
21c7de1
Shift changelog entry, fix setting string, misc. formatting
subhramit Sep 11, 2024
9efdd4b
Merge branch 'main' into feature/11691-add-always-add-cited-on-pages-…
subhramit Sep 13, 2024
dd273f7
Merge remote-tracking branch 'origin/main' into feature/11691-add-alw…
heyitsdross Sep 13, 2024
ee2ebb4
Attempt to update submodules to origin/master state.
heyitsdross Sep 13, 2024
f2c3744
Attempt to fix submodule hashes.
heyitsdross Sep 13, 2024
b43835a
Re-ignore submodules.
heyitsdross Sep 13, 2024
7ce9dc8
Fix spacing.
heyitsdross Sep 13, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv

### Added

- We added a setting which always adds the literal "Cited on pages" text before each JStyle citation. [#11691](https://github.com/JabRef/jabref/pull/11732)
- We added probable search hits instead of exact matches. Sorting by hit score can be done by the new score table column. [#11542](https://github.com/JabRef/jabref/pull/11542)
- We added support finding LaTeX-encoded special characters based on plain Unicode and vice versa. [#11542](https://github.com/JabRef/jabref/pull/11542)
- When a search hits a file, the file icon of that entry is changed accordingly. [#11542](https://github.com/JabRef/jabref/pull/11542)
Expand Down
21 changes: 18 additions & 3 deletions src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,12 +618,16 @@ private boolean checkThatEntriesHaveKeys(List<BibEntry> entries) {
}

private ContextMenu createSettingsPopup() {
OpenOfficePreferences openOfficePreferences = preferencesService.getOpenOfficePreferences();

Comment on lines -617 to -618
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why this was moved? Initializations are usually done at the beginning of the scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personal practice is to initialize as close to first usage as possible to increase refactorability, but if it's project standard to initialize at the beginning, then I will gladly undo.

Copy link
Member

@subhramit subhramit Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, there is no general hard-and-fast best practice regarding this. Beginning-of-scope-initialization is preferred when a dependency is used throughout the scope multiple times in logically segregated segments. Near-first-use-initialization is used if the usage is confined, and refactorability is indeed the argument.

I would have requested you to change back, but since moving it to the class level (my comment below on refactoring) will anyway do it at a more global level, this line will just be removed from here.

ContextMenu contextMenu = new ContextMenu();

if (currentStyle instanceof JStyle) {
addAlwaysCitedOnPagesTextOption(contextMenu);
}

OpenOfficePreferences openOfficePreferences = preferencesService.getOpenOfficePreferences();

CheckMenuItem autoSync = new CheckMenuItem(Localization.lang("Automatically sync bibliography when inserting citations"));
autoSync.selectedProperty().set(preferencesService.getOpenOfficePreferences().getSyncWhenCiting());
autoSync.selectedProperty().set(openOfficePreferences.getSyncWhenCiting());

ToggleGroup toggleGroup = new ToggleGroup();
RadioMenuItem useActiveBase = new RadioMenuItem(Localization.lang("Look up BibTeX entries in the active tab only"));
Expand Down Expand Up @@ -657,4 +661,15 @@ private ContextMenu createSettingsPopup() {

return contextMenu;
}

private void addAlwaysCitedOnPagesTextOption(ContextMenu contextMenu) {
OpenOfficePreferences openOfficePreferences = preferencesService.getOpenOfficePreferences();
Copy link
Member

@subhramit subhramit Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Additional request for refactor (since we're here): If we require openOfficePreferences inside multiple methods, we can declare it at class-level and call the getter just once.

Note: Actually, it is slightly bad design that the entire preferencesService is passed to this class' constructor (not your fault). Could be abstracted, but that's out of scope for this PR an would require more effort to change. You can just do what I mentioned above.


CheckMenuItem alwaysAddCitedOnPagesText = new CheckMenuItem(Localization.lang("Automatically add \"Cited on pages...\" at beginning of citation"));
Copy link
Member

@subhramit subhramit Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. "Citation" is the text (that is a pointer to a reference) we have in the document. "Bibliography" is the section where we have a list of the references used in the document. A reference could have been cited on multiple pages. So, "Cited on pages" should be there in a bibliographic entry, not a citation.

  2. Do you know how to use JabRef's LibreOffice plugin? If yes, can you check where the "Cited on pages" is actually showing up? If no, check out https://docs.jabref.org/cite/openofficeintegration. Ignore the properties part, use a default JStyle. @ThiloteE can also try this out if he has time. I am a bit unsure if "Cited on pages..." should actually come before or after an entry. Whatever it is, we don't have to change the functionality, only the text.

Tl;dr - change "citation" -> "bibliographic entries", "beginning of" -> (wherever it shows up)

Update: Can't test where it appears before work on the backend is complete.

Copy link
Contributor Author

@heyitsdross heyitsdross Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I (seemingly) managed to integrate my running JabRef program with LibreOffice, but the "Cited on pages" text didn't appear anywhere in the citation or bibliography.

image

This is the output I got when using the "Cite" button after selecting an entry in the list. Do I need to write in the actual addition of the text whenever the new setting is active, hence why it isn't appearing? I'm still not completely sure about the boundary between this ticket and what's already been done, but will continue to figure out what exactly is missing, barring input from your side.

Edit: Re-read your main review comment. Need to still update OOBibBase. Will do now.

Second edit: now looks like
image

Copy link
Member

@ThiloteE ThiloteE Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good job. It definitely should be at the end of the citation. "backref", which is part of the hyperref package offers the "cited on pages" feature for LaTeX documents, so that could serve as an inspiration. I do not remember exactly, but I believe I made some minor adjustments to the default backref settings, so that it always starts in a new line, which I think looks slick :-)

Here an example from a paper I wrote in LaTeX that includes backref:
image


alwaysAddCitedOnPagesText.selectedProperty().set(openOfficePreferences.getAlwaysAddCitedOnPages());
alwaysAddCitedOnPagesText.setOnAction(e -> openOfficePreferences.setAlwaysAddCitedOnPages(alwaysAddCitedOnPagesText.isSelected()));

contextMenu.getItems().add(alwaysAddCitedOnPagesText);
Copy link
Member

@subhramit subhramit Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Once added, this is never removed.
    In the issue, I had hinted on using a listener for the style type. We have to use it, otherwise once a JStyle is selected, this will always appear in the settings even if we select a CSL style afterwards.

  2. Can you add this below the option "Automatically sync bibliography when inserting citations" instead of above it? Reason: The pre-existing option should stay where it was, the new one should be added after it for the user's convenience.

  3. For design uniformity, you can also unify the code in this function into the existing createSettingsPopup(). Reason: Although delegating into separate functions makes code cleaner to read, each function of this class deals with a high-level activity.

Copy link
Contributor Author

@heyitsdross heyitsdross Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm really embarrassed I forgot about the CSL Styles, will add the removal mechanism and move the item below the "old" option as requested.

Copy link
Member

Choose a reason for hiding this comment

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

I'm really embarrassed I forgot about the CSL Styles, will add the removal mechanism and move the item below the "old" option as requested.

Please be comfortable, you are new to the codebase, so that's very normal!

}
}
16 changes: 15 additions & 1 deletion src/main/java/org/jabref/logic/openoffice/OpenOfficePreferences.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,21 @@ public class OpenOfficePreferences {
private final ObservableList<String> externalStyles;
private final StringProperty currentJStyle;
private final ObjectProperty<OOStyle> currentStyle;
private final BooleanProperty alwaysAddCitedOnPages;

public OpenOfficePreferences(String executablePath,
boolean useAllDatabases,
boolean syncWhenCiting,
List<String> externalStyles,
String currentJStyle,
OOStyle currentStyle) {
OOStyle currentStyle, boolean alwaysAddCitedOnPages) {
this.executablePath = new SimpleStringProperty(executablePath);
this.useAllDatabases = new SimpleBooleanProperty(useAllDatabases);
this.syncWhenCiting = new SimpleBooleanProperty(syncWhenCiting);
this.externalStyles = FXCollections.observableArrayList(externalStyles);
this.currentJStyle = new SimpleStringProperty(currentJStyle);
this.currentStyle = new SimpleObjectProperty<>(currentStyle);
this.alwaysAddCitedOnPages = new SimpleBooleanProperty(alwaysAddCitedOnPages);
}

public void clearConnectionSettings() {
Expand Down Expand Up @@ -137,4 +139,16 @@ public ObjectProperty<OOStyle> currentStyleProperty() {
public void setCurrentStyle(OOStyle style) {
this.currentStyle.set(style);
}

public boolean getAlwaysAddCitedOnPages() {
return this.alwaysAddCitedOnPages.get();
}

public BooleanProperty alwaysAddCitedOnPagesProperty() {
return this.alwaysAddCitedOnPages;
}

public void setAlwaysAddCitedOnPages(boolean alwaysAddCitedOnPages) {
this.alwaysAddCitedOnPages.set(alwaysAddCitedOnPages);
}
}
6 changes: 5 additions & 1 deletion src/main/java/org/jabref/preferences/JabRefPreferences.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,7 @@ public class JabRefPreferences implements PreferencesService {
public static final String OO_BIBLIOGRAPHY_STYLE_FILE = "ooBibliographyStyleFile";
public static final String OO_EXTERNAL_STYLE_FILES = "ooExternalStyleFiles";
public static final String OO_CURRENT_STYLE = "ooCurrentStyle";
public static final String OO_ALWAYS_ADD_CITED_ON_PAGES = "ooAlwaysAddCitedOnPages";

// Special field preferences
public static final String SPECIALFIELDSENABLED = "specialFieldsEnabled";
Expand Down Expand Up @@ -756,6 +757,7 @@ private JabRefPreferences() {
}

defaults.put(OO_SYNC_WHEN_CITING, Boolean.TRUE);
defaults.put(OO_ALWAYS_ADD_CITED_ON_PAGES, Boolean.FALSE);
defaults.put(OO_SHOW_PANEL, Boolean.FALSE);
defaults.put(OO_USE_ALL_OPEN_BASES, Boolean.TRUE);
defaults.put(OO_BIBLIOGRAPHY_STYLE_FILE, StyleLoader.DEFAULT_AUTHORYEAR_STYLE_PATH);
Expand Down Expand Up @@ -1377,10 +1379,12 @@ public OpenOfficePreferences getOpenOfficePreferences() {
getBoolean(OO_SYNC_WHEN_CITING),
getStringList(OO_EXTERNAL_STYLE_FILES),
get(OO_BIBLIOGRAPHY_STYLE_FILE),
currentStyle);
currentStyle,
getBoolean(OO_ALWAYS_ADD_CITED_ON_PAGES));

EasyBind.listen(openOfficePreferences.executablePathProperty(), (obs, oldValue, newValue) -> put(OO_EXECUTABLE_PATH, newValue));
EasyBind.listen(openOfficePreferences.useAllDatabasesProperty(), (obs, oldValue, newValue) -> putBoolean(OO_USE_ALL_OPEN_BASES, newValue));
EasyBind.listen(openOfficePreferences.alwaysAddCitedOnPagesProperty(), (obs, oldValue, newValue) -> putBoolean(OO_ALWAYS_ADD_CITED_ON_PAGES, newValue));
EasyBind.listen(openOfficePreferences.syncWhenCitingProperty(), (obs, oldValue, newValue) -> putBoolean(OO_SYNC_WHEN_CITING, newValue));

openOfficePreferences.getExternalStyles().addListener((InvalidationListener) change ->
Expand Down
1 change: 1 addition & 0 deletions src/main/resources/l10n/JabRef_en.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,6 +1132,7 @@ Unable\ to\ reload\ style\ file=Unable to reload style file

Problem\ during\ separating\ cite\ markers=Problem during separating cite markers

Automatically\ add\ "Cited\ on\ pages..."\ at\ beginning\ of\ citation=Automatically add "Cited on pages..." at beginning of citation
Copy link
Member

Choose a reason for hiding this comment

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

Text needs to be changed (see comment above for line 668).

Automatically\ sync\ bibliography\ when\ inserting\ citations=Automatically sync bibliography when inserting citations
Look\ up\ BibTeX\ entries\ in\ the\ active\ tab\ only=Look up BibTeX entries in the active tab only
Look\ up\ BibTeX\ entries\ in\ all\ open\ libraries=Look up BibTeX entries in all open libraries
Expand Down
Loading