Delete when in a placeholder should simplify containing expression #117

Happypig375 · 2020-05-24T16:14:39Z

Implements #108

charlesroddie · 2020-05-24T20:54:30Z

Great. Will review on Mon.

charlesroddie · 2020-05-26T10:05:02Z

Functionality seems good from my testing.

charlesroddie · 2020-05-26T10:12:17Z

CSharpMath.Editor/Extensions/MathList.cs

@@ -81,12 +83,66 @@ partial class Extensions {
    }

    public static void RemoveAt(this MathList self, ref MathListIndex index) {
-      index ??= MathListIndex.Level0Index(0);
+      void RemoveAtInnerList<TAtom>(ref MathListIndex index, TAtom atom, int innerListIndex) where TAtom : MathAtom, IMathListContainer {


Can you document this a bit? It's not clear from the name of the function and inputs:

Is atom an Atom that resides at index?

Is atom the thing to be removed, or is instead a subatom of atom residing at innerListIndex within atom?

atom is a MathAtom directly contained in self that contains(index.SubIndex.AtomIndex > -1)/is(index.SubIndex.AtomIndex = -1) the atom to remove.

OK that takes us closer. What would be the right type for AtomIndex to have? Nullable<int>?

An AtomIndex of -1 indicates deletion at the beginning of line (not pointing to any atom). It is not a valid value for other methods accepting MathListIndex, which is why PreviousOrBeforeWholeList is internal.

charlesroddie · 2020-05-26T10:14:13Z

CSharpMath.Editor/Extensions/MathList.cs

+          if(atom.Subscript.Count > 0) self[tempIndex.AtomIndex - 1].Subscript.Append(atom.Subscript);
+        } else atom.InnerLists.ElementAt(innerListIndex).RemoveAt(ref index.SubIndex);
+      }
+      void RemoveAtInnerScript(ref MathListIndex index, MathAtom atom, bool superscript) {


Same for this function

checks pass again

Happypig375 · 2020-06-16T16:27:15Z

@charlesroddie What is the status of this?

CSharpMath.Editor/MathListIndex.cs

…n wrong places

charlesroddie · 2020-06-17T09:28:49Z

Main issue is the -1s. I've started doing a bit of cleaning to try to understand this. Removed some refs which allow removing a duplicate "index" input. (Before there were two indexes which were supposed to be the same.) Also moved IsBeforeSubList and PreviousOrBeforeWholeList to prevent usage in the wrong places. It will take more thinking and cleaning to be sure what to do about the -1s.

Options: 1. keep the current hack, 2. adjust the docs on MathListIndex to allow it to be either the current "points to a particular character in the MathList" or "a sublist in a MathList", 3. avoid using MathListIndex for deletes. I haven't understood the code well enough to decide on this.

Happypig375 · 2020-06-17T09:41:50Z

Does it make sense for a data structure to have a Replace method?

Happypig375 · 2020-06-17T09:44:47Z

CSharpMath.Editor/Extensions/MathList.cs

@@ -81,11 +81,25 @@ partial class Extensions {
          throw new SubIndexTypeMismatchException(index);
      }
    }
-
+  public static MathListIndex? PreviousOrBeforeWholeList(MathListIndex index) {


Should this be an extension method?

Perhaps. I tried to move it inside the Delete method because the xml comment said "only use from Delete..." but it also needed to be used in DeleteBackwards in MathKeyboard.

charlesroddie · 2020-06-17T10:09:36Z

Does it make sense for a data structure to have a Replace method?

The items are settable anyway so the Replace method isn't doing anything that can't be done without it. I think it's better for clarity than having the ref inputs. Although immutable will be better still :).

Happypig375 · 2020-06-17T10:13:00Z

The items are settable anyway so the Replace method isn't doing anything that can't be done without it. I think it's better for clarity than having the ref inputs. Although immutable will be better still :).

With ref, I can easily make a "copy" by assigning the value to a new variable. Now a new Copy method is required. With immutability, even more parameters will need to be passed around as each operation will produce a new MathListIndex.

Happypig375 · 2020-06-23T15:37:37Z

@charlesroddie What now?

charlesroddie · 2020-06-24T21:17:20Z

Currently MathListIndex is well documented. We can't disobey its definition.

The code in MathList.cs I haven't been able to understand from the names and type signatures. So I wouldn't be able to make a start documenting it. To get this mergeable we'll need to document the functions in MathList.cs and make the usage of MathListIndex compatible with its definition. I'm not sure which of these two is the best place is to begin.

I'm happy to take a call on Sat UK morning to look at this code.

Do you think we should change MathListIndex to point to any atom in a MathList, rather than just a character at a leaf of the MathList? Or does another type need to be created for this purpose?

charlesroddie · 2020-07-07T22:21:55Z

Let's separate the behaviour from the data structures a little. Also this is a bit of a tangent. Personally I prefer treating "sin" as one thing, so you cannot navigate inside it. That's the behavior of LyX for example. Word is illogical about this so we can ignore that.

An atom is a basic unit (regardless of whether the smallest atom is "s" or "sin" in "sin"). So a MathListIndex can index into an atom and we can replace "character" with "atom". But that's not the tricky change here.

Happypig375 · 2020-07-08T06:37:57Z

Let's separate the behaviour from the data structures a little. Also this is a bit of a tangent.

What behaviour can be done depends on the data structure. If the structure disallows certain states, then behaviour relying on those states cannot exist. The resolution on MathListIndex's definition directly affects #83.

Personally I prefer treating "sin" as one thing, so you cannot navigate inside it. That's the behavior of LyX for example.

Then again, LyX does not have #83, as s i n does not produce \sin and \sin is its own button, just like current CSharpMath.

Word is illogical about this so we can ignore that.

s i n Space produces \sin, but deleting inside individual characters inside \sin makes roman characters, not reverting to variables. This does seem like unwanted behaviour.

An atom is a basic unit (regardless of whether the smallest atom is "s" or "sin" in "sin").

Then again, we can potentially enable dividing MathAtoms into characters just like how we can divide atoms into subatomic particles.

So a MathListIndex can index into an atom and we can replace "character" with "atom". But that's not the tricky change here.

The original documentation from iosMath does seem to use "character" and "atom" interchangeably, so this is also a solution.

charlesroddie · 2020-07-11T22:11:08Z

I'm juggling a few things but will have another look at this tomorrow. Can you explain what "deletion at the beginning of line (not pointing to any atom)" means? Perhaps an example?

Happypig375 · 2020-07-12T15:30:57Z

charlesroddie · 2020-07-12T15:34:11Z

I see thanks. And do we need to do anything in that case? Do you have a strong preference about the right behavior when you press delete there?

Happypig375 · 2020-07-12T15:35:41Z

I prefer the behaviour as done in this PR before your commits.

charlesroddie · 2020-07-12T15:38:08Z

MathQuill deletes the "sin" and moves the 789 down and so does LyX. Word selects the entire "sin^789" expression.

What did this branch do? I don't remember committing any behavioural changes in this branch.

Happypig375 · 2020-07-12T15:47:26Z

What did this branch do?

I followed MathQuill.

I don't remember committing any behavioural changes in this branch.

Yes. See all the failed tests.

Happypig375 · 2020-07-16T15:28:28Z

This is a draft until tests pass.

Happypig375 · 2020-08-05T07:06:08Z

What's the status of this PR?

charlesroddie · 2020-08-09T23:35:39Z

Sorry for delay. I'll look at this today

…ybadcat/CSharpMath into issue108/DeleteInPlaceholder

charlesroddie · 2020-08-11T00:03:10Z

What is a "finalized list"? (Wording used in many places and connected with a finalize:bool.)

charlesroddie · 2020-08-11T01:10:24Z

I've left TODOs for what would be needed to merge this.

The major functions are still unclear to me and I would have to piece them together from their implementations which would take a long time.

My guesses about the implementation: the cursor is at a certain position (also described by a MathListIndex I presume?), and there may be a MathListIndex describing something to be deleted to the left of it (your diagram of \sin^{789}, top). There may not be (your diagram, bottom), in which case an invalid MathListIndex is generated. This should not be done; instead the logic should go something like this:

let rec delete(atom, cursorPosition:MathListIndex) =
    match cursorPosition.SubIndex with
    | None -> // (this case is not reached recursively)
        if cursorPosition.AtomIndex = 0 then atom // do nothing
        else remove(atom, getMathIndexBefore cursorPosition)
    | Some subIndex ->
        if subIndex.Subindex = None && subIndex.AtomIndex = 0 then // on left of SubIndex
            match SubIndexType with
            | Superscript -> atom.Superscript // replace the atom with the superscript part as in the comments on your diagram
            | Subscript -> atom.Subscript
            ...
        else
            atom with the subindexed part `inner` replaced with delete(inner,cursorPosition.SubIndex)

Doesn't have to be done exactly like this but note the non-use of invalid MathListIndexes.

charlesroddie · 2020-08-11T08:34:04Z

I believe a MathListIndex should have a SubIndex if an only if SubIndexType <> MathListSubIndexType.None.

We should bring this constraint into the type structure: remove MathListSubIndexType.None and have

public (MathListSubIndexType SubIndexType, MathListIndex SubIndex)? SubIndexInfo;

I've started this but not ready to commit.

Happypig375 · 2020-08-12T17:42:46Z

What is a "finalized list"? (Wording used in many places and connected with a finalize:bool.)

Unfinalized list	Finalized list
Each digit is its own Number atom	Digits are concatenated into one Number atom
BinaryOperators with no left argument stay as BinaryOperators	They are converted to UnaryOperators
Atoms don't have Range and FusedAtoms	Range and FusedAtoms are assigned

Happypig375 · 2020-08-12T17:46:03Z

Regarding invalid indexes: Now we use MathListIndex to indicate both spaces between atoms (remove) and atom slots (insert)?

charlesroddie · 2020-08-16T15:47:18Z

Regarding invalid indexes: Now we use MathListIndex to indicate both spaces between atoms (remove) and atom slots (insert)?

I think you meant remove and insert the other way around? The interpretation for Atoms is now clear (at least pending resolution of BetweenBaseAndScripts). Representing cursor positions is something I'm trying to understand in order to document. Best to work on this in the other PR (cleaning up MathListIndex). Help appreciated with documentation of the existing structure and code and implementation of BetweenBaseAndScripts there. That will clear a significant piece of technical debt and allow looking at this PR again.

Happypig375 · 2020-08-31T12:05:27Z

The best way of testing the indexes is to debug CSharpMath.Forms.Example.UWP, navigate to the Editor tab, and insert/navigate.

Happypig375 added 6 commits May 24, 2020 18:19

Implemented for fractions, radicals and superscripts

6a3d01a

Added tests

9e8e435

More tests

98aa35d

Tests for Inner

eccb0a8

Delete trash

ab4a4a7

Tests for delete at beginnning of script when both scripts are present

638bd1e

Happypig375 added the Type/Enhancement label May 24, 2020

Happypig375 requested a review from charlesroddie May 24, 2020 16:14

Happypig375 added 2 commits May 25, 2020 00:22

Also add power counterpart for this test

f6db396

And placeholder counterparts

8e2d2ca

Deleting placeholders at the start should not eliminate scripts

fc58258

charlesroddie reviewed May 26, 2020

View reviewed changes

Happypig375 added 2 commits May 28, 2020 00:29

More range checking

4ec984c

Merge branch 'master' into issue108/DeleteInPlaceholder in order to make

5616369

checks pass again

Happypig375 mentioned this pull request Jun 2, 2020

Navigation by up/down arrows #124

Draft

Remove refs on MathListIndex inputs

f018231

Happypig375 commented Jun 17, 2020

View reviewed changes

CSharpMath.Editor/MathListIndex.cs Outdated Show resolved Hide resolved

Move IsBeforeSubList and PreviousOrBeforeWholeList to prevent usage i…

85427c6

…n wrong places

Rename SetTo to ReplaceWith

4858a68

Happypig375 commented Jun 17, 2020

View reviewed changes

This was referenced Jul 14, 2020

Sized brackets #82

Open

Recognized common functions #83

Open

Happypig375 marked this pull request as draft July 16, 2020 15:28

Merge branch 'master' into issue108/DeleteInPlaceholder

bf87ef0

charlesroddie added 3 commits August 11, 2020 00:42

Merge branch 'issue108/DeleteInPlaceholder' of https://github.com/ver…

da9c95b

…ybadcat/CSharpMath into issue108/DeleteInPlaceholder

tweak wording

4d2b72a

remove unnecessary null checks

c8675a1

charlesroddie added 4 commits August 11, 2020 01:15

add documentation TODOs

ac01721

Add TODOs

5b1ad8e

failing test is unnecessary

9e91a94

add TODO

5a764bb

charlesroddie mentioned this pull request Aug 12, 2020

[WIP] Tidy MathListIndex #159

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete when in a placeholder should simplify containing expression #117

Delete when in a placeholder should simplify containing expression #117

Happypig375 commented May 24, 2020

charlesroddie commented May 24, 2020

charlesroddie commented May 26, 2020

charlesroddie May 26, 2020 •

edited

Loading

Happypig375 May 26, 2020 •

edited

Loading

charlesroddie May 27, 2020

Happypig375 May 27, 2020 •

edited

Loading

charlesroddie May 26, 2020

Happypig375 commented Jun 16, 2020

charlesroddie commented Jun 17, 2020 •

edited

Loading

Happypig375 commented Jun 17, 2020

Happypig375 Jun 17, 2020

charlesroddie Jun 17, 2020

charlesroddie commented Jun 17, 2020

Happypig375 commented Jun 17, 2020

Happypig375 commented Jun 23, 2020

charlesroddie commented Jun 24, 2020 •

edited

Loading

charlesroddie commented Jul 7, 2020

Happypig375 commented Jul 8, 2020

charlesroddie commented Jul 11, 2020

Happypig375 commented Jul 12, 2020

charlesroddie commented Jul 12, 2020

Happypig375 commented Jul 12, 2020

charlesroddie commented Jul 12, 2020

Happypig375 commented Jul 12, 2020

Happypig375 commented Jul 16, 2020

Happypig375 commented Aug 5, 2020

charlesroddie commented Aug 9, 2020

charlesroddie commented Aug 11, 2020

charlesroddie commented Aug 11, 2020 •

edited

Loading

charlesroddie commented Aug 11, 2020

Happypig375 commented Aug 12, 2020

Happypig375 commented Aug 12, 2020

charlesroddie commented Aug 16, 2020

Happypig375 commented Aug 31, 2020

Delete when in a placeholder should simplify containing expression #117

Are you sure you want to change the base?

Delete when in a placeholder should simplify containing expression #117

Conversation

Happypig375 commented May 24, 2020

charlesroddie commented May 24, 2020

charlesroddie commented May 26, 2020

charlesroddie May 26, 2020 • edited Loading

Choose a reason for hiding this comment

Happypig375 May 26, 2020 • edited Loading

Choose a reason for hiding this comment

charlesroddie May 27, 2020

Choose a reason for hiding this comment

Happypig375 May 27, 2020 • edited Loading

Choose a reason for hiding this comment

charlesroddie May 26, 2020

Choose a reason for hiding this comment

Happypig375 commented Jun 16, 2020

charlesroddie commented Jun 17, 2020 • edited Loading

Happypig375 commented Jun 17, 2020

Happypig375 Jun 17, 2020

Choose a reason for hiding this comment

charlesroddie Jun 17, 2020

Choose a reason for hiding this comment

charlesroddie commented Jun 17, 2020

Happypig375 commented Jun 17, 2020

Happypig375 commented Jun 23, 2020

charlesroddie commented Jun 24, 2020 • edited Loading

charlesroddie commented Jul 7, 2020

Happypig375 commented Jul 8, 2020

charlesroddie commented Jul 11, 2020

Happypig375 commented Jul 12, 2020

charlesroddie commented Jul 12, 2020

Happypig375 commented Jul 12, 2020

charlesroddie commented Jul 12, 2020

Happypig375 commented Jul 12, 2020

Happypig375 commented Jul 16, 2020

Happypig375 commented Aug 5, 2020

charlesroddie commented Aug 9, 2020

charlesroddie commented Aug 11, 2020

charlesroddie commented Aug 11, 2020 • edited Loading

charlesroddie commented Aug 11, 2020

Happypig375 commented Aug 12, 2020

Happypig375 commented Aug 12, 2020

charlesroddie commented Aug 16, 2020

Happypig375 commented Aug 31, 2020

charlesroddie May 26, 2020 •

edited

Loading

Happypig375 May 26, 2020 •

edited

Loading

Happypig375 May 27, 2020 •

edited

Loading

charlesroddie commented Jun 17, 2020 •

edited

Loading

charlesroddie commented Jun 24, 2020 •

edited

Loading

charlesroddie commented Aug 11, 2020 •

edited

Loading