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

Tweak annotation ordering during modifications #53794

Closed

Conversation

tecosaur
Copy link
Contributor

@tecosaur tecosaur commented Mar 20, 2024

In my estimation, these changes make the sorting, particularly in combination with StyledStrings, more consistent/intuitive.

See the commit message and _annot_sortkey docstring for more details.

Status quo

image

With this PR (and JuliaLang/StyledStrings.jl@c21b43e)

image

As I've continued to use the AnnotatedString type and StyledStrings,
I've come across some un-intuitive instances where more "broad" styling
attributes override more "specific" attributes. Talking with other
people confirms that this isn't just me.

Giving the current behaviour some more thought, I've realised that just
sorting using the default comparison for UnitRange{Int} doesn't quite
produce the most approriate results here. So, I introduce
`_annot_sortkey` (which see), that tries to produce a more intuitive
results.
@tecosaur tecosaur added strings "Strings!" display and printing Aesthetics and correctness of printed representations of objects. labels Mar 20, 2024
@tecosaur
Copy link
Contributor Author

Test failures appear unrelated

@tecosaur
Copy link
Contributor Author

If this could be backported to 1.11 that would be nice I think, but I'm not sure how this change fits in with the guidelines on that.

@tecosaur
Copy link
Contributor Author

tecosaur commented Apr 2, 2024

Let me know if I'm being too ambitious, but I'm going to speculatively tag this with backport-1.11 since it (IMO) improves the state of this feature in the 1.11 release.

@tecosaur tecosaur added backport 1.11 Change should be backported to release-1.11 awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. labels Apr 2, 2024
@KristofferC KristofferC mentioned this pull request Apr 9, 2024
41 tasks
@LilithHafner
Copy link
Member

On the surface, this looks like a behavioral/api change, so if it isn't backported it would be breaking in 1.12

The fourth example in your status quo screenshot looks like a bug to me. Bugfixes should be backported.


Where, if anywhere, is the interpretation for conflicting style annotations specified?

I propose that

  1. annotations applied later always take precedence over annotations applied earlier
  2. the @styled_str macro produces annotation in a sensible order given the above (subsets after supersets)

Which is what most GUI text editors I'm familiar with use (if I select a region and make it red, and then select a larger region and make it green, the red is overridden. But an annotation without precedence could still have an impact, for example if I first annotated a small region bold and then a large region red, the intersection would be bold and red.


What I think you are proposing (B) is

  1. annotations that start later take precedence over annotations that start earlier
  2. of annotations that have the exact same start, those that nest within take precedence over those that encompass
  3. of annotations on the exact same region, those that are applied later take precedence

Which is a more complicated set of rules than (A).

I think that these rules differ when using interpolation (A makes the outer string's annotations take precedence while B gives the inner string precedence iff it does not fill the entire outer face (see table))

Interpolation A B
"{new: $old}" new old
"{new:$old }" new old
"{new:$old}" new new

Otherwise, they do not differ (as far as I can tell) except when explicitly using the annotate or annotate! functions. When folks explicitly annotate, I think we should let them pick the order (i.e. better to use insertion order than a fancy sort with ties broken by insertion order).


I only have cursory understanding of what's going on here, so lmk if I'm misunderstanding.

@tecosaur
Copy link
Contributor Author

The fourth example in your status quo screenshot looks like a bug to me. Bugfixes should be backported.

Yea, after I saw that behaviour my reaction was "that's not right". I'd like to see this change merged and backported.

Where, if anywhere, is the interpretation for conflicting style annotations specified?

The current interpretation is "last specified wins", the question is: when using the styled"" and annotated! facilities, what is put last?

I think when discussing the behaviour, it's worth talking about the styled"" macro and annotate! separately.

@styled

With these changes, I think we can characterise the styled"" macro behaviour just as:

  • innermost declaration wins

and interpolation acts as if you wrote in the value.

To clarify by way of an example, with styled"{green:{blue:{red:text}}}" the red style wins, and if you let a = styled"{red:text}", styled"{green:{blue:$a}}" behaves the same.

annotate!

I believe your interpretation (B) is correct, in that

  • Annotations applied to later, narrower regions take priority
  • When applying multiple annotations to a region, the last one applied wins

To take your "highlight a larger region and make it red, and only red" example, I think the way to do that would be not just to apply it later but:

  • Scan for annotations that overlap with the target region
  • Remove them
  • Apply the "red" annotation

It could be helpful to expose the "remove all annotations in this region" functionality provided by _clear_annotations_in_region!, but that's a slightly separate matter.

I think the reason why I didn't just go for "later wins" (A) is that the final result of a "later wins" scheme should be able to be represented by a set of annotations that is in the expected order, and it seems nice to have the set of annotations kept in a single ordering when possible.

As an aside, if you re-constuct the AnnotatedString you can put the annotations in whatever order you want.

@LilithHafner
Copy link
Member

@styled_str

I agree that interpolated strings should behave as if you wrote them in, and that "innermost declaration wins" is an appropriate characterization for styled"". I agree with the proposed semantic.

annotate!

_clear_annotations_in_region! would have no way of knowing that blue should be cleared and bold should not be cleared when applying red to a larger region. The scan and remove step is "scan for annotations that overlap with the target region and have a conflicting semantic"

...it seems nice to have the set of annotations kept in a single ordering when possible.

I don't understand what you mean by this

if you re-constuct the AnnotatedString you can put the annotations in whatever order you want.

#53800 makes that point somewhat moot. I don't want to rely on a display function not performing optimization.

@tecosaur
Copy link
Contributor Author

...it seems nice to have the set of annotations kept in a single ordering when possible.

I don't understand what you mean by this

There are a few small reasons why annotations are sorted at all. Perhaps this should be re-evaluated entirely, but I think it's good that: (1) having annotations appear sorted by where they start makes it possible to generally look for (and operate on) annotations in a certain region in O(log(n)) time rather than O(n), (2) annotatedstring_optimize! relies on the assumption that annotations are ordered to be O(n) instead of O(n^2), (3) this ordering helps produces the desirable behavior seen with styled"" and I partially see the effect on annotate! as a matter of consistency.

You're right about removing annotations in the target region perhaps not being the particular behavior wanted, but for that case you can accomplish the result you desire by adding annotations for the same region(s) as existing annotations after them. Since the sorting is all stable, they will override previous attributes (this is all under the annotation interpretation model that StyledStrings uses, once could also implement at "first annotation wins" policy in a different context).

I say this not to make the point "it's trivially easy", but that it is very much possible to achieve the result you describe with sorting, and so I think this is best characterised as a tradeoff of convenience. I currently judge the sorting to be worth having, and this scheme to be better, but I'm not dead set on this view by any means.

@KristofferC KristofferC mentioned this pull request Apr 17, 2024
59 tasks
@tecosaur tecosaur removed awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. backport 1.11 Change should be backported to release-1.11 labels Apr 27, 2024
@tecosaur
Copy link
Contributor Author

After a long chat with @LilithHafner, we're going to be taking a different approach to this. A superseding PR should be up shortly.

@tecosaur tecosaur closed this Apr 27, 2024
@tecosaur tecosaur deleted the annotation-priority-tweaks branch April 27, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects. strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants