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

more options for multi_find_all #31

Merged
merged 4 commits into from
Oct 1, 2017
Merged

more options for multi_find_all #31

merged 4 commits into from
Oct 1, 2017

Conversation

mg979
Copy link
Contributor

@mg979 mg979 commented Sep 29, 2017

No description provided.

@mg979 mg979 closed this Sep 29, 2017
@mg979 mg979 reopened this Sep 29, 2017
newRegions.append(region)

if word:
deleted = []
Copy link
Owner

Choose a reason for hiding this comment

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

Can you extract Lines 24 to 27 into an inline function which takes a region as a parameter and returns a boolean indicating whether the region should be ignored or not. Then, we do not need the deleted list variable and could call said function directly in the for comprehension in Line 28.

Copy link
Owner

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Honestly, I did not use the multi find all functionality yet (it was a PR from another contributor), but with these new features, I can see myself using it :)

However, I decided to not add standard keybindings to ST packages anymore. The likelihood that such keybindings conflict with existing, user-specific keybindings is very high, resulting in a poor user experience. Can you adapt your PR so that the keybindings are only suggested in the README and not added by default in the keymap files?
Also feel free to add the commands to the ST menu.

newRegions = [region for region in newRegions if region not in deleted]

if ignore_comments:
deleted = []
Copy link
Owner

Choose a reason for hiding this comment

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

Same here from Line 31 to 36.

…mmand palette, made keybindings inactive for these commands, updated readme
// multi find all - the first is the same behaviour as old [alt+f3]
{ "keys": ["alt+f3", "alt+f3"], "command": "find_all_under"},
// Multi Find All example keybindings, uncomment to activate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All keybindings are inactive this way

// multi find all - the first is the same behaviour as old [alt+f3]
{ "keys": ["alt+f3", "alt+f3"], "command": "find_all_under"},
// Multi Find All example keybindings, uncomment to activate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All keybindings are inactive this way

@@ -8,6 +8,6 @@
{ "command": "split_selection", "caption" : "MultiEditUtils: Split selection" },
{ "command": "strip_selection", "caption" : "MultiEditUtils: Strip Selection" },
{ "command": "remove_empty_regions", "caption" : "MultiEditUtils: Remove Empty Regions" },
{ "command": "multi_find_all", "caption" : "MultiEditUtils: Multi FindAll" },
{ "command": "multi_find_menu", "caption" : "MultiEditUtils: Multi FindAll" },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of adding all commands(they're a lot) it launches a mini-menu where you can choose

Imgur


# filter selections in order to exclude duplicates since it can hang
# Sublime if search is performed on dozens of selections, this doesn't
# happen with built-in command because it works on a single selection
Copy link
Contributor Author

@mg979 mg979 Sep 30, 2017

Choose a reason for hiding this comment

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

I added this section because it was a bit hang-friendly when repeating searches over previous searches and too many selections, now you can do stuff like performing multiple searches over each other for different selections and with different commands

if view.substr(view.word(region)).lower() not in selected_words:
deleted.append(region)
deleted = [region for region in newRegions
if view.substr(view.word(region)).lower() not in selected_words]
newRegions = [region for region in newRegions if region not in deleted]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's the best I could come with


for region in newRegions:
view.sel().add(region)

class MultiFindAllRegexCommand(sublime_plugin.TextCommand):

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 a new command, additive or subtractive, it works on top of any selection/searches you made

sublime.active_window().show_input_panel(c, "", self.on_done, None, None)

class MultiFindMenuCommand(sublime_plugin.TextCommand):

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 the menu from the command palette

Copy link
Owner

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Thank you, looks very good! Can you fix the one typo and merge afterwards?

"Find All Case + Word -",
"Find All Case - Word +",
"Find All Case - Word -",
"Find All Case + Word + Ignore Comments)",
Copy link
Owner

Choose a reason for hiding this comment

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

Should probably be (Ignore Comments).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops.. thanks! fixed and pushed

@philippotto philippotto merged commit 1bb398e into philippotto:master Oct 1, 2017
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