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 latex rendering in sage-shell-view #71

Merged
merged 2 commits into from
Jun 9, 2023
Merged

Fix latex rendering in sage-shell-view #71

merged 2 commits into from
Jun 9, 2023

Conversation

apresta
Copy link
Contributor

@apresta apresta commented Apr 9, 2023

This fixes issue #61.
Adapted from PR #70 with @aikrahguzar's help.

@EmmanuelCharpentier
Copy link
Collaborator

That's a better alternative than #72. However, it requires some documentation, and a default (initial) value.

I retracted (closed) #72.

Thanks !

@apresta
Copy link
Contributor Author

apresta commented Apr 10, 2023

@EmmanuelCharpentier sounds good, thanks.
Can you please clarify what needs documenting and an initial value?

@EmmanuelCharpentier
Copy link
Collaborator

Can you please clarify what needs documenting and an initial value?

  • Document why this variable may be useful and what is its default value
  • Possibly give an example of customization (I have trouble thinking one...).

HTH,

@apresta
Copy link
Contributor Author

apresta commented Apr 10, 2023

This PR doesn't introduce any new variable, it actually deletes sage-shell-view-latex-math-environment which is no longer needed.

@EmmanuelCharpentier
Copy link
Collaborator

This PR doesn't introduce any new variable, it actually deletes sage-shell-view-latex-math-environment which is no longer needed.

Wups ! I reverse-read your patch....

Deepest apologies !

@apresta
Copy link
Contributor Author

apresta commented Apr 10, 2023

No problem!

@hivert
Copy link

hivert commented May 22, 2023

Thanks a lot for this work ! Any show stopper merging this one ? Having to type "view(...)" each time and switching to my browser seriously disrupt my workflow.

@dimpase
Copy link
Member

dimpase commented Jun 9, 2023

@EmmanuelCharpentier - I've promoted you to a maintainer of this repo, you can merge things now.

@dimpase
Copy link
Member

dimpase commented Jun 9, 2023

@hivert - I can promote you to maintainer of this repo too - would you like to be one?

@hivert
Copy link

hivert commented Jun 9, 2023

@hivert - I can promote you to maintainer of this repo too - would you like to be one?

First of all, thank for the proposition !

If I'd definitely like to help, I'm still at the script-kiddie level concerning emacs internals and lisp programming. I don't think I'm knowledgeable enough for the task. Sorry...

@dimpase
Copy link
Member

dimpase commented Jun 9, 2023

@hivert - well, I suppose you can test things, and that's good enough. Of course I can merge this without looking (I haven't been using emacs for 20 years), but that's not as good.

@EmmanuelCharpentier
Copy link
Collaborator

@EmmanuelCharpentier - I've promoted you to a maintainer of this repo, you can merge things now.

Thanks ! I'll do that (I've reviewed this patch already, and I'm using it currently).

@EmmanuelCharpentier EmmanuelCharpentier merged commit c3ea00d into sagemath:master Jun 9, 2023
@EmmanuelCharpentier
Copy link
Collaborator

I just accepted the merge. It should appear on Melpa in a not too long time. Bug me if that doesn't happen.

@EmmanuelCharpentier
Copy link
Collaborator

EmmanuelCharpentier commented Jun 10, 2023

Well...

  • This PR did reach MELPA.
  • The resulting package :
    • does plot in the Sage buffer, but
    • dos not typeset LaTeX.

whereas my local branch does. I'll investigate.

@EmmanuelCharpentier
Copy link
Collaborator

Could you please review this pull request which reintegrates @apresta 's fix, which somehow disappeared from the previous pull request I merged (after successfully reviewing its branch) ?

Thanks again...

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.

4 participants