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

Option for avoiding overlay movements #11852

Merged
merged 8 commits into from Dec 12, 2016

Conversation

Projects
None yet
7 participants
@MikeInnes
Contributor

MikeInnes commented May 26, 2016

Right now overlays will often move up and down as you scroll, attempting to stay within the editor view. This makes sense for autocomplete, but less sense for things like annotations in code (see e.g. Ink, Hydrogen packages). Indeed in these cases the jumpiness feels very unstable.

So I added a simple option which turns it off. The name could probably do with refinement.

Edit: more helpful diff. Also, same testing concern as #11864.

@OmarShehata

This comment has been minimized.

Show comment
Hide comment
@OmarShehata

OmarShehata Jun 23, 2016

+1! I'm working on a tool to do some code analysis and annotation, definitely need this.

OmarShehata commented Jun 23, 2016

+1! I'm working on a tool to do some code analysis and annotation, definitely need this.

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Jul 13, 2016

Member

We would need some specs to ensure we don't accidentally break this in the future too.

Member

lee-dohm commented Jul 13, 2016

We would need some specs to ensure we don't accidentally break this in the future too.

@MikeInnes

This comment has been minimized.

Show comment
Hide comment
@MikeInnes

MikeInnes Jul 17, 2016

Contributor

Understood – do you have any comments on the testing concerns I mentioned in the linked issue? In particular,

I can't seem to find current tests and it seems like being headless causes difficulties for testing things like positioning.

Advice on what would make for an acceptable test would be much appreciated.

Contributor

MikeInnes commented Jul 17, 2016

Understood – do you have any comments on the testing concerns I mentioned in the linked issue? In particular,

I can't seem to find current tests and it seems like being headless causes difficulties for testing things like positioning.

Advice on what would make for an acceptable test would be much appreciated.

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Aug 26, 2016

I'm not sure there's a good way to test this. I'll follow up on that.

In the meanwhile, there's a legitimate build failure here.

src/text-editor-presenter.coffee
line 478 Don't use &&, ||, ==, !=, or ! Replace "!" with "not"

Looks like an easy fix to at least get the build passing. 😄

Haacked commented Aug 26, 2016

I'm not sure there's a good way to test this. I'll follow up on that.

In the meanwhile, there's a legitimate build failure here.

src/text-editor-presenter.coffee
line 478 Don't use &&, ||, ==, !=, or ! Replace "!" with "not"

Looks like an easy fix to at least get the build passing. 😄

@MikeInnes

This comment has been minimized.

Show comment
Hide comment
@MikeInnes

MikeInnes Aug 26, 2016

Contributor

Ah yes, missed that – fixed!

Contributor

MikeInnes commented Aug 26, 2016

Ah yes, missed that – fixed!

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Aug 26, 2016

Contributor

Maybe we should phrase the naming in the inverse. Maybe an option like avoidOverflow which defaults to true but you can assign to false? I'd love to hear some other name ideas if you don't like avoidOverflow... that's just off the top of my head.

Contributor

nathansobo commented Aug 26, 2016

Maybe we should phrase the naming in the inverse. Maybe an option like avoidOverflow which defaults to true but you can assign to false? I'd love to hear some other name ideas if you don't like avoidOverflow... that's just off the top of my head.

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Aug 26, 2016

@MikeInnes regarding the testing question. I don't think we have any good way right now to write an automated test that exercises the UI and ensures that overlays stay in the same position as you scroll.

But there's probably a way to write a unit test to test this code in isolation such that the test fails when you revert your changes, but succeeds with your changes. Does that make sense?

Haacked commented Aug 26, 2016

@MikeInnes regarding the testing question. I don't think we have any good way right now to write an automated test that exercises the UI and ensures that overlays stay in the same position as you scroll.

But there's probably a way to write a unit test to test this code in isolation such that the test fails when you revert your changes, but succeeds with your changes. Does that make sense?

@OmarShehata

This comment has been minimized.

Show comment
Hide comment
@OmarShehata

OmarShehata Nov 28, 2016

Any news on this being merged?

OmarShehata commented Nov 28, 2016

Any news on this being merged?

@MikeInnes

This comment has been minimized.

Show comment
Hide comment
@MikeInnes

MikeInnes Nov 28, 2016

Contributor

Apologies for letting this slide for a while. @Haacked Good news is I found the overlay positioning tests (perhaps they are new or I just missed them), so the tests I've just added are checking this really works :)

@nathansobo I'm not wildly opinionated about the name so will be happy to change to whatever the consensus is. avoidOverflow seems sensible – perhaps fixed or stayVisible as alternatives? Aside, it may be worth considering making this mode the default, assuming we can figure out an acceptable approach to backwards compatibility.

Contributor

MikeInnes commented Nov 28, 2016

Apologies for letting this slide for a while. @Haacked Good news is I found the overlay positioning tests (perhaps they are new or I just missed them), so the tests I've just added are checking this really works :)

@nathansobo I'm not wildly opinionated about the name so will be happy to change to whatever the consensus is. avoidOverflow seems sensible – perhaps fixed or stayVisible as alternatives? Aside, it may be worth considering making this mode the default, assuming we can figure out an acceptable approach to backwards compatibility.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 28, 2016

Contributor

Taking a look at the failing tests.

Contributor

nathansobo commented Nov 28, 2016

Taking a look at the failing tests.

Rename `stable: true` to `avoidOverlay: false` and fix tests
As part of the test fixes, I’m honoring the `autoscroll: false` option
in `insertText` and `insertNewline` to avoid inadvertently scrolling the
editor during tests when the editor is modified.
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 28, 2016

Contributor

@MikeInnes can you give me push access to this branch? I made some changes in f4c45c1 that I'd like to apply to this PR before merging.

Contributor

nathansobo commented Nov 28, 2016

@MikeInnes can you give me push access to this branch? I made some changes in f4c45c1 that I'd like to apply to this PR before merging.

@MikeInnes

This comment has been minimized.

Show comment
Hide comment
@MikeInnes

MikeInnes Nov 28, 2016

Contributor

Thanks @nathansobo, I've added you as a collaborator on my fork of atom.

Contributor

MikeInnes commented Nov 28, 2016

Thanks @nathansobo, I've added you as a collaborator on my fork of atom.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 28, 2016

Contributor

Thanks for granting me access. I also just added some documentation for the new option. For future reference, it's now possible to allow repository owners to make changes just to your pull request branch when you open the PR. But being a full collaborator works as well. Thanks for taking the initiative with this change. Hopefully we get a green build.

Contributor

nathansobo commented Nov 28, 2016

Thanks for granting me access. I also just added some documentation for the new option. For future reference, it's now possible to allow repository owners to make changes just to your pull request branch when you open the PR. But being a full collaborator works as well. Thanks for taking the initiative with this change. Hopefully we get a green build.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Dec 12, 2016

Contributor

Figured out a CI failure caused by something on our end. Running another build now and will merge once it's green.

Contributor

nathansobo commented Dec 12, 2016

Figured out a CI failure caused by something on our end. Running another build now and will merge once it's green.

@nathansobo nathansobo merged commit 0a5f74a into atom:master Dec 12, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the MikeInnes:overlay-scroll branch Dec 12, 2016

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Dec 12, 2016

Contributor

Thanks for this contribution @MikeInnes! ⚡️

Contributor

nathansobo commented Dec 12, 2016

Thanks for this contribution @MikeInnes! ⚡️

@MikeInnes

This comment has been minimized.

Show comment
Hide comment
@MikeInnes

MikeInnes Dec 12, 2016

Contributor

Awesome, thanks for the help @nathansobo!

Contributor

MikeInnes commented Dec 12, 2016

Awesome, thanks for the help @nathansobo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment