Skip to content

Conversation

@bantic
Copy link
Collaborator

@bantic bantic commented Dec 1, 2015

Add postEditor#toggleSection, postEditor#setRange, editor#selectRange APIs

toggleSection is list-section and list-item aware and will toggle a markup section to/from a list item. Toggling a markup section that is adjacent to a list will join it with that list. Toggling a list item will turn it into a markup section and separate it from the before/after list section. The postEditor run loop will join contiguous list sections of the same type during the before_complete callback queue. This means that deleting a markup section that was between two lists will also cause the lists to be joined together.

There is some internal refactoring of the cursor movement/selection methods. It is possible that a postEditor method returns a position that has a section property that is no longer part of the post abstract after the postEditor's run() finishes. This can happen when a markup section is turned into a listSection+listItem and then (in the before_complete queue) merged with an adjacent list.
As a result, it is no longer always safe to use a returned Position with a call to one of the cursor movement methods on editor.cursor (e.g. moveToPosition/selectRange/selectSection). The postEditor#setRange method is added to handle this case.
Mutation methods on the postEditor that could destroy/detach a section can now introspect the current range and update it if they are removing/changing a section that is part of the active range. The postEditor#_joinContiguousListSections is the only method that does this so far, but eventually all of the methods should be updated to update an active range when necessary. The postEditor automatically renders the updated range at the end of its run loop if postEditor#setRange was called.

This is ready for review and merge. If this looks good I will update the documentation to reflect the new APIs shortly before/after merging.

Possible breaking changes

  • removal of the following methods on cursor:
    • selectSections, moveToSection, moveToPosition. None of these was explicitly marked as public.
  • removal of editor#moveToPosition. This was not explicitly public before
  • The README mentions editor#moveToSection, but this API did not actually exist (it should have been editor.cursor.moveToSection). After merge I'll fix up this part of the readme

abc

fixes #186

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can happen in the walkMarkerableSections above- the array sectionsToChange seems to not serve any logical function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At one point there was an issue with mutation while walking — a section's next would get reassigned and the walkMarkerableSections callback would skip sections. I will try removing this and see if the tests still pass.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it and tests are still 🍏, seems good

@mixonic
Copy link
Contributor

mixonic commented Dec 1, 2015

Was there a bug of note in the linked list implementation?

@bantic
Copy link
Collaborator Author

bantic commented Dec 2, 2015

Was there a bug of note in the linked list implementation?

The heuristic for detecting if an item is already in a list doesn't cover every case. In particular, an item can be the only item in another list, in which case its next and prev will be null, and no error will be thrown. In addition there's a code path that is dead in all circumstances except in this edge case, in which case it (incorrectly) overwrites the tail of the list with the item-from-another-list. This isn't something that we ever encounter in correctly-running code, however, but it does mean the insertion-guard code doesn't protect against this case (and in fact an unusual bug — as described — occurs). I went a little overboard refactoring; I'll separate the LL code into a separate PR for discussion.

@bantic bantic force-pushed the toggle-list-sections-186 branch 8 times, most recently from 8903110 to 5824fb9 Compare December 7, 2015 21:58
@bantic bantic changed the title WIP Toggle list type sections Toggle list type sections Dec 7, 2015
@bantic bantic force-pushed the toggle-list-sections-186 branch from 5824fb9 to 8b69ace Compare December 7, 2015 22:31
@bantic
Copy link
Collaborator Author

bantic commented Dec 7, 2015

@mixonic This is ready for another review when you have a chance

Copy link
Contributor

Choose a reason for hiding this comment

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

So eager to change all these cursor.offsets :-D

  * marked `changeSectionTagName` private in favor of `toggleSection`.
    'ember-mobiledoc-editor' uses `changeSectionTagName` so it will need
    to be updated
  * join contiguous list sections -- when a section adjacent to a
    list section is removed, the postEditor will scan for and join
    contiguous list sections in the before_complete queue
  * put cursor position at start of changed range -- the end of the
    range is not stable (it can move around when joinin contiguous lists
    or when the toggle changes the last section to/from a list item) so
    it is quite difficult to select the entire range after the toggle.
  * remove unused `splitAtListItem`, `splitIntoSections` methods on
    ListSection, ListItem
  * add `MarkupSection.isMarkupSection` property
  * add Section `canJoin`, `join` methods
  * add test to ensure toggling while on a card section is a no-op
  * Add editor#selectRange, postEditor#setRange APIs
  * Remove Cursor `moveToSection`, `selectSections`, `moveToPosition`
  * use `Cursor#selectRange` primarily, instead (called by `editor#selectRange`)
  * Add Range.fromSection convenience method
  * Remove `Editor#moveToPosition` (in favor of `selectRange`)

fixes #186
@bantic bantic force-pushed the toggle-list-sections-186 branch from 18676f9 to 1e47433 Compare December 8, 2015 16:25
bantic added a commit that referenced this pull request Dec 8, 2015
@bantic bantic merged commit f4fc3cc into master Dec 8, 2015
@bantic bantic deleted the toggle-list-sections-186 branch December 8, 2015 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

List items can be enabled, removed based on selections

3 participants