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

Fix 11102 by allowing users to add local bst files to preview layout list #11234

Merged
merged 31 commits into from
May 20, 2024

Conversation

sahilsekr42
Copy link
Contributor

@sahilsekr42 sahilsekr42 commented Apr 21, 2024

I've tried to fix #11102 by adding a button which may be used to add the selected bst files to the available styles and tested it to work although I think for preview to function well bstvm should be used not sure how. Would like some guidance . After it works as expected may be documentation can be modified.

Mandatory checks

grafik
  • 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.

@Siedlerchr
Copy link
Member

No need to close the PR and reopen. Just continue committing and pushing. The PR will be updated automatically

@Siedlerchr
Copy link
Member

think for preview to function well bstvm should be used not sure how

I am not quite sure I understand you correctly, but the BstPreviewLayout itself already handles the bstVm:, so it should work out of the box

@Override
public String generatePreview(BibEntry originalEntry, BibDatabaseContext databaseContext) {
if (error != null) {
return error;
}
// ensure that the entry is of BibTeX format (and do not modify the original entry)
BibEntry entry = (BibEntry) originalEntry.clone();
new ConvertToBibtexCleanup().cleanup(entry);
String result = bstVM.render(List.of(entry));
// Remove all comments
result = result.replaceAll("%.*", "");

@sahilsekr42
Copy link
Contributor Author

@Siedlerchr thanks I didn't really understand the mechanism of updating pull request (i.e. just committing to my branch automatically updates the pr and runs tests again.

@Siedlerchr
Copy link
Member

Thanks for the PR! We refactored the code a bit and found an issue with the BST rendering itself, (not your fault, but we would like to continue here, so please leave the PR open).

@koppor
Copy link
Member

koppor commented Apr 25, 2024

Intermediate result:

@koppor
Copy link
Member

koppor commented Apr 25, 2024

Current state:

The t is a function which is "not found", but t seems to be string or a variable.

Maybe, the whole new interpretation mixes up parsing of the .bst file and the real execution.

Other than that, I find following code strange - this seems to mix parsing and execution

    @Override
    public Integer visitBstFunction(BstParser.BstFunctionContext ctx) {
        String name = ctx.getChild(0).getText();
        if (bstVMContext.functions().containsKey(name)) {
            LOGGER.trace("Function '{}' found", name);
            bstVMContext.functions().get(name).execute(this, ctx, selectedBstEntry);
        } else {
            LOGGER.trace("Function '{}' not found", name);
            visit(ctx.getChild(0));
        }

        return BstVM.TRUE;
    }

@koppor
Copy link
Member

koppor commented May 19, 2024

Strange git error:

error: Authentication error: Authentication required: You must have push access to verify locks
error: failed to push some refs to 'https://github.com/sahilsekr42/jabref.git'

Pushed to koppor.

@Siedlerchr
Copy link
Member

something with git lfs stuff

@koppor
Copy link
Member

koppor commented May 19, 2024

We identified that there is an endless loop happening in following bst function

% Converts all single dashes "-" to double dashes "--".
FUNCTION {n.dashify}
{ large.number.separate
  't :=
  ""
    { t empty$ not }
    { t #1 #1 substring$ "-" =
        { t #1 #2 substring$ "--" = not
            { "--" *
              t #2 global.max$ substring$ 't :=
            }
            {   { t #1 #1 substring$ "-" = }
                { "-" *
                  t #2 global.max$ substring$ 't :=
                }
              while$
            }
          if$
        }
        { t #1 #1 substring$ *
          t #2 global.max$ substring$ 't :=
        }
      if$
    }
  while$
}

(And we fixed the lookup)

@koppor koppor mentioned this pull request May 19, 2024
6 tasks
* upstream/main:
  Fix BSTVM (JabRef#11304)

# Conflicts:
#	src/main/java/org/jabref/logic/bst/BstVM.java
#	src/main/java/org/jabref/logic/bst/BstVMVisitor.java
#	src/test/java/org/jabref/logic/bst/BstVMTest.java
@Siedlerchr Siedlerchr marked this pull request as draft May 20, 2024 09:45
Fix NPE
add preview to chosen
@Siedlerchr
Copy link
Member

Siedlerchr commented May 20, 2024

Storing in the prefs now works as well

add to layout cycle

Apply some intellij logger suggestions
@Siedlerchr Siedlerchr marked this pull request as ready for review May 20, 2024 10:55
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 20, 2024
@Siedlerchr Siedlerchr requested review from koppor and calixtus May 20, 2024 11:35
@Siedlerchr Siedlerchr enabled auto-merge May 20, 2024 11:44
@Siedlerchr Siedlerchr added this pull request to the merge queue May 20, 2024
Merged via the queue into JabRef:main with commit 16f0e54 May 20, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the option to select a BST style for Preview
4 participants