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

Double click on citation opens LaTeX editor #12044

Merged
merged 35 commits into from
Oct 23, 2024

Conversation

u7465990
Copy link
Contributor

@u7465990 u7465990 commented Oct 21, 2024

Fixes #11996

I think I make the function works. The viewModel in LatexCitationTab is to store those entries as DoubleClickOnlyListWrapper. The variable citationsDisplay is the Listener of viewModel'. I use citationsDisplayto catch the double click event. If it's click the right entry, I use thePushToApplicationto call thejumpToLine. I found the set up ways in the PushToApplicationCommand. After the setup, the publicJumpToLinewill be called, it usejumpStringto get the command and use the newprocessBuilder` to run the command. It should works for TeXstudio.

Mandatory checks

  • I own the copyright of the code submitted and I licence it under the MIT license
  • 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.

@ThiloteE ThiloteE changed the title Feature latex editor#11996 Double click on citation opens LaTeX editor Oct 21, 2024
@ThiloteE
Copy link
Member

To ease organizational workflows, I have linked the pull-request to the issue with syntax as described in https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue.

Linking a pull request to an issue using a keyword

You can link a pull request to an issue by using a supported keyword in the pull request's description or in a commit message. The pull request must be on the default branch.

  • close
  • closes
  • closed
  • fix
  • fixes
  • fixed
  • resolve
  • resolves
  • resolved

If you use a keyword to reference a pull request comment in another pull request, the pull requests will be linked. Merging the referencing pull request also closes the referenced pull request.

The syntax for closing keywords depends on whether the issue is in the same repository as the pull request.

Example:

  • Fix #xyz links pull-request to issue. Merging the PR will close the Issue.
  • Fix https://github.com/JabRef/jabref/issues/xyz links pull-request to issue. Merging the PR will close the Issue.
  • Fix https://github.com/Koppor/jabref/issues/xyz links pull-request to issue. Merging the PR will close the Issue.
  • Fix [#xyz](https://github.com/JabRef/jabref/issues/xyz) links pull-request to issue. Merging the PR will NOT close the Issue.

@u7465990
Copy link
Contributor Author

Fix #11996

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Many obsolte code comments, some methods seem be left-overs from trial and error.

Please also fix sub module errors: https://devdocs.jabref.org/code-howtos/faq.html#submodules

Comment on lines 145 to 148
// private void jump(Path fileName, int line, int column) {
// BibDatabaseContext database = stateManager.getActiveDatabase().orElseThrow(() -> new NullPointerException("Database null"));
// application.publicJumpToLine(database, stateManager.getSelectedEntries(), getKeyString(stateManager.getSelectedEntries(), application.getDelimiter()));
// }
Copy link
Member

Choose a reason for hiding this comment

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

Is this commented-out code needed?

ProcessBuilder processBuilder = new ProcessBuilder(command);
processBuilder.start();
} catch (IOException e) {
// Use robust logging instead of printStackTrace()
Copy link
Member

Choose a reason for hiding this comment

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

Remove comment

// Use robust logging instead of printStackTrace()
LOGGER.error("Could not open TeXstudio at the specified location.", e);

// Show an error dialog to the user
Copy link
Member

Choose a reason for hiding this comment

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

This is clear from the line below - remove comment

LOGGER.error("Could not open TeXstudio at the specified location.", e);

// Show an error dialog to the user
dialogService.showErrorDialogAndWait("Error", "Could not open TeXstudio at the specified location.");
Copy link
Member

Choose a reason for hiding this comment

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

Use localization. Paramterize the tool using %0. See https://devdocs.jabref.org/code-howtos/localization.html for details.

protected void jumpToLine(Path fileName, int line, int column) {
commandPath = preferences.getPushToApplicationPreferences().getCommandPaths().get(this.getDisplayName());

// Check if a path to the command has been specified
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these kinds of comments - there are clear form the code.

Comment on lines 214 to 220
protected String[] jumpString(Path fileName, int line, int column) {
return new String[0];
}

public void publicJumpToLine(Path fileName, int line, int column) {
jumpToLine(fileName, line, column);
}
Copy link
Member

Choose a reason for hiding this comment

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

What are these methods for? I think, they can be deleted?

@u7465990
Copy link
Contributor Author

The fetcher tests is failled. But I didn't change any code relate to the internet. How should I fix it?

@Siedlerchr
Copy link
Member

Siedlerchr commented Oct 22, 2024 via email

@u7465990
Copy link
Contributor Author

You can ignore the fetcher tests if you did not change the fetchers Ricky-Du @.> schrieb am Di., 22. Okt. 2024, 08:23:

The fetcher tests is failled. But I didn't change any code relate to the internet. How should I fix it? — Reply to this email directly, view it on GitHub <#12044 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACOFZB4QZIBHJ54WHXWHGTZ4XVOZAVCNFSM6AAAAABQJFOXXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRYGM2TOOJYHA . You are receiving this because you are subscribed to this thread.Message ID: @.
>

Thanks

u7465990 and others added 3 commits October 23, 2024 14:45
…11996

# Conflicts:
#	.github/workflows/on-review-submitted.yml
#	src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java
#	src/main/java/org/jabref/logic/bibtex/InvalidFieldValueException.java
#	src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java
@koppor
Copy link
Member

koppor commented Oct 23, 2024

@u7465990 I assume you used git rebase. Please use git merge upstream/main, this performs better. -- I fixed the merge conflicts for you to let the CI do its work here.

@@ -154,6 +160,26 @@ public void onDirectoryDelete(File directory) {
};
}

// Handle mouse click event
Copy link
Member

Choose a reason for hiding this comment

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

Remove obsolete comment - next line states the same

@@ -154,6 +160,26 @@ public void onDirectoryDelete(File directory) {
};
}

// Handle mouse click event
public void handleMouseClick(MouseEvent event, CitationsDisplay citationsDisplay) {
// Get the currently selected item
Copy link
Member

Choose a reason for hiding this comment

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

Remove obsolete comment - next line states the same

// Get the currently selected item
Citation selectedItem = citationsDisplay.getSelectionModel().getSelectedItem();

// Check if the left mouse button was double-clicked
Copy link
Member

Choose a reason for hiding this comment

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

Remove obsolete comment - next line states the same


// Check if the left mouse button was double-clicked
if (event.getButton() == MouseButton.PRIMARY && event.getClickCount() == 2 && selectedItem != null) {
// Perform a jump or other actions
Copy link
Member

Choose a reason for hiding this comment

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

Remove obsolete comment - next line states the same

Comment on lines 186 to 189
/**
* This function is to jump to a specific line due to different editor.
*
*/
Copy link
Member

Choose a reason for hiding this comment

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

Remove obsolete comment - next line states the same

* This function is to jump to a specific line due to different editor.
*
*/
protected void jumpToLine(Path fileName, int line, int column, ProcessBuilder processBuilder) {
Copy link
Member

Choose a reason for hiding this comment

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

Never overridden. Not sure why it is separated of jumpToLine(Path fileName, int line, int column). Inline it to

public void jumpToLine(Path fileName, int line, int column) {

Comment on lines 33 to 35
/**
* Method to open TeXstudio at the given line number in the specified LaTeX file.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Remove obsolete comment - next line states the same

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Something was wrong when doing git merge upstream/main.

@@ -0,0 +1,21 @@
name: On reviewed PR
Copy link
Member

Choose a reason for hiding this comment

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

Why is this appearing - I removed in main. Please remove this file from your branch.

@@ -147,4 +148,9 @@ protected String getCommandName() {
public PushToApplicationSettings getSettings(PushToApplication application, PushToApplicationPreferences preferences) {
return new PushToEmacsSettings(application, dialogService, this.preferences.getFilePreferences(), preferences);
}

@Override
protected String[] jumpString(Path fileName, int line, int column) {
Copy link
Member

Choose a reason for hiding this comment

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

Rename to jumpToLineCommandlineArguments (or similar)


@Override
public String[] jumpString(Path fileName, int line, int column) {
// Construct the TeXstudio command
Copy link
Member

Choose a reason for hiding this comment

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

Remvoe comment. It is clear from the context.


@Override
protected String[] jumpString(Path fileName, int line, int column) {
// didn't find any command to jump to a specific line
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
// didn't find any command to jump to a specific line
// No command known to jump to a specific line

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun, check the results, commit, and push.

You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".

@koppor
Copy link
Member

koppor commented Oct 23, 2024

I fixed merge conflicts with upstream/main and refactored some code. The vim code looks strange. The other method has "--remote-send", the new method does not make use of "--remote-send". However, this is not important, can be refined in a follow-up PR (if users have suggestions).

koppor
koppor previously approved these changes Oct 23, 2024
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Tested with TeXsudio. Works.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Works. Needed to add CHANGELOG.md entry.

@koppor koppor added this pull request to the merge queue Oct 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 23, 2024
@koppor koppor added this pull request to the merge queue Oct 23, 2024
Merged via the queue into JabRef:main with commit c3e5621 Oct 23, 2024
23 checks passed
@u7465990
Copy link
Contributor Author

Thanks. I plan to add the vscode as a push target. Should I open a new issue for this?

@u7465990 u7465990 deleted the feature-latex-editor#11996 branch October 23, 2024 23:17
@Siedlerchr
Copy link
Member

Just create a new pr

This was referenced Oct 26, 2024
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.

Double click on citation should open the LaTeX editor at the respective place
4 participants