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

Select to the only bookmark #100

Merged
merged 4 commits into from Jan 24, 2019

Conversation

Projects
None yet
4 participants
@alefragnani
Copy link
Contributor

alefragnani commented Oct 1, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This PR adds update the Select to Next Bookmark and Select to Previous Bookmark, to work even if the only bookmark is positioned in the other direction

Alternate Designs

It follows the same behavior of Jump to Next/Previous Bookmark commands.

Benefits

You can take advantage of the selection commands even you don't know exactly if the cursor is above or below

Possible Drawbacks

Nothing at all, since it follows the same behavior as Jump commands

Applicable Issues

This PR closes #94 .

alefragnani added some commits Jan 15, 2018

Merge pull request #1 from atom/master
Update from original
Merge pull request #2 from atom/master
Update from original
@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Oct 15, 2018

The one question I have here is around directionality of the operations. While it makes sense to make "Select to ..." and "Jump to ..." operations work equivalently, selecting a range adds another wrinkle to things. For example, take the following file contents:

1
2
3
4
5
6
7
8
9
0

Let's assume that the cursor is at the beginning of the line containing 8 and the bookmark is on the line contaniing 2. If one executes the command "Jump to next bookmark", it doesn't matter which direction the cursor travels to get there. It doesn't matter if it travels backwards through the file from 8, to 7, to 6, etc, onwards to 2 or if the cursor travels forwards through the file and wraps around from 0 to 1, to 2.

But when executing "Select to next bookmark", directionality does matter. If I am at the line containing 8 and I execute "Select to next bookmark", I would expect the selection, if any, to go from the current location toward the end of the file. But it appears this change makes the selection go from the current location toward the beginning of the file. Is my understanding of this change correct? If so, I don't believe that this is the correct behavior, even when it is the only bookmark in the file.

@alefragnani

This comment has been minimized.

Copy link
Contributor Author

alefragnani commented Oct 16, 2018

Hi @lee-dohm ,

I understand your point, but this PR was created to fix #94, which was created almost an year ago and labeled as bug and triaged because the Jump to ... and Select to ... commands weren't consistent.

I think that the reason for that is: The user should not worry if the unique bookmark is above or below the current cursor position. He/She would like to Jump to / Select to it. At this point, the Jump to command is moving the cursor in the oposite direction (because it does a circular movement) but not the Select to command. That's what I understand to be the bug.

Do you think this behavior (Jump to / Select to) with only one bookmark should have a setting, to define if the user would like this circular movement?

Thanks

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Oct 18, 2018

If, as you say, this is intended to create circular movement, or the illusion thereof, shouldn't two selections be created then in my scenario above?

  1. From the beginning of the line with 8 through the end of the line with 0
  2. From the beginning of the line with 1 through the beginning of the line with 2

If your proposed change creates only one selection as described in my previous comment, why do you believe that is more correct behavior than the two selections I describe here? How would that represent circular movement?

@alefragnani

This comment has been minimized.

Copy link
Contributor Author

alefragnani commented Oct 18, 2018

If your proposed change creates only one selection as described in my previous comment, why do you believe that is more correct behavior than the two selections I describe here? How would that represent circular movement?

I don't think it is more/less correct, than your proposal, this was just my interpretation of the expected behavior described in the original issue.

Expected behavior: All the lines between where the cursor was the momento I pressed F2 and where the bookmark is should be selected.

As I said, I think the reason was the user does not worry where the bookmark is located, it just selects to it. IMHO, create two selections is a bit weird, but it's just my point of view.

I think it would be a good idea to ping @alexandernst (which created the original issue), and @rsese (which labeled bug and triaged), so they could also describe their opinions.

What to you think?

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Oct 19, 2018

@alefragnani Well, you've already taken care of pinging the correct people 😀

FYI @rsese and I work on the same team and were in the same meeting with other Atom team members when we discussed the question of what should be the correct behavior. We all had the same concerns after the discussion, hence the reason why I asked the questions. Additionally, I'm the one that created the triage process for the Atom team, so I'm more than familiar with how we use the triaged and bug labels, neither of which preclude asking questions to ensure that the correct implementation is reached.

Also, while @alexandernst created the issue, the Atom team are the ones that are going to have to maintain the implementation going forward and answer questions as to why things are implemented the way they are when someone disagrees with the decisions that were made. So the Atom development team, of which I am a part, will need to make a final determination on what most people will expect the behavior to be, even if that differs from the original issue description.

I hope that makes things a bit more clear and eases your concerns and frustration.

@alefragnani

This comment has been minimized.

Copy link
Contributor Author

alefragnani commented Oct 19, 2018

@alefragnani Well, you've already taken care of pinging the correct people 😀

😆

I hope that makes things a bit more clear and eases your concerns and frustration.

Don't worry. There is no concern, nor frustration. I know you from the very beginning of Atom. (If I remember correctly, you were an avid forum's user and based on your engagement, you were contacted by the Atom's team to be part of the project). Even if my memory is playing with me, I'm sure that if you ask more info about some issue, you have a good reason to do so 😄 .

I'm contributing to the project because I felt confident about the original issue, and noted that I could contribute a bit more to the project. I'm open to update the PR if necessary.

In fact, I would like to bring some of the features of my Bookmarks extension for VS Code, but of course, only if you (the maintainers) would like to receive. I noted some open issues that I could contribute as well.

Thank you

@alefragnani

This comment has been minimized.

Copy link
Contributor Author

alefragnani commented Nov 1, 2018

Hi @lee-dohm, any news about it?

Would you like me to update the PR in order to create two selection, as you described. Is there any additional change that you would like me to do?

Thank you

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Nov 1, 2018

No news currently. It's awaiting triage by the dev team.

@lee-dohm lee-dohm added the triaged label Nov 19, 2018

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld left a comment

I agree with @lee-dohm that practically speaking, I can't see the utility of the selection wrapping. But:

  • this does improve consistency with the jump-to commands
  • the command was already not very useful in this particular case

So overall, I'm 👍

@alefragnani

This comment has been minimized.

Copy link
Contributor Author

alefragnani commented Nov 21, 2018

That's great @maxbrunsfeld, thank you 👍

@smashwilson smashwilson merged commit 8e39209 into atom:master Jan 24, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Contributor Author

alefragnani commented Jan 24, 2019

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.