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

Default to placing cursor on root when closing bookmark table #866

Merged
merged 1 commit into from
Aug 3, 2018

Conversation

lifecrisis
Copy link
Contributor

Don't know why this bothered me so much... but it needed to be fixed.

If the cursor is not already positioned on a node when the bookmark
table is closed, then the resulting position of the cursor is not
determined.  Here, we default to positioning the cursor on the
root when the bookmark table is closed.
@PhilRunninger
Copy link
Member

@lifecrisis , I checked on both Mac and Windows, and I don't see any difference between your branch's behavior and that of master's. What steps should I use to recreate the original issue?

@lifecrisis
Copy link
Contributor Author

  1. Toggle on bookmark table. (it helps if you have several bookmarks)
  2. Navigate to the last entry in the table.
  3. Toggle off the bookmark table.
  4. You'll see that the cursor just goes wherever instead of being placed on the tree root.

If the cursor was already on a tree node, we want it to remain on that tree node (which it does with this fix). However, if the cursor was somewhere above the tree, we now just put it on the tree root.

Copy link
Member

@PhilRunninger PhilRunninger left a comment

Choose a reason for hiding this comment

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

Ah! Step 2 is what I was missing. Good catch.

@lifecrisis lifecrisis merged commit 43d5691 into preservim:master Aug 3, 2018
@lifecrisis lifecrisis deleted the bm_toggle branch August 3, 2018 18:05
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.

2 participants