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

feat(tactic/interactive_expr): Color instances in the infoview #15959

Closed
wants to merge 7 commits into from

Conversation

YaelDillies
Copy link
Collaborator

@YaelDillies YaelDillies commented Aug 9, 2022

Change the behavior of the infoview to visually distinguish:

  • frozen instances
  • typeclass assumptions that are not instances
  • unfrozen typeclass assumptions
  • non typeclass assumptions

The style is very much up for debate! See Zulip. Once we have decided on the style, I will open a preliminary PR to vscode-lean adding the new attributes to the CSS.

Open in Gitpod

@YaelDillies YaelDillies added the RFC Request for comment label Aug 9, 2022
@YaelDillies YaelDillies marked this pull request as ready for review August 13, 2022 07:58
@YaelDillies YaelDillies added the awaiting-review The author would like community review of the PR label Aug 13, 2022
@YaelDillies YaelDillies added t-meta Tactics, attributes or user commands and removed RFC Request for comment labels Aug 25, 2022
@jcommelin jcommelin added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Sep 4, 2022
@YaelDillies YaelDillies added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Sep 18, 2022
let ns : list (html γ) := lc.locals.map $ λ n,
h "span" [cn "goal-hyp b pr2", key n.local_uniq_name] [html.of_name n.local_pp_name],
Copy link
Member

Choose a reason for hiding this comment

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

Where did the key n.local_uniq_name go?

@eric-wieser eric-wieser added the awaiting-author A reviewer has asked the author a question or requested changes label Sep 18, 2022
then "goal-hyp-inst"
else "goal-hyp-noninst"
| none := "goal-hyp-unfrozen"
end in h "span" [cn $ var_style ++ " b pr2"] [html.of_name $ expr.local_pp_name n],
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
end in h "span" [cn $ var_style ++ " b pr2"] [html.of_name $ expr.local_pp_name n],
end in h "span" [cn $ var_style ++ " b pr2"] [html.of_name n.local_pp_name],

which is more similar to how it was spelt originally

Comment on lines +397 to +402
then "goal-hyp"
else match is with
| some is' := if n ∈ is'
then "goal-hyp-inst"
else "goal-hyp-noninst"
| none := "goal-hyp-unfrozen"
Copy link
Member

Choose a reason for hiding this comment

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

In the interest of backwards-compatibility with older extension versions, perhaps

Suggested change
then "goal-hyp"
else match is with
| some is' := if n ∈ is'
then "goal-hyp-inst"
else "goal-hyp-noninst"
| none := "goal-hyp-unfrozen"
then "goal-hyp"
else match is with
| some is' := if n ∈ is'
then "goal-hyp goal-hyp-inst"
else "goal-hyp goal-hyp-noninst"
| none := "goal-hyp goal-hyp-unfrozen"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would it make sense to do

Suggested change
then "goal-hyp"
else match is with
| some is' := if n ∈ is'
then "goal-hyp-inst"
else "goal-hyp-noninst"
| none := "goal-hyp-unfrozen"
then "goal-hyp"
else match is with
| some is' := if n ∈ is'
then "goal-hyp inst"
else "goal-hyp noninst"
| none := "goal-hyp unfrozen"

instead?

Copy link
Member

Choose a reason for hiding this comment

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

No, because inst is a pretty vague CSS class name, as is unfrozen. If in doubt, qualify your names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Won't the fully qualified name be goal-hyp inst? Disclaimer: I don't understand CSS.

Copy link
Member

@eric-wieser eric-wieser Sep 18, 2022

Choose a reason for hiding this comment

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

class (which cn sets) is an unordered set of unrelated classnames. You can of course apply styles to things that match both goal-hyp and inst (via the indistinguisable selectors .inst.goal-hyp or .goal-hyp.inst, since conjunction is concatenation), but the danger is that someone else might already be applying a style you don't expect to .inst by itself.

@jcommelin jcommelin removed the awaiting-review The author would like community review of the PR label Sep 19, 2022
@YaelDillies
Copy link
Collaborator Author

This PR is made irrelevant by the way Lean 4 handles instances. No need to worry about frozen instances anymore.

@YaelDillies YaelDillies deleted the infoview_instances branch March 24, 2023 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author A reviewer has asked the author a question or requested changes t-meta Tactics, attributes or user commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants