Oobranch g : add actions by antalk2 · Pull Request #7792 · JabRef/jabref

antalk2 · 2021-06-03T12:32:58Z

Adds actions: GUI independent part of actions to be provided from the GUI.

Brings oobranch-E up to 89b0968 @ origin/improve-reversibility-rebased-03 Merge remote-tracking branch 'upstream/main' into improve-reversibility-rebased-03

commit cb13256 (HEAD -> improve-reversibility-rebased-03, origin/improve-reversibility-rebased-03) Author: Antal K <antalk2@gmail.com> Date: Fri Aug 20 11:39:39 2021 +0200 align dots

calixtus · 2021-10-25T21:02:30Z

Sorry @antalk2 for taking so long reading through your PRs.
Some of us were on vacation, some of us were extremly busy with life or had a lot of work to do for our day job.
I'll try to get into your PR the next days.

antalk2 · 2021-10-26T09:31:22Z

OK. Thank you.

calixtus · 2021-10-30T16:07:53Z

One general preliminary issue I see is that we don't have all the open office actions integrated in our command infrastructure, so we cannot see any telemetry on this or undo/redo this in the gui.
We should keep this in mind for later possible refactoring.

calixtus

Finally had time to do some reviewing. Got some smaller issues. Thanks for your work!

calixtus · 2021-11-06T14:47:46Z

+
+        List<String> citationKeys = OOListUtil.map(entries, EditInsert::insertEntryGetCitationKey);
+
+        final int nEntries = entries.size();


Please avoid abbreviations for variables.

Suggested change

final int nEntries = entries.size();

final int totalEntries = entries.size();

calixtus · 2021-11-06T16:35:50Z

+        OOText citeText =
+            (style.isNumberEntries()
+             ? OOText.fromString("[-]") // A dash only. Only refresh later.
+             : style.createCitationMarker(citations,
+                                          citationType.inParenthesis(),
+                                          NonUniqueCitationMarker.FORGIVEN));


On more complex if clauses, please avoid ? : for better readability

Changed to

OOText citeText = null; if (style.isNumberEntries()) { citeText = OOText.fromString("[-]"); // A dash only. Only refresh later. } else { citeText = style.createCitationMarker(citations, citationType.inParenthesis(), NonUniqueCitationMarker.FORGIVEN); }

calixtus · 2021-11-06T16:36:44Z

+                                          citationType.inParenthesis(),
+                                          NonUniqueCitationMarker.FORGIVEN));
+
+        if ("".equals(OOText.toString(citeText))) {


StringUtil.isBlank

calixtus · 2021-11-06T16:40:07Z

+
+            for (JoinableGroupData joinableGroupData : joinableGroups) {
+
+                List<CitationGroup> cgs = joinableGroupData.group;


Please avoid abbreviations

ok. Renamed to groups

calixtus · 2021-11-06T16:42:25Z

+
+        // assume: currentGroupCursor.getEnd() == cursorBetween.getEnd()
+        if (UnoTextRange.compareEnds(state.cursorBetween, state.currentGroupCursor) != 0) {
+            String msg = ("MergeCitationGroups: cursorBetween.end != currentGroupCursor.end");


Can be inlined.
Please avoid technical details in error messages, thats up for debug or logger. End user must be able to understand why this fails.

Changed to

LOGGER.warn("MergeCitationGroups: cursorBetween.end != currentGroupCursor.end (during expand)"); throw new IllegalStateException("MergeCitationGroups failed");

End user must be able to understand why this fails.

It would be nice, if I did. Failures around here happened due to
using visual order with two-column layout and/or viewing two pages side-by-side.
This could provide an order of groups that did not respect XText boundaries and/or the textual order within.
Now this should not happen, scan calls frontend.getCitationGroupsSortedWithinPartitions(doc, false) to
get the groups.

Turning on "Record changes" with or without "Show changes" can result in behaviour I do not really understand.
OOBibBase2.checkIfOpenOfficeIsRecordingChanges is intended to check against this.

Something else? There might be, I am not sure. I hope not, but testing anyway, so we get at least a warning
if the assumptions fail here.

In the end, I cannot tell why did the test fail, only that it did.

calixtus · 2021-11-06T16:52:11Z

+        frontend.citationGroups.lookupCitations(databases);
+        frontend.citationGroups.imposeLocalOrder(OOProcess.comparatorForMulticite(style));
+
+        List<CitationGroup> cgs = frontend.citationGroups.getCitationGroupsUnordered();


Please avoid abbreviations

changed cgs to groups

calixtus · 2021-11-06T16:52:26Z

+                                     .orElseThrow(IllegalStateException::new));
+                XTextCursor textCursor = range1.getText().createTextCursorByRange(range1);
+
+                List<Citation> cits = group.citationsInStorageOrder;


Please avoid abbreviations

cits -> citations
cit -> citation

calixtus · 2021-11-06T16:52:53Z

+        CitedKeys cks = frontend.citationGroups.getCitedKeysUnordered();
+        cks.lookupInDatabases(databases);


Please avoid abbreviations

calixtus · 2021-11-06T16:53:26Z

+        List<BibEntry> entriesToInsert = new ArrayList<>();
+        Set<String> seen = new HashSet<>(); // Only add crossReference once.
+
+        for (CitedKey ck : cks.values()) {


Suggested change

for (CitedKey ck : cks.values()) {

for (CitedKey citation : citationKeys.values()) {

calixtus · 2021-11-06T16:55:43Z

+        WrappedTargetException,
+        com.sun.star.lang.IllegalArgumentException {
+
+        final boolean useLockControllers = true;


If this is a debug constant, it should be

Suggested change

final boolean useLockControllers = true;

static final boolean USE_LOCK_CONTROLLERS = true;

Siedlerchr · 2021-11-14T19:33:24Z

Grobid test fails, not related

Siedlerchr · 2021-11-14T19:35:57Z

Sorry, that it took us so long to review your code. We will continue reviewing the other branches now

* upstream/main: (181 commits) Add of ADRs 22 and 23 (#8256) [Bot] Update CSL styles (#8245) Replace styfle/cancel-workflow-action@0.9.1 by GitHub's "concurrency" feature (#8243) Bump gittools/actions from 0.9.10 to 0.9.11 (#8248) Bump commons-cli from 1.4 to 1.5.0 (#8250) Bump byte-buddy-parent from 1.12.0 to 1.12.1 (#8249) Bump antlr4 from 4.9.2 to 4.9.3 (#8251) Bump archunit-junit5-api from 0.21.0 to 0.22.0 (#8252) Fix search: NOT binds more than AND (#8241) New Crowdin updates (#8240) Make PdfGrobiImporterTest as FetcherTest Oobranch g : add actions (#7792) Fix mixed CRLF / CR (#8238) Fix "Library has changed externally" with CRLF markers (#8239) Fix for issue 8198, 8133 (#8229) Added more unit tests in AuthorTest (#8214) Add confirmation dialog for empty entries in JabRef (#8218) Fix entry editor column visibility (#8232) Use regexp to remove non-ASCII characters from DOI and inform user when data for valid DOI does not exist #8127 (#8228) Fix exception for search flags (#8237) ...

antalk2 added 30 commits May 27, 2021 23:15

step0 : start model/openoffice, logic/openoffice/style

a50cc79

correction: import order

d93bd56

add general utilities

c9b72e9

add UNO utilities, move CreationException, NoDocumentException

381a549

Xlint:unchecked model/openoffice/util

f94c1f6

add ootext

f9a8176

add rangesort

6d013f1

add compareStartsUnsafe, compareStartsThenEndsUnsafe

0434579

add Tuple3

3c45a8a

add ootext

32f7a43

add rangesort

6bae6b4

delNamesArray size correction

314902c

rangeSort update

dfe18bc

Merge remote-tracking branch 'origin/oobranch-C' into oobranch-C

c9f4e57

cleanup

6e4c7c2

style additions

9388b2a

add backend

d2a53c7

add frontend

0fed618

add actions

3632d00

checkstyle on tests

88cab9c

add missing message

b7aaf2a

add backend

0fa9a4c

Merge remote-tracking branch 'origin/oobranch-E' into oobranch-E

946242f

add frontend

a5b7a05

Merge remote-tracking branch 'origin/oobranch-F' into oobranch-F

765f570

add actions

c50c458

Merge remote-tracking branch 'origin/oobranch-G' into oobranch-G

da39254

apply oobranch-[DEFG]-update.patch

18ef070

Merge remote-tracking branch 'upstream/main' into oobranch-G

46232ad

not using RangeSet

b9491d8

antalk2 added 19 commits July 18, 2021 17:21

checkstyle

5be1e74

use putIfAbsent

f9c0415

use "{}" with LOGGER

843b7ac

use Objects.hash and Objects.equals in CitationLookupResult

09f5503

simplified CitedKey.getBibEntry

d6a22be

more use of "{}" in LOGGER

8395e0c

more use of "{}" in LOGGER

0343d81

more use of "{}" in LOGGER

9890320

Citation.lookup: use streams

b34803f

Citation.lookup: Optional::get before findFirst

0490547

Merge remote-tracking branch 'upstream/main' into oobranch-G

fa87623

putIfAbsent returns null for new entry

dd0c039

What is 52 in Backend52

2f36705

apply 2021-08-20-a/oobranch-E-update.patch

d2cf373

Brings oobranch-E up to 89b0968 @ origin/improve-reversibility-rebased-03 Merge remote-tracking branch 'upstream/main' into improve-reversibility-rebased-03

sync to improve-reversibility-rebased-03 cb13256

952f08b

commit cb13256 (HEAD -> improve-reversibility-rebased-03, origin/improve-reversibility-rebased-03) Author: Antal K <antalk2@gmail.com> Date: Fri Aug 20 11:39:39 2021 +0200 align dots

Merge remote-tracking branch 'upstream/main' into oobranch-G

1663189

longer name

bf1a990

follow some PMD suggestions

2e80351

Merge remote-tracking branch 'upstream/main' into oobranch-G

79734fb

calixtus requested changes Nov 6, 2021

View reviewed changes

antalk2 added 2 commits November 9, 2021 09:29

Merge remote-tracking branch 'upstream/main' into oobranch-G

9fedc96

apply suggested changes

5272e98

Siedlerchr approved these changes Nov 14, 2021

View reviewed changes

Siedlerchr merged commit 5558408 into JabRef:main Nov 14, 2021


		List<String> citationKeys = OOListUtil.map(entries, EditInsert::insertEntryGetCitationKey);

		final int nEntries = entries.size();

	final int nEntries = entries.size();
	final int totalEntries = entries.size();


		for (JoinableGroupData joinableGroupData : joinableGroups) {

		List<CitationGroup> cgs = joinableGroupData.group;

		CitedKeys cks = frontend.citationGroups.getCitedKeysUnordered();
		cks.lookupInDatabases(databases);

	for (CitedKey ck : cks.values()) {
	for (CitedKey citation : citationKeys.values()) {

	final boolean useLockControllers = true;
	static final boolean USE_LOCK_CONTROLLERS = true;

Uh oh!

Conversation

antalk2 commented Jun 3, 2021

Uh oh!

calixtus commented Oct 25, 2021

Uh oh!

antalk2 commented Oct 26, 2021

Uh oh!

calixtus commented Oct 30, 2021

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antalk2 Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Nov 14, 2021

Uh oh!

Siedlerchr commented Nov 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

antalk2 Nov 9, 2021 •

edited

Loading