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

heyitsdross · 2024-09-08T20:59:40Z

Draft PR for coding feedback

TODO (for me):

[] Update user documentation, open documentation PR.
[] Review developer documentation to see if update necessary. (I think it's unlikely).

To Test:

Toggle the Open- / LibreOffice view under View > OpenOffice/LibreOffice (or Alt + 0)
Click on "Select Style" in the OpenOffice menu which appears (assumedly on the left side of the window).
Choose any JStyle.
In the same area as "Select Style", click on "Settings"
Expected: The option "Automatically add "Cited on pages..." at beginning of citation" should appear unchecked.
Toggle the setting exactly once by clicking.
Fully close and restart the application.
Repeat steps 1 (if the OpenOffice view doesn't reopen) and 4.
Expected: The toggled option should still be checked.
Optionally repeat steps 6, then 1 and 4 again. The now twice toggled setting should again still be preserved (aka it should be unchecked).

Screenshot Preview of feature:

Mandatory checks

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

You can check review dog's comments at the tab "Files changed" of your pull request.

…text-setting

subhramit

Hey, thanks for taking out time to work on this.
You have gotten the preferences part right, as well as the UI (except for the change listener - check review comments).
Regarding the backend - the following variable is still always set to false, which still renders the functionality useless:

jabref/src/main/java/org/jabref/gui/openoffice/OOBibBase.java

Line 72 in 3df3b4f

private final boolean alwaysAddCitedOnPages; // TODO (see comment above)

Please update it's state according to the user's selection. Should be easy passing it to OOBibBase, see how the other settings are evaluated there. The issue references a PR where I worked on another setting, which was also always set to false before - you can take a hint from there.

I have also left some general comments. I also suggest going through the issue once again for more clarity.

subhramit · 2024-09-08T21:53:56Z

src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

-        OpenOfficePreferences openOfficePreferences = preferencesService.getOpenOfficePreferences();
-


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

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.

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.

subhramit · 2024-09-08T22:10:33Z

src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

@@ -657,4 +661,15 @@ private ContextMenu createSettingsPopup() {

        return contextMenu;
    }
+
+    private void addAlwaysCitedOnPagesTextOption(ContextMenu contextMenu) {
+        OpenOfficePreferences openOfficePreferences = preferencesService.getOpenOfficePreferences();


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.

subhramit · 2024-09-08T22:23:04Z

src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

+    private void addAlwaysCitedOnPagesTextOption(ContextMenu contextMenu) {
+        OpenOfficePreferences openOfficePreferences = preferencesService.getOpenOfficePreferences();
+
+        CheckMenuItem alwaysAddCitedOnPagesText = new CheckMenuItem(Localization.lang("Automatically add \"Cited on pages...\" at beginning of citation"));


"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.

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.

~~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.~~

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

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:

subhramit · 2024-09-08T22:25:29Z

src/main/resources/l10n/JabRef_en.properties

@@ -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


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

subhramit · 2024-09-08T22:58:37Z

src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

+        alwaysAddCitedOnPagesText.selectedProperty().set(openOfficePreferences.getAlwaysAddCitedOnPages());
+        alwaysAddCitedOnPagesText.setOnAction(e -> openOfficePreferences.setAlwaysAddCitedOnPages(alwaysAddCitedOnPagesText.isSelected()));
+
+        contextMenu.getItems().add(alwaysAddCitedOnPagesText);


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.

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.

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.

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.

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!

subhramit · 2024-09-08T23:20:06Z

[Unrelated to issue]

@InAnYan Hey Ruslan, although you've done most of it in #11727, I took a look at OpenOfficePanel, I think aiService is not required here as well?

jabref/src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

Line 98 in 3df3b4f

private final AiService aiService;

It is never used beyond:

jabref/src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

Line 126 in 3df3b4f

this.aiService = aiService;

If not needed, should we remove it?

subhramit · 2024-09-09T08:08:48Z

[Unrelated to issue]

@InAnYan Hey Ruslan, although you've done most of it in #11727, I took a look at OpenOfficePanel, I think aiService is not required here as well?

jabref/src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

Line 98 in 3df3b4f

private final AiService aiService;

It is never used beyond:

jabref/src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

Line 126 in 3df3b4f

this.aiService = aiService;

If not needed, should we remove it?

Removed in #11733

…text-setting

The new option's presence now based on selected style, with presence being ensured only if the style is a JStyle. The option now appears always first from the top, using index based insertion to achieve this. The text has been updated to reference bibliographic entries as opposed to citations, which was factually incorrect.

Reference existing preference in OOBibBase to properly propagate setting, so that "Cited on pages" text literal will appear when user has it configured. Remove accomplished TODOs.

src/main/java/org/jabref/gui/openoffice/OOBibBase.java

subhramit · 2024-09-12T00:07:46Z

Great work, Austin! Everything works now, tests are green too.
I took the opportunity to make some minor changes, you can check my commit (21c7de1). Mostly changed the dialog string, shifted the changelog entry down (we follow entries to be in chronological and sections in reverse chronological order) and some minor nitpicks.

Only one last thing to fix - you have accidentally committed submodules (see abbrv.jabref.org and csl-styles in changed files). To fix, follow:

$ git merge origin/main
$ git checkout main -- src/main/resources/csl-styles
$ ... git commit ... 
$ git push

And similarly for abbrv.jabref.org.

To avoid this: avoid using git add . to stage from CLI. Preferably use a GUI-based git manager, such as the one built in IntelliJ or open git gui from the command line. Even if you accidentally stage them, don't commit all files, selectively commit the files you touched using the GUI based tool, and push.

Context: JabRef needs submodules such as CSL style files for some of its respective features. When setting up a local development environment, these are cloned once using --recurse-submodules (you may have noticed). These submodules, in the main branch, are automatically periodically updated but not fetched into local again when you pull, as they are set to be ignored in .gitmodules (this is to avoid merge conflicts). So when remote has updated submodules, and your local has the old ones, when you stage, these changes are noticed.
What's strange (mostly an IntelliJ bug): Regardless of CLI or GUI, These changes should ideally not be noticed on staging, as per the .gitmodules configuration. However, that is somehow overruled when using IntelliJ's CLI.

…text-setting

Siedlerchr · 2024-09-13T20:53:07Z

except for the submodule stuff it looks good from my side! Thanks for your contribution and the hopefully onoging interest in JabRef!

…ays-add-cited-on-pages-text-setting

heyitsdross · 2024-09-13T21:28:25Z

Regarding the submodules:

The steps above don't seem to have removed? them from the changelist in my branch. What is the desired change I'm trying to enact here, a removal? And is there more in-depth reading I can use to get a better understanding of what I'm doing here?
Are the submodules not supposed to be checked in? I have myself little experience with them, execpt for one project at work, where git simply kept a commit hash in place of the files, which one could pull via git submodule update or similar.

subhramit · 2024-09-13T21:35:31Z

What is the desired change I'm trying to enact here, a removal?

Right now, the desired change would be that you sync the submodules in your PR (hence your branch) with the ones in Jabref:main right now. You can checkout these commit ids in blue:

Are the submodules not supposed to be checked in? I have myself little experience with them, execpt for one project at work, where git simply kept a commit hash in place of the files, which one could pull via git submodule update or similar.

Yes, git submodule update does update the submodules. But for committing them, you will have to go to .gitmodules and comment ou the line that sets them to ignored. After committing, revert the .gitmodules configuration, and push - everything should be synced then (that is one way to do it). I think @Siedlerchr might have a better answer to this.

Siedlerchr · 2024-09-13T21:51:54Z

The problem with submodules is often when you create a new branch with IntelliJ, it will create a new branch for the submodules as well. If you now fetch the latest updates in IntelliJ it will update the submodules as well and ignore the "pinned" commit from main. In the commit window, it will now list the changes to the submodules as well. One has to explicit uncheck them

TL;DR: reset submodules
edit .gitmodules
comment out ignore = all
#ignore = all

cd buildres/abbrv.jabref.org
git checkout 234cce8

same for csl styles:
cd src/main/resources/csl-styles
git checkout 4e0902d

use git gui to commit submodule changes only.
Afterwards revert the changes in .gitmodules

Siedlerchr · 2024-09-13T21:53:53Z

.gitmodules

@@ -1,15 +1,15 @@
 [submodule "abbrv.jabref.org"]
  path = buildres/abbrv.jabref.org
  url = https://github.com/JabRef/abbrv.jabref.org.git
-  ignore = all
+# ignore = all


see my latest comment for a TL;DR (git submodules are a pain in the a**)

heyitsdross · 2024-09-13T21:56:08Z

Thank you two very much for the help! I believe the submodule changes are synced again.

subhramit · 2024-09-13T21:59:26Z

@heyitsdross by the way, if you use "Closes" in your PR description, it is treated as a keyword and the issue is linked automatically.

heyitsdross · 2024-09-13T22:02:41Z

So that's how that works! Very nice :)

Siedlerchr · 2024-09-13T22:12:27Z

@heyitsdross Thanks again for your PR! Keep up the good work :)
Fixes and closes and some other keywords are possible https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests

heyitsdross added 4 commits September 8, 2024 22:40

Add setting to always add "Cited on pages" text in JStyle Citations.

68834e9

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

7243dda

Add changelog.

a5a902b

Add change to Changelog.

cda2669

github-actions bot reviewed Sep 8, 2024

View reviewed changes

Fix translation key. Ensure checkstyle conformity.

160721d

koppor requested review from subhramit and ThiloteE September 8, 2024 21:42

Merge branch 'main' into feature/11691-add-always-add-cited-on-pages-…

a4f05b1

…text-setting

subhramit requested changes Sep 8, 2024

View reviewed changes

subhramit added ui openoffice/libreoffice preferences labels Sep 9, 2024

heyitsdross added 3 commits September 11, 2024 20:22

Merge branch 'main' into feature/11691-add-always-add-cited-on-pages-…

3e507b8

…text-setting

Cited on pages... text now appears.

576eb92

Reference existing preference in OOBibBase to properly propagate setting, so that "Cited on pages" text literal will appear when user has it configured. Remove accomplished TODOs.

heyitsdross commented Sep 11, 2024

View reviewed changes

src/main/java/org/jabref/gui/openoffice/OOBibBase.java Show resolved Hide resolved

heyitsdross mentioned this pull request Sep 11, 2024

Document adding 'Cited on pages' in bibliography entry. JabRef/user-documentation#516

Merged

heyitsdross requested a review from subhramit September 11, 2024 22:01

Shift changelog entry, fix setting string, misc. formatting

21c7de1

Merge branch 'main' into feature/11691-add-always-add-cited-on-pages-…

9efdd4b

…text-setting

Merge remote-tracking branch 'origin/main' into feature/11691-add-alw…

dd273f7

…ays-add-cited-on-pages-text-setting

Attempt to update submodules to origin/master state.

ee2ebb4

Attempt to fix submodule hashes.

f2c3744

Siedlerchr reviewed Sep 13, 2024

View reviewed changes

heyitsdross added 2 commits September 13, 2024 23:54

Re-ignore submodules.

b43835a

Fix spacing.

7ce9dc8

Siedlerchr approved these changes Sep 13, 2024

View reviewed changes

subhramit approved these changes Sep 13, 2024

View reviewed changes

subhramit marked this pull request as ready for review September 13, 2024 21:57

subhramit enabled auto-merge September 13, 2024 21:57

subhramit added this pull request to the merge queue Sep 13, 2024

Merged via the queue into JabRef:main with commit 35b0516 Sep 13, 2024
23 checks passed

heyitsdross deleted the feature/11691-add-always-add-cited-on-pages-text-setting branch September 15, 2024 14:08

subhramit mentioned this pull request Sep 16, 2024

OO Maintenance: Better checks, clean up dead code, refactoring #11779

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

heyitsdross commented Sep 8, 2024 •

edited

Loading

github-actions bot left a comment

subhramit left a comment •

edited

Loading

subhramit Sep 8, 2024

heyitsdross Sep 9, 2024

subhramit Sep 9, 2024 •

edited

Loading

subhramit Sep 8, 2024 •

edited

Loading

subhramit Sep 8, 2024 •

edited

Loading

heyitsdross Sep 11, 2024 •

edited

Loading

ThiloteE Sep 13, 2024 •

edited

Loading

subhramit Sep 8, 2024

subhramit Sep 8, 2024 •

edited

Loading

heyitsdross Sep 9, 2024 •

edited

Loading

subhramit Sep 9, 2024

subhramit commented Sep 8, 2024 •

edited

Loading

subhramit commented Sep 9, 2024

subhramit commented Sep 12, 2024 •

edited

Loading

Siedlerchr commented Sep 13, 2024

heyitsdross commented Sep 13, 2024

subhramit commented Sep 13, 2024 •

edited

Loading

Siedlerchr commented Sep 13, 2024

Siedlerchr Sep 13, 2024

heyitsdross commented Sep 13, 2024

subhramit commented Sep 13, 2024 •

edited

Loading

heyitsdross commented Sep 13, 2024

Siedlerchr commented Sep 13, 2024

		OpenOfficePreferences openOfficePreferences = preferencesService.getOpenOfficePreferences();

		@@ -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

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

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

Conversation

heyitsdross commented Sep 8, 2024 • edited Loading

Draft PR for coding feedback

TODO (for me):

To Test:

Screenshot Preview of feature:

Mandatory checks

github-actions bot left a comment

Choose a reason for hiding this comment

subhramit left a comment • edited Loading

Choose a reason for hiding this comment

subhramit Sep 8, 2024

Choose a reason for hiding this comment

heyitsdross Sep 9, 2024

Choose a reason for hiding this comment

subhramit Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

subhramit Sep 8, 2024 • edited Loading

Choose a reason for hiding this comment

subhramit Sep 8, 2024 • edited Loading

Choose a reason for hiding this comment

heyitsdross Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

ThiloteE Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

subhramit Sep 8, 2024

Choose a reason for hiding this comment

subhramit Sep 8, 2024 • edited Loading

Choose a reason for hiding this comment

heyitsdross Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

subhramit Sep 9, 2024

Choose a reason for hiding this comment

subhramit commented Sep 8, 2024 • edited Loading

subhramit commented Sep 9, 2024

subhramit commented Sep 12, 2024 • edited Loading

Siedlerchr commented Sep 13, 2024

heyitsdross commented Sep 13, 2024

subhramit commented Sep 13, 2024 • edited Loading

Siedlerchr commented Sep 13, 2024

Siedlerchr Sep 13, 2024

Choose a reason for hiding this comment

heyitsdross commented Sep 13, 2024

subhramit commented Sep 13, 2024 • edited Loading

heyitsdross commented Sep 13, 2024

Siedlerchr commented Sep 13, 2024

heyitsdross commented Sep 8, 2024 •

edited

Loading

subhramit left a comment •

edited

Loading

subhramit Sep 9, 2024 •

edited

Loading

subhramit Sep 8, 2024 •

edited

Loading

subhramit Sep 8, 2024 •

edited

Loading

heyitsdross Sep 11, 2024 •

edited

Loading

ThiloteE Sep 13, 2024 •

edited

Loading

subhramit Sep 8, 2024 •

edited

Loading

heyitsdross Sep 9, 2024 •

edited

Loading

subhramit commented Sep 8, 2024 •

edited

Loading

subhramit commented Sep 12, 2024 •

edited

Loading

subhramit commented Sep 13, 2024 •

edited

Loading

subhramit commented Sep 13, 2024 •

edited

Loading