Unmerge LO citations #7455

antalk2 · 2021-02-18T23:02:14Z

Implemented "Unmerge citations" in the Openoffice/Libreoffice
integration panel.
Fixes #7454(#7454)

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)

Tests created for changes (if applicable)
No, tests were not created. Do you have GUI tests?

There is a known problem: "Merge citations" does not save the information needed
to find the optional extra info (like page reference) stored in document properties.
When reestablishing the individual references in unCombineCiteMarkers,
getUniqueReferenceMarkName() may or may not return the original name.
When it does not, the extra info may be lost, or even taken from another entry.

A solution could be to encode the number making the originals unique
in the merged name: in stead of  "JR_cite_1_XX2000a,YY2010" it would be e.g.
"JR_cite_1_XX2000a,1_YY2010". Apart from construction and parsing,
 probably marking (or calulating) the originals as "in use" for getUniqueReferenceMarkName()
 would be needed.

 Another problem: combineCiteMarkers orders the merged entries by year  (I am not sure why).
 The original order is lost.

 In summary, the presented solution, although improves the situation, does not cover
 all aspects.

Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)

Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

https://docs.jabref.org/cite/openofficeintegration does not mention the "Merge citations" button.
The most appropriate addition would probably go under the "Known issues" section,
describing the two problems above (or the original, if the proposed changes are not applied)

Siedlerchr · 2021-02-21T16:36:11Z

Thanks for your contribution. Codewise looks good to me so far.
I think it would make sense to change the cite generation to encode the unique thing when combining, but then the parse ref Names needs to be adjusted as well and many other cases as well.

antalk2 · 2021-02-22T10:44:57Z

I think it would make sense to change the cite generation to encode the unique thing when combining, but then the parse ref Names needs to be adjusted as well and many other cases as well.

I started a new branch to reorganize OOBibBase.java, at https://github.com/antalk2/jabref/tree/improve-reversibility

So far it should contain no changes to user-visible functionality,
mainly reorganization of the code. Parsing of ref marks is now only in parseRefMarkName
and isJabRefReferenceMarkName

On the other hand, reorganizing means lots of changes,
many of which may seem or turn out to be superfluous,
since I am editing while trying to understand what is going on.

I am worried, that with so many changes I end up with
something that will not be merged, due to the amount of changes.

Please let me know what to expect and what to watch out for.

calixtus · 2021-02-22T12:15:07Z

Probably the best would be to merge this one and you keep working on your branch. But try to focus on one issue change per commit, so we can understand what you changed, commit by commit. You can also label your PR as WIP and draft, growing as you work on it, and we check regularly in and have a look at your changes.
You can even comment on your own code in the files changed tab, so you can discuss your changes in that PR with us.

Don't worry, working on JabRef together should be fun. We don't bite. 😄

calixtus

Just a few quick nitpicks I saw.

calixtus · 2021-02-22T12:17:12Z

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

+            UndefinedCharacterFormatException, UnknownPropertyException, PropertyVetoException, CreationException,
+            BibEntryNotFoundException {
+        XNameAccess nameAccess = getReferenceMarks();
+        // TODO: doesn't work for citations in footnotes/tables


Are you going to work on this todo?

This one is not really mine,

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

Line 1189 in 3dc2039

// TODO: doesn't work for citations in footnotes/tables

, just copied with the code of combineCiteMarkers.
There is some code in getReferenceMarks concerning footnotes, but not tables.

I did insert some TODO notes, however.

Some of these are questions, like: https://github.com/antalk2/jabref/blob/ff1bc8933e3b05630633ef262e8b6f7928446ba9/src/main/java/org/jabref/gui/openoffice/OOBibBase.java#L184, where I am not sure
if the code is there for a purpose (e.g. to provoke an exception if some assumption fails) or just forgotten.

calixtus · 2021-02-22T12:17:33Z

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

+                .getAnchor();
+
+            XTextCursor mxDocCursor = range1.getText().createTextCursorByRange(range1);
+            //


Empty comment line can be removed

calixtus · 2021-02-22T12:17:50Z

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

+            List<String> keys = parseRefMarkName(names.get(piv));
+            if (keys.size() > 1) {
+                removeReferenceMark(names.get(piv));
+                //


Empty comment line

calixtus · 2021-02-22T12:19:12Z

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

+
+        final XTextRangeCompare compare = UnoRuntime.queryInterface(XTextRangeCompare.class, text);
+
+        int piv = 0;


Please try to avoid abbreviations a variable names. Use full length names, maybe in camelCase.

Do you mean piv should be pivot or pivotElement?
In https://github.com/antalk2/jabref/blob/ff1bc8933e3b05630633ef262e8b6f7928446ba9/src/main/java/org/jabref/gui/openoffice/OOBibBase.java#L1995 I renamed text to xtext, and expanded to this.xtext
to clarifiy its scope. The local variables compare and piv seemed OK.
If there is a part of the project that could serve as an example please point me there,
it might help to pick up the conventions.

Yes, pivot is better. We usually also don't prefix variables with their type, so instead of mxDocCursor documentCursor (or textCursor?) would be preferred. Similarily, bName below can simply be name or if you want to be verbose markName.

antalk2 · 2021-02-22T14:47:52Z

Probably the best would be to merge this one and you keep working on your branch. But try to focus on one issue change per commit, so we can understand what you changed, commit by commit. You can also label your PR as WIP and draft, growing as you work on it, and we check regularly in and have a look at your changes.
You can even comment on your own code in the files changed tab, so you can discuss your changes in that PR with us.

Don't worry, working on JabRef together should be fun. We don't bite. smile

tobiasdiez · 2021-02-25T20:30:07Z

@antalk2 what do you think about merging this one first, and then having the further refactoring an a second PR?

antalk2 · 2021-02-26T07:33:01Z

@antalk2 what do you think about merging this one first, and then having the further refactoring an a second PR?

Yes. I think this is better as a separate merge.
Do I need to do something for that?

I am doing the refactoring in a separate branch.

tobiasdiez · 2021-02-26T08:43:22Z

src/main/resources/l10n/JabRef_en.properties

@@ -2282,6 +2282,9 @@ No\ metadata\ found.\ Creating\ empty\ entry\ with\ file\ link=No metadata found
 Processing\ file\ %0=Processing file %0
 Export\ selected=Export selected

+UnCombine\ merged\ citations=UnCombine merged citations


Uncombine seems to be a proper english word. So the inword capitialization is not needed. But maybe separate instead of uncombine and unmerge would be better?

Merge citations | Unmerge citations
Join citations | Separate citations

I would like to make it clear that the two buttons correspond,
hence came Unmerge.

To Separate I associate Join as the opposite.
(English is not my primary language)

Which pair (of the above, or something else) would you prefer?

I'm not familiar enough with the feature, but I would say join/separate are a bit clearer / non-technical terms. Maybe @Siedlerchr has a preference.

It does a bit more than just putting them in a pair of shared parentheses:
The entries under the same reference mark are sorted according to comparatorForMulticite
And there is a comment at getCitationMarker saying:
// // E.g. (Olsen, 2005a, b) should be output instead of (Olsen, 2005a; Olsen, 2005b).

I've got no strong opinion, but think separate/merge is also suited

tobiasdiez · 2021-02-26T08:44:01Z

Ok, I've reopened this PR then. If you could address the small comments by @calixtus and me, then we can merge. Thanks!

antalk2 · 2021-02-26T11:14:05Z

Ok, I've reopened this PR then. If you could address the small comments by @calixtus and me, then we can merge. Thanks!

I believe I did that. Let me know if I left out something.

antalk2 · 2021-02-26T18:33:21Z

Style checks (reviewdog) often ask me to make the code less readable.
Shall I ignore it, or follow its suggestions anyway?

Siedlerchr · 2021-02-26T18:48:38Z

The style check is important to have consistency across the repository and to prevent merge conflicts of unrelated changes e.g. whitespaces or line breaks. You can import JabRef's code style in intellij and do a kind of
https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace#configure-your-ide

antalk2 · 2021-02-26T20:12:39Z

The style check is important to have consistency across the repository and to prevent merge conflicts of unrelated changes e.g. whitespaces or line breaks.

This sounds like "follow its suggestions anyway". OK, I will try that.

calixtus · 2021-02-27T06:24:03Z

Well, we configured reviewdog to automate the suggestions, otherwise we would make. What suggestions in particular do you think makes the code less readable?

antalk2 · 2021-02-27T10:13:23Z

Well, we configured review dog to automate the suggestions,
otherwise we would make. What suggestions in particular do you think
makes the code less readable?

No whitespace in sin(x) sounds OK, but

   longVariableName.longMethodname(another.long(something))

is harder to read than

   longVariableName.longMethodname( another.long(something) )

Style checker documentation says:

Abstract class for checking the padding of parentheses. That is
whether a space is required after a left parenthesis and before a
right parenthesis, or such spaces are forbidden.

Apparently they support both required and forbidden (and implicitly, noCheck)

Avoid nested blocks

Rationale:
Nested blocks are often leftovers from the debugging process, they
confuse the reader.

Inner blocks are useful in restricting scope of local variables,
assuring the reader they are not referred to later.
Current style checks disallow this.

   void f(x) {
       ...
       String[][] bibtexkeys;
       {
           Some localvar ... // used to calculate initial value for bibtexkeys
           ...
           bibtexkeys = ...
       }
       ... more code here: does it use localvar?
   }

Style checker seems to prefer this:

   void f(x) {
       ...
       String[][] bibtexkeys;
       Some localvar ... // used to calculate initial value for bibtexkeys
           ...
       bibtexkeys = ...
       ... more code here: does it use localvar?
   }

Line up, for example assignments, variable declatarions (style checker insists on single space)

assert another.length == field.length;
assert anEvenLongerName == field.length;
this.field = field;
this.another = another;
this.anEvenLongerName = anEvenLongerName;

or

assert another.length   == field.length;
assert anEvenLongerName == field.length;
this.field            = field;
this.another          = another;
this.anEvenLongerName = anEvenLongerName;

I know, when this.anEvenLongerNameTheSecond arrives, we either loose
the line up, or change unrelated lines, leading to potential merge
problems. Google is more permissive here than
the style checker in JabRef.

Where should the brace go?

Easy

void f(x) {  //<- brace here, ok
   body here
}

With a longer head brace on next line could improve visual separation of head and body.

void f(x,
       y,
       z)
      throws XException,
             YException,
             ZException {  //<- brace here according to style
   body here
}

void f(x,
       y,
       z)
      throws XException,
             YException,
             ZException
{
   body here
}

calixtus · 2021-02-27T11:31:31Z

Thanks for your answer. I think we (@JabRef/developers) maybe talk about these considerations in the next devcall.

stefan-kolb · 2021-02-27T13:05:18Z

Well, we configured review dog to automate the suggestions,
otherwise we would make. What suggestions in particular do you think
makes the code less readable?

No whitespace in sin(x) sounds OK, but
   longVariableName.longMethodname(another.long(something))
is harder to read than
   longVariableName.longMethodname( another.long(something) )
Style checker documentation says:

Abstract class for checking the padding of parentheses. That is
whether a space is required after a left parenthesis and before a
right parenthesis, or such spaces are forbidden.

Apparently they support both required and forbidden (and implicitly, noCheck)

Avoid nested blocks

Rationale:
Nested blocks are often leftovers from the debugging process, they
confuse the reader.

Inner blocks are useful in restricting scope of local variables,
assuring the reader they are not referred to later.
Current style checks disallow this.
   void f(x) {
       ...
       String[][] bibtexkeys;
       {
           Some localvar ... // used to calculate initial value for bibtexkeys
           ...
           bibtexkeys = ...
       }
       ... more code here: does it use localvar?
   }
Style checker seems to prefer this:
   void f(x) {
       ...
       String[][] bibtexkeys;
       Some localvar ... // used to calculate initial value for bibtexkeys
           ...
       bibtexkeys = ...
       ... more code here: does it use localvar?
   }
Line up, for example assignments, variable declatarions (style checker insists on single space)
assert another.length == field.length;
assert anEvenLongerName == field.length;
this.field = field;
this.another = another;
this.anEvenLongerName = anEvenLongerName;
or
assert another.length   == field.length;
assert anEvenLongerName == field.length;
this.field            = field;
this.another          = another;
this.anEvenLongerName = anEvenLongerName;
I know, when this.anEvenLongerNameTheSecond arrives, we either loose
the line up, or change unrelated lines, leading to potential merge
problems. Google is more permissive here than
the style checker in JabRef.
Where should the brace go?
Easy
void f(x) {  //<- brace here, ok
   body here
}
With a longer head brace on next line could improve visual separation of head and body.
void f(x,
       y,
       z)
      throws XException,
             YException,
             ZException {  //<- brace here according to style
   body here
}
void f(x,
       y,
       z)
      throws XException,
             YException,
             ZException
{
   body here
}

I'm sorry but I disagree with all of your suggestions and prefer the current JabRef code style. @JabRef/developers

it is better to keep params short, so you could also extract the long param to a var.
totally agree with the given rationale why inner braces should not be used. your case to restrict use of vars should be a rare exception and you're better of to extract that into a function then
might look nice but leads to a lot of refactorings when you add a new assignment. we wanted to kep the number of "unrelated" line changes in diffs low to make it easier to review PRs
of course only a preference, but we stick with the standard Java code style here to add the brace in the same line. might be too restrictive in oyur example, but also here we tried to keep the number of rules low and simple without exceptions.

antalk2 · 2021-02-27T20:19:17Z

I'm sorry but I disagree with all of your suggestions and prefer the current JabRef code style. @JabRef/developers

It is OK, I do not expect or suggest to change JabRef code style, just shared my preferences.

I accept the rules, even if I do not always think they are optimal.
I am not a maintainer (here or elsewhere), thus probably underestimate the probability and burden of merging conflicts. I am aware of this, so I do not intend to shape your code style to my preferences.

antalk2 · 2021-02-27T21:55:04Z

You can import JabRef's code style in intellij and ...

Thank you for this suggestion. Installing IDEA helped a lot to find a style
accepted by the checker.

Siedlerchr · 2021-03-01T19:26:02Z

Thanks again for your contribution, I tested your feature with both an author year and the numerical style and it works.

calixtus

Codewise looks good to me!

calixtus · 2021-03-02T20:20:49Z

Checks are good, two approvals, @Siedlerchr confirmed its working. Thanks for your great contribution!

antalk2 added 4 commits February 18, 2021 22:44

implement Unmerge citations for Openoffice integration

f03fbcb

New text in src/main/resources/l10n/JabRef_en.properties

2e08d21

add CHANGELOG.md line

ec6f0ba

reviewdog

bab2aa3

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 19, 2021

reviewdog

31e01e0

tobiasdiez requested a review from Siedlerchr February 21, 2021 10:57

Siedlerchr changed the title ~~Fix 7454~~ Unmerge LO citations Feb 21, 2021

Siedlerchr previously approved these changes Feb 21, 2021

View reviewed changes

calixtus reviewed Feb 22, 2021

View reviewed changes

antalk2 closed this Feb 22, 2021

tobiasdiez reopened this Feb 26, 2021

tobiasdiez reviewed Feb 26, 2021

View reviewed changes

antalk2 added 5 commits February 26, 2021 10:38

removed duplicate TODO

0eb17f4

remove empty comments

8b096e3

piv renamed to pivot

148928f

mxDocCursor renamed to textCursor

44c1b55

bName renamed to newName

99cef09

antalk2 dismissed Siedlerchr’s stale review via 99cef09 February 26, 2021 10:36

antalk2 added 2 commits February 26, 2021 11:58

"UnMerge citations" changed to "Separate citations"

ea346a2

"UnCombine merged citations" changed to "Separate merged citations"

3c7ee63

calixtus assigned Siedlerchr Mar 1, 2021

Merge branch 'master' into fix-7454

614d960

Siedlerchr approved these changes Mar 1, 2021

View reviewed changes

calixtus approved these changes Mar 2, 2021

View reviewed changes

calixtus merged commit b856dd4 into JabRef:master Mar 2, 2021

antalk2 mentioned this pull request Mar 8, 2021

WIP: Improve reversibility #7469

Closed

5 tasks

k3KAW8Pnf7mkmdSMPHz27 mentioned this pull request May 7, 2021

WIP: Improve reversibility rebased 03 JabRef/jabref-koppor#496

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unmerge LO citations #7455

Unmerge LO citations #7455

antalk2 commented Feb 18, 2021

Siedlerchr commented Feb 21, 2021

antalk2 commented Feb 22, 2021

calixtus commented Feb 22, 2021

calixtus left a comment

calixtus Feb 22, 2021

antalk2 Feb 22, 2021

calixtus Feb 22, 2021

calixtus Feb 22, 2021

calixtus Feb 22, 2021

antalk2 Feb 22, 2021

tobiasdiez Feb 26, 2021

antalk2 commented Feb 22, 2021

tobiasdiez commented Feb 25, 2021

antalk2 commented Feb 26, 2021

tobiasdiez Feb 26, 2021

antalk2 Feb 26, 2021

tobiasdiez Feb 26, 2021

antalk2 Feb 26, 2021

Siedlerchr Feb 26, 2021

tobiasdiez commented Feb 26, 2021

antalk2 commented Feb 26, 2021

antalk2 commented Feb 26, 2021

Siedlerchr commented Feb 26, 2021

antalk2 commented Feb 26, 2021

calixtus commented Feb 27, 2021 •

edited

Loading

antalk2 commented Feb 27, 2021

calixtus commented Feb 27, 2021

stefan-kolb commented Feb 27, 2021

antalk2 commented Feb 27, 2021

antalk2 commented Feb 27, 2021

Siedlerchr commented Mar 1, 2021

calixtus left a comment

calixtus commented Mar 2, 2021


		final XTextRangeCompare compare = UnoRuntime.queryInterface(XTextRangeCompare.class, text);

		int piv = 0;

Unmerge LO citations #7455

Unmerge LO citations #7455

Conversation

antalk2 commented Feb 18, 2021

Siedlerchr commented Feb 21, 2021

antalk2 commented Feb 22, 2021

calixtus commented Feb 22, 2021

calixtus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalk2 commented Feb 22, 2021

tobiasdiez commented Feb 25, 2021

antalk2 commented Feb 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez commented Feb 26, 2021

antalk2 commented Feb 26, 2021

antalk2 commented Feb 26, 2021

Siedlerchr commented Feb 26, 2021

antalk2 commented Feb 26, 2021

calixtus commented Feb 27, 2021 • edited Loading

antalk2 commented Feb 27, 2021

calixtus commented Feb 27, 2021

stefan-kolb commented Feb 27, 2021

antalk2 commented Feb 27, 2021

antalk2 commented Feb 27, 2021

Siedlerchr commented Mar 1, 2021

calixtus left a comment

Choose a reason for hiding this comment

calixtus commented Mar 2, 2021

calixtus commented Feb 27, 2021 •

edited

Loading