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

Preserve selection when folding and unfolding blocks #95

Merged
merged 3 commits into from
Oct 26, 2022

Conversation

alexeyinkin
Copy link
Contributor

@alexeyinkin alexeyinkin marked this pull request as ready for review October 26, 2022 13:53
@alexeyinkin alexeyinkin requested a review from Malarg October 26, 2022 13:54

// Linear interpolation search.
while (upperRange > lowerRange) {
// Full characters at the lower ange start and at the upper range end.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo "range"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+

final range = ranges[rangeIndex];

switch (range.compareToPosition(position)) {
case 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAy be will be more readable, if numbers will be changed on "equals", "upper", and "lower"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-1, 0, and 1 are common for sign-like methods.

);
});

test('Cut with visible beginning and end', () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test does not cover case, where text contains hidden ranges at the start and at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is the job for #27
Only with visible sections can the beginning be invisible.

};
int i = 0;

for (final example in examples.entries) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this loop repeat the same at 68 line. Suggest to extract

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+

/// the visible text.
///
/// If [position] is hidden, returns the last visible position before it.
int cutPosition(int position) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be better toHiddenPosition, toVisiblePosition? And also toHiddenSelection' and toVisibleSelection`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is longer. cut... and recover... fit well.

});

group('Collapsed', () {
test('before ranges', () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This 4 tests almost the same. Suggest to extract to a function and pass numbers as parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged with the test below.

),
];

for (final selection in selections) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be this function helps to make this loop more clear

  void _expectTheSame(TextSelection selection) {
    expect(
      HiddenRanges.empty.cutSelection(selection),
      selection,
    );
    expect(
      HiddenRanges.empty.recoverSelection(selection),
      selection,
    );
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hides expect calls which is a bad pattern. They should be clearly visible. In 6 months when something breaks you will want to see expect on the top level.

@alexeyinkin alexeyinkin merged commit ba101db into main Oct 26, 2022
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.

Preserve selection when folding and unfolding
2 participants