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

Maths support (MSC2191) #2133

Merged
merged 7 commits into from
Dec 31, 2021
Merged

Conversation

NickHu
Copy link
Contributor

@NickHu NickHu commented Sep 18, 2020

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 by div instead of span).

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:

  • Rendering
    • Renders inline maths <span data-mx-maths=...>...</span>
    • Renders display maths <div data-mx-maths=...>...</div>
    • Render messages which have been edited
  • Sending
    • $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

$$
latex
$$

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 with
commonmark/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

  • Changes has been tested on an Android device or Android emulator with API 21
  • UI change has been tested on both light and dark themes
  • Pull request is based on the develop branch
  • Pull request updates CHANGES.md
  • Pull request includes screenshots or videos if containing UI changes
  • Pull request includes a sign off

@tobiasBora
Copy link

Can't wait for this to be merged ;-) Did you manage to solve the issue with cut texts?

@a22sc
Copy link
Contributor

a22sc commented Jul 9, 2021

Since matrix has become more and more popular in science and universities, this feature is a must have!
I'm not deep enough into the code to review the pull request myself. Maybe @bmarty or someone else has capacities..

Copy link
Member

@bmarty bmarty left a 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.

vector/build.gradle Outdated Show resolved Hide resolved
@kylrth
Copy link

kylrth commented Sep 3, 2021

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.

@antonis-tsolomitis
Copy link

I agree with @kylrth This is Very important for me too as a Mathematician. I hope someone fixes this to be merged soon.

@NickHu
Copy link
Contributor Author

NickHu commented Sep 8, 2021

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 AsyncDrawableScheduler over the messageView; however, this preserves the dimensions of the messageView which is (especially in display maths) smaller. I need to find some way to rerender the widget, but I'm not sure how.

@NickHu
Copy link
Contributor Author

NickHu commented Sep 8, 2021

Ok, now it's gated behind Labs

cc @bmarty

@NickHu
Copy link
Contributor Author

NickHu commented Sep 8, 2021

So I've determined that the 'wonkiness' with respect to clipping is a bit more subtle:

  1. display maths gets clipped (but you can still essentially see it, just not the bottom of the integration symbol e.g.);
  2. if a message contains only inline maths, it initially gets drawn with near zero height (and forcing a minimum height doesn't make it render correctly either); however, once you scroll until it falls out of view (dropped by the RecyclerView), and then scroll back, it renders correctly.

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.

@antonis-tsolomitis
Copy link

for the impatient: you can download the APK from buildkite once it finishes building

I am unexperienced... can someone write how to download the apk on an android phone?

@mr-c
Copy link

mr-c commented Sep 9, 2021

for the impatient: you can download the APK from buildkite once it finishes building

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
Click on Artifacts and side load the relevant APK for your phone.

@antonis-tsolomitis
Copy link

Go to https://buildkite.com/matrix-dot-org/element-android/builds/3299#68f085bf-2408-4a5c-8375-fbddc1c05399
Click on Artifacts and side load the relevant APK for your phone.

Thanks a lot!

@antonis-tsolomitis
Copy link

antonis-tsolomitis commented Sep 9, 2021

I installed this version on a 10" android tablet. It seems that there is a size bug for the math that may be relates to the clipping problem. The attached
sizebug1

photo from the tablet shows chat in Greek but it is clear that the Mathematics are too large and not positioned properly. Some above the base line and some below.

@antonistsolomitis
Copy link

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 $x^2$ on the phone, the desktop app AND the phone app does not convert this to math. It remains verbatim. Can you please verify?

@MadLittleMods MadLittleMods added A-Timeline Z-Community-PR Issue is solved by a community member's PR labels Sep 17, 2021
Copy link
Member

@bmarty bmarty left a 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.

@@ -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)
Copy link
Member

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?

Copy link
Contributor Author

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.

@NickHu
Copy link
Contributor Author

NickHu commented Sep 18, 2021

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 $x^2$ on the phone, the desktop app AND the phone app does not convert this to math. It remains verbatim. Can you please verify?

Please confirm you have both

  1. enabled Markdown input in preferences
  2. enabled the Maths Labs feature and restarted the app (clear it from recent apps and launch it again)

@antonis-tsolomitis
Copy link

Ah, yes I missed the Markdown setting. So now it works for inline math on a phone. There are two problems. One is that displaymath does not work (or I miss one more setting (?)):

2

Remaining issues include proper vertical spacing (except clipping) as seen below:

1

3

(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.
@NickHu
Copy link
Contributor Author

NickHu commented Sep 24, 2021

I rebased onto develop, but I had to downgrade the markwon version relative to there (it was updated just over a week ago) because of noties/Markwon#264 (closed but never fixed). It's still a version of markwon newer than what the current stable version uses.

cc @bmarty

@Esokrates
Copy link

Esokrates commented Oct 4, 2021

@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 $$ or \[.

@bmarty
Copy link
Member

bmarty commented Oct 13, 2021

@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 $$ or \[.

Same here. Markdown is enabled, new lab feature is enabled, but latex is not rendered, if I send for instance $$x^2$$, it renders x^2 in the timeline

The source of the Event is

image

Also I'm wondering why the html tag is div and not span as in the PR description.

@@ -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"
Copy link
Member

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

@NickHu
Copy link
Contributor Author

NickHu commented Oct 13, 2021

@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 $$ or \[.

Same here. Markdown is enabled, new lab feature is enabled, but latex is not rendered, if I send for instance $$x^2$$, it renders x^2 in the timeline

The source of the Event is

image

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. $$x^2$$ rather than $$ x^2 $$ it should work.

Also I'm wondering why the html tag is div and not span as in the PR description.

The MSC defines div for display maths, and span for inline.

@antonistsolomitis
Copy link

antonistsolomitis commented Oct 13, 2021

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?
If I write \geq instead of > it works. If I write \greater (from unicode-math) it does not know \greater.

<im.vector.app.core.preference.VectorSwitchPreference
android:defaultValue="false"
android:key="SETTINGS_LABS_ENABLE_LATEX_MATHS"
android:title="@string/labs_enable_latex_maths"/>
Copy link
Member

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.

@bmarty bmarty added this to Inbox in Element Android PRs lifecycle via automation Dec 31, 2021
@bmarty bmarty moved this from Inbox to Need update from submitter in Element Android PRs lifecycle Dec 31, 2021
@bmarty bmarty moved this from Need update from submitter to To be finalized by us in Element Android PRs lifecycle Dec 31, 2021
@bmarty bmarty self-assigned this Dec 31, 2021
@bmarty bmarty changed the base branch from develop to feature/bma/math_final December 31, 2021 15:41
@bmarty
Copy link
Member

bmarty commented Dec 31, 2021

I will handle the remaining work on my branch.

@bmarty bmarty merged commit 4a1c924 into element-hq:feature/bma/math_final Dec 31, 2021
Element Android PRs lifecycle automation moved this from To be finalized by us to Done Dec 31, 2021
This was referenced Jan 3, 2022
@antonis-tsolomitis
Copy link

Is there any estimate when this will land in Playstore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Timeline Z-Community-PR Issue is solved by a community member's PR
Development

Successfully merging this pull request may close these issues.

None yet

10 participants