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

tell if any selection changed in addition to just first region #2204

Merged
merged 13 commits into from
Feb 23, 2023

Conversation

rchl
Copy link
Member

@rchl rchl commented Feb 17, 2023

Detect whenever any selection region has changed in addition to detecting whether just the first one has changed.

This is an extra functionality that is current not utilized but will be useful for #1702.

Fixes #2182

@jwortmann
Copy link
Member

I haven't looked exactly at all changes in this PR, but I neither get any more DocumentHighlights nor code actions on this branch. So something doesn't seem right at the moment.

plugin/core/views.py Outdated Show resolved Hide resolved
@@ -425,7 +425,15 @@ def text_document_identifier(view_or_uri: Union[DocumentUri, sublime.View]) -> T
return {"uri": uri}


def get_first_selection_region(selelection: sublime.Selection) -> Optional[sublime.Region]:
Copy link
Member

@jwortmann jwortmann Feb 19, 2023

Choose a reason for hiding this comment

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

Is this function really necessary? Now you need to put view.sel() into the argument at all the places where it is called.
And I think you can rewrite the relevant change from this PR in a simpler way without the need to use this function

selection = view.sel()
if len(selection):
    current_first_region = selection[0]
    ...

Copy link
Member Author

@rchl rchl Feb 19, 2023

Choose a reason for hiding this comment

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

And if we want to avoid calling it multiple times then i needed to change it like that. Though I'm not convinced it was necessary to optimize that in the first place

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that len does a call to a native API so it would be in practice less efficient. Does it matter? Probably not.

    def __len__(self):
        return sublime_api.view_selection_size(self.view_id)

    def __getitem__(self, index):
        r = sublime_api.view_selection_get(self.view_id, index)
        if r.a == -1:
            raise IndexError()
        return r

Copy link
Member Author

@rchl rchl Feb 19, 2023

Choose a reason for hiding this comment

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

Well, I figured I should test instead of guessing.

    def test_len():
        return sel[0] if len(sel) else None

    repeat = 100000
    times = timeit.repeat(test_len, repeat=3, number=repeat)
    return 'Best of {}: {} usec.'.format(repeat, min(times))

	# 0.26699970833396947 s.
    def test_trycatch():
        try:
            return sel[0]
        except Exception:
            return None

    repeat = 100000
    times = timeit.repeat(test_trycatch, repeat=3, number=repeat)
    return 'Best of {}: {} usec.'.format(repeat, min(times))

	# 0.4004749166670081 s.

So no, len variant is faster.

Copy link
Member Author

@rchl rchl Feb 19, 2023

Choose a reason for hiding this comment

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

I rewrote _update_stored_selection_async in a different way and reverted first_selection_region name change and parameters.

Copy link
Contributor

@jfcherng jfcherng Feb 20, 2023

Choose a reason for hiding this comment

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

Weird. I get different benchmark results.

import sublime
import sublime_plugin
import timeit


class ExampleCommand(sublime_plugin.TextCommand):
    def run(self, edit: sublime.Edit) -> None:
        sel = self.view.sel()
        repeat = 100000

        def test_len_1():
            return sel[0] if len(sel) else None

        def test_len_2():
            try:
                return sel[0]
            except Exception:
                return None

        def test_len_3():
            return next(iter(sel), None)

        times = timeit.repeat(test_len_1, repeat=3, number=repeat)
        print("(test_len_1) Best of {}: {} usec.".format(repeat, min(times)))
        times = timeit.repeat(test_len_2, repeat=3, number=repeat)
        print("(test_len_2) Best of {}: {} usec.".format(repeat, min(times)))
        times = timeit.repeat(test_len_3, repeat=3, number=repeat)
        print("(test_len_3) Best of {}: {} usec.".format(repeat, min(times)))

with py33 plugin-host results in

(test_len_1) Best of 100000: 0.39621629999999186 usec.
(test_len_2) Best of 100000: 0.2581017000000543 usec.
(test_len_3) Best of 100000: 0.2883709000000181 usec.

Copy link
Member Author

@rchl rchl Feb 20, 2023

Choose a reason for hiding this comment

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

True. I can see the same here when using actual view.sel() rather than manually created sublime.Selection(1).

* view.sel()
(test_len_1) Best of 100000: 0.26706908333332535 usec.
(test_len_2) Best of 100000: 0.4003557916666409 usec.
(test_len_3) Best of 100000: 0.4005454583333119 usec.

* sublime.Selection(1)
(test_len_1) Best of 100000: 0.5351135833333274 usec.
(test_len_2) Best of 100000: 0.26820062500001995 usec.
(test_len_3) Best of 100000: 0.2768708750000428 usec.

Copy link
Member Author

Choose a reason for hiding this comment

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

Which seems to boil down to __len__ implementation having to do an API call and that appears to have more overhead when view ID is not valid (does not exist).

Copy link
Member Author

Choose a reason for hiding this comment

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

Any more comments on this?

plugin/documents.py Outdated Show resolved Hide resolved
@rchl rchl changed the title detect if any selection has changed in addition to just first region tell if any selection changed in addition to just first region Feb 23, 2023
@rchl rchl merged commit 035e2bd into main Feb 23, 2023
@rchl rchl deleted the fix/change-selections branch February 23, 2023 17:33
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.

3 participants