-
Notifications
You must be signed in to change notification settings - Fork 731
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
Maths support (MSC2191) #2133
Maths support (MSC2191) #2133
Conversation
Can't wait for this to be merged ;-) Did you manage to solve the issue with cut texts? |
Since matrix has become more and more popular in science and universities, this feature is a must have! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Is it possible to:
- Add a lab flag to this feature, disabled by default? The impacted code is sensible, we do not want to break anything, also we do not want to downgrade the performance
- Convert the Java classes you added to kotlin.
Where is this at? This feature is really important for me and it'd be good to get all this merged before the work becomes stale compared to develop. |
I agree with @kylrth This is Very important for me too as a Mathematician. I hope someone fixes this to be merged soon. |
I have ported the Java over to Kotlin and confirmed that things are working as they were before. I will try to put everything behind a labs setting next. I still haven't managed to fix the clipping issue however. I think what happens is that JLaTeXMath schedules what it calls an |
Ok, now it's gated behind Labs cc @bmarty |
So I've determined that the 'wonkiness' with respect to clipping is a bit more subtle:
In any case, it's quite usable even in this state (for the impatient: you can download the APK from buildkite once it finishes building), and I don't think I have the capability to fix this. I'm sure someone more experienced with Android development would be able to figure it out pretty quickly. All told, I think this can be merged if it's going to be behind a labs toggle. People are pretty desperate to use this now, and I'm sure once it's in there it won't be long before someone can take this the rest of the way. Remember to enable the labs setting and then restart the app; also, enable Markdown formatting if you wish to send messages with LaTeX content. |
I am unexperienced... can someone write how to download the apk on an android phone? |
Go to https://buildkite.com/matrix-dot-org/element-android/builds/3299#68f085bf-2408-4a5c-8375-fbddc1c05399 |
Thanks a lot! |
Dear @NickHu , an android phone converts the LaTeX code to math when the message is written say from element-desktop. The other way around does not work. If we write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the update.
- I've enabled GitHub action (CI), please fix mandatory action(s) in error.
- Can you add a changelog file please? See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog for more details
@@ -484,6 +484,7 @@ class MessageItemFactory @Inject constructor( | |||
} | |||
.useBigFont(linkifiedBody.length <= MAX_NUMBER_OF_EMOJI_FOR_BIG_FONT * 2 && containsOnlyEmojis(linkifiedBody.toString())) | |||
.canUseTextFuture(canUseTextFuture) | |||
.markwonPlugins(htmlRenderer.get().plugins) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this behind the lab flag too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I think this should not be gated behind the labs flag. I think you are supposed to use markwon this way. It just so happens that with the existing configuration, it only loads the html rendering plugin and that doesn't have any pre/post hooks so it doesn't matter if you omit this line. In any case, it shouldn't affect performance.
Please confirm you have both
|
(matrix-org/matrix-spec-proposals#2191) Firstly, this implements a commonmark-java plugin which is solely used to parse LaTeX input in the composer box, so that they can be rendered into `<span data-mx-maths=...>fallback</span>` and `<div data-mx-maths=...>fallback</div>` for inline and display maths respectively in the sent message. Secondly, received messages of this form are pre-processed by a simple regex into a form which markwon (which performs the rendering) expects.
I rebased onto cc @bmarty |
@NickHu Tested this build here: https://buildkite.com/matrix-dot-org/element-android/builds/3581#e9486708-9309-47d5-9810-665fe4d8ab68 Displayed math is not rendered at all here, no matter if its |
Same here. Markdown is enabled, new lab feature is enabled, but latex is not rendered, if I send for instance The source of the Event is Also I'm wondering why the html tag is |
@@ -14,7 +14,7 @@ def kotlinCoroutines = "1.5.1" | |||
def dagger = "2.38.1" | |||
def retrofit = "2.9.0" | |||
def arrow = "0.8.2" | |||
def markwon = "4.6.2" | |||
def markwon = "4.3.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependabot will upgrade this dependency, as soon as the PR is merged. Can you add a comment just above to explain why we should stick to this version? A related issue we can track to check if we can safely upgrade would be nice too, or decribe a test that we should do to check that upgrading the lib does not break Math Latex rendering
I just tested it and it's working for me. Did you remember to fully restart the app after enabling the labs setting, and enable Markdown formatting? @antonis-tsolomitis I believe that if you remove the spaces, i.e.
The MSC defines |
Yes I just checked and it worked. It seems there is another bug though, hopefully easy to solve. It does not like > (maybe < too) either display or inline. If I write \[\mathcal{H}^s (A)=\sup_{\delta > 0}\] it appears fine on desktop but fails on android. It shows the source as \[\mathcal{H}^s (A)=\sup_{\delta > 0}\] Is it possible it first changes > to > and then runs katex and of course fails? |
<im.vector.app.core.preference.VectorSwitchPreference | ||
android:defaultValue="false" | ||
android:key="SETTINGS_LABS_ENABLE_LATEX_MATHS" | ||
android:title="@string/labs_enable_latex_maths"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should mention in the summary that user has to restart the app for the change to be taken into account.
I will handle the remaining work on my branch. |
Is there any estimate when this will land in Playstore? |
This is a PR to implement maths support as described here. (TLDR: inline maths is given by HTML
<span data-mx-maths="latex here">fallback representation here</span>
, and display maths bydiv
instead ofspan
).Fortunately, markwon already supports rendering LaTeX; it was just a
matter of enabling it, bumping its version (for inline LaTeX), and doing
some minor processing for MSC2191.
Current state:
<span data-mx-maths=...>...</span>
<div data-mx-maths=...>...</div>
$latex$
→<span data-mx-maths="latex"><code>latex</code></span>
\(latex\)
→<span data-mx-maths="latex"><code>latex</code></span>
$$latex$$
→<div data-mx-maths="latex"><code>latex</code></div>
\[latex\]
→<div data-mx-maths="latex"><code>latex</code></div>
I only implemented an inline Delimiter parser so things like
won't be parsed correctly (requires a block parser), and apparently this class only supports having (possibly many
consecutive) single-character parsers, so
\(
and\[
weren't so straightforward to implement either. It seems like that withcommonmark/commonmark-java#180 inline parsing is getting an overhaul anyway, so it might be best to
wait for that to implement parsing these. It's probably fine to omit this for a first version.
A strange bug is that when rendering a message which only contains maths, it seems to draw the message with very small vertical height to the point at which it appears like a blank bit of vspace (the content is not visible). Sometimes sections of display maths are cropped at the bottom which suggests in general I'm not setting the height of the TextView properly. Any guidance on this would be appreciated.
Signed-off-by: Nick Hu <[email protected]>
Pull Request Checklist