SourceSpan support #123

ForNeVeR · 2018-04-01T11:01:31Z

Closes #115.

This is a rebased version of PR #116. I will be fixing the tests here and polishing the changes.

TODO

Rebase all the remaining changes
Fix the issues mentioned in Add StringSpan type to save the source information in Atoms and Boxes #116 (comment) and further discussion:
- Remove BoxWalker and FormulaUtils
- Drop Atom.Copy in favor of AtomWithSource
- ~~Add Box.SourceAtom, drop Box.Source~~ I decided to not do that
Change StringSpan into SourceSpan
Add tests for SourceSpans
Fix the differences between Add StringSpan type to save the source information in Atoms and Boxes #116 and this PR (see my comment below)

ForNeVeR · 2018-04-05T16:24:31Z

Well, I've started removing Atom.Copy in favor of AtomWithSource and I have a feeling that something's wrong. Look: we have all these references all over the place (e.g. BigOperatorAtom referencing its' BaseAtom), and it looks like we should give this property a signature AtomWithSource BaseAtom { get; set; }

Our code will become a mess with slices of Atoms and AtomWithSources, I don't like that.

Looks like we should return back to the design board. Probably we will need that Atom.Copy after all? Maybe other guys' suggestion about rewriting everything in F# wasn't so radical as I thought?

Because, well, current problem is that we cannot get cheap class copying in C#, so we have to implement this Copy method in every Atom descendant, and I extremely dislike that. What could we do? Make everything struct? Will it work? Any other suggestions?

alexreg · 2018-04-05T18:05:02Z

@ForNeVeR As long as there are no many-to-one or one-to-many references (from memory I don't think so), then converting everything to value types (i.e. structs) would seem to make sense to me. You get efficient copying for free then.

ForNeVeR · 2018-04-06T04:12:40Z

Yeah, working on it in a separate branch. We'll see what else we can do. I have some other ideas.

This commit intentionally won't compile. # Conflicts: # src/WpfMath/Atom.cs # src/WpfMath/CharSymbol.cs # src/WpfMath/DummyAtom.cs # src/WpfMath/OverUnderDelimiter.cs # src/WpfMath/RowAtom.cs # src/WpfMath/SpaceAtom.cs # src/WpfMath/TexFormulaParser.cs # src/WpfMath/UnderOverAtom.cs

ForNeVeR · 2018-04-14T10:46:43Z

Add Box.SourceAtom, drop Box.Source

Now I'm not sure about that. Currently, all of our Atoms are internal classes, so we cannot publish them through a public member. And I'd keep it as is for the moment, because the Atom API will likely be unstable in the future.

We could mark only the root of the Atom type hierarchy as public, but I can't see a point for that: the only usable public members would be TexAtomType and SourceSpan. Basic Atom is useless if the user cannot see its' structure.

Hereby I revoke my requirement about adding Box.SourceAtom. For now, let's leave a Box.Source member intact.

ForNeVeR · 2018-04-14T10:53:07Z

Also, we have some bugs here. @yamamoto-reki made the selected blocks to highlight themselves in the example project. E.g.

(the highlighting works well and it's accurate enough)

In my PR, it doesn't highlight some blocks and sometimes behaves weird:

Before merging, we'll need to fix that weirdness and make sure everything works well.

ForNeVeR · 2018-04-14T15:18:17Z

src/WpfMath/Controls/FormulaControl.xaml.cs

@@ -75,6 +94,18 @@ public ControlTemplate ErrorTemplate
            "ErrorTemplate", typeof(ControlTemplate), typeof(FormulaControl),
            new PropertyMetadata(new ControlTemplate()));

+        public static readonly DependencyProperty SelectionStartProperty = DependencyProperty.Register(
+            "SelectionStart", typeof(int), typeof(FormulaControl),


Don't forget to use nameof here!

ForNeVeR · 2018-04-14T15:35:04Z

src/WpfMath/TexFormula.cs

@@ -49,18 +49,23 @@ public TexRenderer GetRenderer(TexStyle style, double scale, string systemTextFo
            return new TexRenderer(CreateBox(environment), scale);
        }

-        public void Add(TexFormula formula)
+        public void Add(TexFormula formula, SourceSpan source = null)


The binary compatibility has been broken here, unfortunately (the source compatibility was preserved). We could fix that. Should we?

Don't think so. Hopefully nobody uses that API. And we're still in API-breaking period of our semver (0.x versions). And it's notoriously hard if not impossible to keep full source+binary compatibility when method overloads are involved.

ForNeVeR · 2018-04-14T15:38:29Z

src/WpfMath/TexPredefinedFormulaParser.cs

@@ -1,341 +1,339 @@
-using System;


Previously this file had CRLF and now it has LF (it was replaced because I renamed it to match the class name). I think that's alright.

Please check the online diff with ?w=1 appended to the page URL to see the real changes (it will ignore the whitespace diff): https://github.com/ForNeVeR/wpf-math/pull/123/files?w=1#diff-e81f0c498a5c8d83d49d999c0375f718

ForNeVeR · 2018-04-15T06:15:08Z

Alright, it's ready for review, guys.

@yamamoto-reki could you also please take a look?

My next step will be to review (and probably merge) the remaining changes (BoxWalker, FormulaUtils) from your PR #116.

ForNeVeR · 2018-04-16T05:36:32Z

src/WpfMath/TexFormulaHelper.cs


+        // TODO[F]: Review the cases where the formula gets constructed from this.Formula.RootAtom wrapped in something
+        // (e.g. SetFixedTypes): in these cases, it looks like we should clean the SourceSpan inside of a formula and
+        // set it for the new root atom only?


Please file an issue about that!

gsomix

OK, that's very big PR...

🐐

gsomix · 2018-04-27T15:18:11Z

Great job!

ForNeVeR · 2018-04-27T16:09:44Z

Thanks for the review!

alexreg · 2018-04-27T16:33:23Z

Good job with this, @ForNeVeR! Not a small task, but looks like really important work going forwards.

#115 Path to convey range of source string

e2dd93e

ForNeVeR self-assigned this Apr 1, 2018

yamamoto-reki and others added 9 commits April 1, 2018 18:04

#115 Add range of source string to CharAtom and CharBox

8c28d3c

Fix the BoxTest compilation (#115)

d4f084c

Disable checking of an Atom.Source property (#115)

49e0cac

Copy Source in Atom.CreateBox()

b6d2862

Fixed StringSpan delivery bug

45bcfcc

BigOperator was supported.

e36e8b8

FormulaUtils was created.

d94af8d

"\sqrt" source was supported.

c30dfa1

Box deep copy and source updating.

4445d35

ForNeVeR changed the title ~~[WIP] SourceSpan support~~ [WIP] StringSpan support Apr 1, 2018

ForNeVeR changed the title ~~[WIP] StringSpan support~~ [WIP] SourceSpan support Apr 1, 2018

Rename StringSpan into SourceSpan (#115)

d33430b

ForNeVeR force-pushed the feature/115-source-support branch from b12d986 to d33430b Compare April 1, 2018 13:19

This was referenced Apr 7, 2018

Move all the Atoms and Boxes to the corresponding namespaces #124

Closed

Make all the Atoms immutable #125

Closed

ForNeVeR added 3 commits April 11, 2018 23:59

Fix project compilation (#115)

8579fa9

Fix TexFormula.Add bug (#115)

f0e80bc

ForNeVeR added 2 commits April 14, 2018 21:12

Fix source detection for rows (#115)

d976cf8

Fix box source assignment for the RowAtom (#115)

a1777b2

ForNeVeR commented Apr 14, 2018

View reviewed changes

Fix source assignment for BigOperatorAtom (#115)

e88f620

ForNeVeR changed the title ~~[WIP] SourceSpan support~~ SourceSpan support Apr 15, 2018

ForNeVeR requested review from alexreg and gsomix April 15, 2018 06:08

Use nameof in dependency property definitions (#123)

1d1297b

ForNeVeR mentioned this pull request Apr 15, 2018

\colorbox renders root index invisible #128

Closed

ForNeVeR commented Apr 16, 2018

View reviewed changes

gsomix approved these changes Apr 27, 2018

View reviewed changes

Small style fixes (#115)

baa74fd

ForNeVeR merged commit baa74fd into master Apr 27, 2018

ForNeVeR deleted the feature/115-source-support branch April 27, 2018 16:12

ForNeVeR mentioned this pull request Apr 27, 2018

Add StringSpan type to save the source information in Atoms and Boxes #116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SourceSpan support #123

SourceSpan support #123

ForNeVeR commented Apr 1, 2018 •

edited

Loading

ForNeVeR commented Apr 5, 2018 •

edited

Loading

alexreg commented Apr 5, 2018

ForNeVeR commented Apr 6, 2018

ForNeVeR commented Apr 14, 2018

ForNeVeR commented Apr 14, 2018 •

edited

Loading

ForNeVeR Apr 14, 2018

ForNeVeR Apr 14, 2018

ForNeVeR Apr 15, 2018 •

edited

Loading

ForNeVeR Apr 14, 2018

ForNeVeR commented Apr 15, 2018

ForNeVeR Apr 16, 2018

ForNeVeR Apr 27, 2018

gsomix left a comment

gsomix commented Apr 27, 2018

ForNeVeR commented Apr 27, 2018

alexreg commented Apr 27, 2018

SourceSpan support #123

SourceSpan support #123

Conversation

ForNeVeR commented Apr 1, 2018 • edited Loading

TODO

ForNeVeR commented Apr 5, 2018 • edited Loading

alexreg commented Apr 5, 2018

ForNeVeR commented Apr 6, 2018

ForNeVeR commented Apr 14, 2018

ForNeVeR commented Apr 14, 2018 • edited Loading

ForNeVeR Apr 14, 2018

Choose a reason for hiding this comment

ForNeVeR Apr 14, 2018

Choose a reason for hiding this comment

ForNeVeR Apr 15, 2018 • edited Loading

Choose a reason for hiding this comment

ForNeVeR Apr 14, 2018

Choose a reason for hiding this comment

ForNeVeR commented Apr 15, 2018

ForNeVeR Apr 16, 2018

Choose a reason for hiding this comment

ForNeVeR Apr 27, 2018

Choose a reason for hiding this comment

gsomix left a comment

Choose a reason for hiding this comment

gsomix commented Apr 27, 2018

ForNeVeR commented Apr 27, 2018

alexreg commented Apr 27, 2018

ForNeVeR commented Apr 1, 2018 •

edited

Loading

ForNeVeR commented Apr 5, 2018 •

edited

Loading

ForNeVeR commented Apr 14, 2018 •

edited

Loading

ForNeVeR Apr 15, 2018 •

edited

Loading