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.

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

Oobranch g : add actions #7792

Merged
merged 61 commits into from
Nov 14, 2021
Merged

Oobranch g : add actions #7792

merged 61 commits into from
Nov 14, 2021

Conversation

antalk2
Copy link
Contributor

@antalk2 antalk2 commented Jun 3, 2021

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

@calixtus
Copy link
Member

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
Copy link
Contributor Author

antalk2 commented Oct 26, 2021

OK. Thank you.

@calixtus
Copy link
Member

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.

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

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


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

final int nEntries = entries.size();
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid abbreviations for variables.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Comment on lines 88 to 93
OOText citeText =
(style.isNumberEntries()
? OOText.fromString("[-]") // A dash only. Only refresh later.
: style.createCitationMarker(citations,
citationType.inParenthesis(),
NonUniqueCitationMarker.FORGIVEN));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
        }

citationType.inParenthesis(),
NonUniqueCitationMarker.FORGIVEN));

if ("".equals(OOText.toString(citeText))) {
Copy link
Member

Choose a reason for hiding this comment

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

StringUtil.isBlank


for (JoinableGroupData joinableGroupData : joinableGroups) {

List<CitationGroup> cgs = joinableGroupData.group;
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid abbreviations

Copy link
Contributor Author

@antalk2 antalk2 Nov 9, 2021

Choose a reason for hiding this comment

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

ok. Renamed to groups


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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

frontend.citationGroups.lookupCitations(databases);
frontend.citationGroups.imposeLocalOrder(OOProcess.comparatorForMulticite(style));

List<CitationGroup> cgs = frontend.citationGroups.getCitationGroupsUnordered();
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid abbreviations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed cgs to groups

.orElseThrow(IllegalStateException::new));
XTextCursor textCursor = range1.getText().createTextCursorByRange(range1);

List<Citation> cits = group.citationsInStorageOrder;
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid abbreviations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cits -> citations
cit -> citation

Comment on lines 55 to 56
CitedKeys cks = frontend.citationGroups.getCitedKeysUnordered();
cks.lookupInDatabases(databases);
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid abbreviations

List<BibEntry> entriesToInsert = new ArrayList<>();
Set<String> seen = new HashSet<>(); // Only add crossReference once.

for (CitedKey ck : cks.values()) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

WrappedTargetException,
com.sun.star.lang.IllegalArgumentException {

final boolean useLockControllers = true;
Copy link
Member

Choose a reason for hiding this comment

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

If this is a debug constant, it should be

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@Siedlerchr
Copy link
Member

Grobid test fails, not related

@Siedlerchr Siedlerchr merged commit 5558408 into JabRef:main Nov 14, 2021
@Siedlerchr
Copy link
Member

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

Siedlerchr added a commit that referenced this pull request Nov 19, 2021
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants