-
Notifications
You must be signed in to change notification settings - Fork 297
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
[Merged by Bors] - feat(meta/widget): Add css classes for indentation as required by group and nest. #3764
Conversation
Signed-off-by: Daniel Fabian <daniel.fabian@integral-it.ch>
src/tactic/interactive_expr.lean
Outdated
pure [h "span" [cn "indent-code", style [("--indent-level", to_string i)]] inner] | ||
| ca (sf.highlight c a) := do | ||
inner ← view ca a, | ||
pure [h "span" [cn $ "highlight_" ++ c.to_string] inner] |
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.
Can you try replacing [cn $ "highlight_" ++ c.to_string]
with [cn c.to_string]
? There are already some CSS classes which style particular colours.
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.
I guess a problem is that you want the highlighting colours to be theme-dependent so that they are readable for both light and dark themes.
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.
BTW, would it be possible to inline the
.indent-code
class for now (i.e. usestyle
instead ofcn
)? Then we could test it without modifying the extension.
Unfortunately not. We need to change the CSS in the extension in such a way, that the whole subgoals become display:inline-block
as opposed to display:inline
what they are now. If you do it only on one tag, it completely moves stuff around and it's not even in the same order anymore.
I was in contact with @EdAyers about this and understood, that we can take a look at the vscode extension shortly, this is mostly preparatory work, really.
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.
LGTM. Maybe @EdAyers wants to look at this as well (there is a demo webpage linked from the PR in the lean repo).
bors d=EdAyers
src/tactic/interactive_expr.lean
Outdated
pure [h "span" [cn "indent-code", style [("--indent-level", to_string i)]] inner] | ||
| ca (sf.highlight c a) := do | ||
inner ← view ca a, | ||
pure [h "span" [cn $ "highlight_" ++ c.to_string] inner] |
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.
This should probably be translated to one of the classes like hot-pink
that are already in tachyon.
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.
I'd rather not do that. Because it encodes a hard mapping between mathlib's source code and the plugin, whereas the CSS class is conceptually much more flexible. Plus highlighting might also be encoded with other means than color like italics or bold. From an accessibility point-of-view, I'd rather have the CSS handle that, if it's ok.
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.
Everything about the lean widget system already assumes a hard mapping, namely it assumes that a CSS library called Tachyons is being used. So any styling choices should be made in the lean code not in a separate CSS file (I admit indent-level
is an exception). I don't think it makes sense to have color.red
mean anything other than colouring the text red.
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.
I guess it's awkward for people who have a kind of colourblindness where they can't distinguish certain colours from black or white (idk if that's possible). I don't think we will have cases where it is necessary for the user to be able to distinguish the highlight colours to understand the format string.
✌️ EdAyers can now approve this pull request. To approve and merge a pull request, simply reply with |
BTW, would it be possible to inline the |
We should hold merging this until the
Maybe... I'm not sure if React styles supports these CSS variable calculations yet. Another approach is to use the customCSS vscode setting. "lean.infoViewStyle": ".indent-code { text-indent: calc(var(--indent-level) * -1ch); padding-left: calc(var(--indent-level) * 1ch);}" |
How did you get your output in the jsfiddle to have |
you need all elements to be |
I think not all elements need to be |
Added `dib` to render function for `sf.block`.
could you share the HTML you used to draw this? I think I had similar kinds of artifacts previously, too |
Ah, yes, I had exactly these kinds of artifacts, until everything, including |
The inner inline block is linebreaking prematurely, I guess because the width linebreak calculation is taking in to account the indentation / padding wrong. It's not in the CSS standard yet but in the future it should be possible to use |
The problem with |
|
non-breaking spaces I get by using |
I just played around with the jsfiddle a bit. The |
yes, in principle, The main idea was about not hard-coding that |
Having the |
I think the weird rendering is due to directly nested |
Some experience reports from the documentation PR:
(example here: https://gebner.github.io/mathlib_docs/control/traversable/basic.html#is_lawful_traversable) |
one thing we could do, is make the space after |
But wouldn't you then get this?
Ideally we'd have a span around |
not But just a span does not prevent breaking. |
Regarding the wider discussion points: First of all, I don't think there is a way to faithfully support the current format API in CSS. There is a huge semantic gap between Therefore, I believe we either need quick-to-implement hacky heuristics, or some extension to the current format code like an explicit new Extending the current format code (and pretty-printer) is a significant undertaking, though. May I remind you that the days of Lean 3 are numbered, and we will soon pay allegiance to the Clover. On the other hand, this could be an experiment that makes it into Lean 4, I don't know. The CSS-based formatting is less powerful (and hence uglier) than the Wadler-Leijen pretty-printer, so we can't get rid of the "old" one. Maybe it is enough to distinguish between
I don't think the Lean 3 pretty printer is ever going to change anymore.
The information in the format object is not a pretty-printer-agnostic description of how the text should be rendered. It has a precise semantics (see the Wadler paper), and we tune the format objects so that the one implementation we have prints it nicely. Additionally, the format object also contains one more piece of information, which is at least as important as the two you mentioned. Namely the group information, which tells us that several line breaks can only be inserted together.
I wouldn't underestimate the effort here. You'd also need a lot of API that is only accessible from C++ at the moment (like notations, etc.). |
Fair enough, just one last comment. The behaviour or introducing line breaks if and only if the content doesn't fit is very much what a browser does all the time. In fact, it does it, whenever you don't disallow line breaks. Hence the distinction between |
Mmmh, maybe I wasn't clear enough. If the following doesn't fit on a single line, it will take up 10 lines (because all of the linebreaks were activated): format.group $ format.nest 1 $ "[" ++
"x," ++ format.line ++
"x," ++ format.line ++
"x," ++ format.line ++
"x," ++ format.line ++
"x," ++ format.line ++
"x," ++ format.line ++
"x," ++ format.line ++
"x," ++ format.line ++
"x," ++ format.line ++
"x" ++ "]" |
Ah gotcha |
Yeah this is way more difficult to implement than I was hoping it would be. When I originally wrote |
Suppose that the |
Would something like 2021706 work? |
It tracks the length of the current line with a counter and then wraps the rest of the expr in a div if it overflows a fixed column count. It looks ugly rn but as a POC is it the right approach? |
@EdAyers If you have the screen width available, then you should just implement the formatting algorithm that The "insert newline on overflow" approach is pretty much was the CSS version does, AFAICT. I don't think we need to reimplement that in Lean. The heuristics used in the mathlib documentation seem to work mostly fine (maybe the trailing closing parentheses could be cleaned up though). |
Right so could we just set the screen width to be a constant for now? |
To be honest, I would prefer even the current slightly buggy version of this PR over a fixed screen width. |
I think getting the screen width from JavaScript would in principle enable us to use the correct But without it, I too think the current CSS-based version is a better trade-off. |
I have a question, did the Wadler formatter ever work in the vscode extension? |
Looks mostly good to me now. Thanks @gebner for sorting that out, much easier than implementing a formatter! |
Let's save the fix for above for another PR bors r+ |
👎 Rejected by label |
bors r+ |
…up and nest. (#3764) this is a transplant of leanprover-community/lean#439 the relevant css section looks more or less like this: ```css .indent-code { text-indent: calc(var(--indent-level) * -1ch); padding-left: calc(var(--indent-level) * 1ch); } ``` For details, one can play around here: https://jsfiddle.net/xbzhL60m/45/ Co-authored-by: Ed Ayers <EdAyers@users.noreply.github.com> Co-authored-by: Gabriel Ebner <gebner@gebner.org> Co-authored-by: E.W.Ayers <E.W.Ayers@maths.cam.ac.uk>
Pull request successfully merged into master. Build succeeded: |
…up and nest. (#3764) this is a transplant of leanprover-community/lean#439 the relevant css section looks more or less like this: ```css .indent-code { text-indent: calc(var(--indent-level) * -1ch); padding-left: calc(var(--indent-level) * 1ch); } ``` For details, one can play around here: https://jsfiddle.net/xbzhL60m/45/ Co-authored-by: Ed Ayers <EdAyers@users.noreply.github.com> Co-authored-by: Gabriel Ebner <gebner@gebner.org> Co-authored-by: E.W.Ayers <E.W.Ayers@maths.cam.ac.uk>
…up and nest. (#3764) this is a transplant of leanprover-community/lean#439 the relevant css section looks more or less like this: ```css .indent-code { text-indent: calc(var(--indent-level) * -1ch); padding-left: calc(var(--indent-level) * 1ch); } ``` For details, one can play around here: https://jsfiddle.net/xbzhL60m/45/ Co-authored-by: Ed Ayers <EdAyers@users.noreply.github.com> Co-authored-by: Gabriel Ebner <gebner@gebner.org> Co-authored-by: E.W.Ayers <E.W.Ayers@maths.cam.ac.uk>
this is a transplant of leanprover-community/lean#439
the relevant css section looks more or less like this:
For details, one can play around here: https://jsfiddle.net/xbzhL60m/45/