Results keyboard navigation, keyboard shortcuts and fixes #67

Filios92 · 2018-02-04T21:24:13Z

Hi,

Here's a bunch of features I added some time ago and would like to share, could be merged to master.

mainly keyboard navigation of results with vim-like j/k and with page up/page down. Also watching selection change for highlighting result on mouse click, as a side effect navigation with arrows also works :)
closing opened tabs/views with results with keyboard shortcut, I find it useful (["ctrl+l", "ctrl+x"])
keyboard shortcut for rebuilding database (["ctrl+l", "ctrl+f"])
fix for results such as the ones found in case statements ("case SYMBOL:") which were falling in the '(.*):$' regex. Changed in code and syntax file for Lookup Results
on rebuilding the database, when none is found, set root path to the first path in project paths, even if there is more than one
and if it has more than one treat the next ones as additional source dirs. Not 100% sure about use cases for that (except for mine), so made it an option, disabled by default

…, fix for file path regex

…ing opened tabs and rebuilding DB, OSX fix

…dirs

vanrijn · 2018-04-05T20:26:02Z

Hi @Filios92!! I meant to answer months ago and got way behind. So sorry about that. Thanks so much for the patch! I'll look through this now. =:)

vanrijn

I appreciate all the hard work you've done here, but you've submitted a large chunk of changes that are for different features. I'd rather review each piece of functionality individually. As I see it, the logical groups here are:

Allow multiple project directories to be passed to cscope for database operations
Add arguments to the CscopeVisiter class so that movement can be added
Add keybindings and commands for the above
Change the way results are displayed in a way I don't understand

So... this change is too big as it is. I'd like to focus on the first 2 pieces of functionality first and then look at the other pieces separately. Also, please add comments generously as to exactly what the code is that you're proposing be added. There's a lot of very non-obvious changes you're proposing here.

If you can do that, I'd appreciate it and we can re-look at this patch. =:)

Thanks!

vanrijn · 2018-04-05T20:30:17Z

cscope.py

            root_re = re.compile(r'In folder (.+)')
-            filepath_re = re.compile(r'^(.+):$')
+            filepath_re = re.compile(r'^(((?!scope: ).)+):$')


Can you explain this change please? Why the change in regexp?

If line with symbol ends with ':' it is catched by this regex (seen below), which leads to such error when trying to open results lower than that.

Unable to open file: E:\Dev\C\sandbox\cscope_tmp\ 17 [scope: main] case ENUM_TESTCASE_1

vanrijn · 2018-04-05T20:30:49Z

Lookup Results.hidden-tmLanguage

@@ -9,7 +9,7 @@
 	<array>
 		<dict>
 			<key>match</key>
-			<string>(.*):$</string>
+			<string>^(((?!scope: ).)*):$</string>


Can you explain why you changed this please? I'm not aware of a problem with this right now, so I'm not sure what this is trying to fix.

If line with symbol ends with ':' it is catched by this regex (seen below, result treated like filename), fix prevents it.

vanrijn · 2018-04-05T20:39:36Z

CscopeSublime.sublime-settings

+
+    // On default build command, whether to treat folders in project (after the 1st one) as
+    // additional source directories
+    "auto_next_source_dirs": false,


I'm curious about the use case here. I've never been part of a project so big that it is split into multiple directories and is not contained within a common parent folder. Can you help me understand what kind of scenario would require this or how you have things set up such that this is useful?

Also, I wonder if we even need auto_next_source_dirs or if we can do the right thing automatically?

Because of this reason I didn't want to make it default option :) In my case the other folders contain library-like project (used by other 'main' projects also) or headers of this library, it is handy to have these sources in the same project. Sublime Text automatically indexes all folders in the project for definitions, so I guess this is similar behavior.
Also, it wasn't possible to update or create database with more than 1 folder in project, because of restriction ... len(project_info_paths) == 1

vanrijn · 2018-04-05T20:40:52Z

Default (Linux).sublime-keymap

@@ -51,6 +51,50 @@
        "command": "cscope_visiter",
        "context": [{"key": "selector", "operand": "text.find-in-files"}]
    },
+    {
+        "keys": ["j"],


I think I'd rather leave out the sublime-keymap changes at least for the first round and just make the changes that are required to support it. Can you remove these sublime-keymap changes for at least this first review and let's get through all the other changes first?

vanrijn · 2018-04-05T20:42:55Z

cscope.py

@@ -41,6 +42,14 @@ def get_setting(key, default=None, view=None):
        pass
    return get_settings().get(key, default)

+def update_result_selection(view, selection_index, selection, update_sel=True):


I'm confused on what this is doing here too. Can you please help me understand what this is doing that it wasn't already doing? Also, I suspect that this will not work with Sublime Text 2. Thus far, while I'm not actively supporting Sublime Text 2, I've been making sure I at least don't break it with changes. Can you verify that this will at least not cause errors in ST2?

I keep results and currently selected results in view settings of every Cscope results.
I checked on ST2 and fixed scrolling to result and disabled class for mouse support.
But. The master branch wasn't working on ST2 :) Minor problems:

trailing comma in settings

ValueError: zero length fields name in format cause of lack of indices in the format specs

vanrijn · 2018-04-05T20:46:46Z

cscope.py

        if self.view.settings().get('syntax') == CSCOPE_SYNTAX_FILE:
+            if args.get('direction'):


Can you help me understand what you're doing here? I gather the concept is to allow navigation in the results buffer using direction increments? Is this assuming there's only 1 cscope results window? I frequently have multiple cscope results windows. I'm not sure what the goal is here.

Yes, keyboard navigation. I also have multiple cscope results windows, no problems. Saved results and currently selected result are held in settings 'results' and 'cscope_results_sel' per cscope results buffer/view.

vanrijn · 2018-04-05T20:47:30Z

cscope.py

+                what = {
+                    'up': -1,
+                    'down': 1,
+                    'up2': -20,


If we are going to do this, I think maybe up/down/pageup/pagedown would be better than up/down/up2/down2 as far as names go.

Agree, changed

vanrijn · 2018-04-05T20:48:53Z

cscope.py

        if command_mode in [0, 4, 6, 8]:
            if nested:
-                match_string = ("{0:>6}\n{1:>6} [scope: {2}] {3}").format("..", match["line"], match["scope"], match["instance"])
+                match_string['head'] = ("{0:>6}\n").format("..")
+                match_string['main'] = ("{0:>6} [scope: {1}] {2}").format(match["line"], match["scope"], match["instance"])


Yeah, going to need help understand what you're doing here too, please, in each of these. I didn't write these regexps originally, so I honestly don't understand what they were doing completely nor what you're changing.

The regexes just format results appearance. I only split them into the 'head' part with the separator ('..') or file path and 'main' part with proper result line, which was needed for keyboard navigation and selecting/highlighting. Effectively there is no difference in appearance.

I would also add the option to the settings for disabling the separator between results altogether.

vanrijn · 2018-11-19T19:57:59Z

Hey @Filios92 , I’m so sorry this has gone so long since I last looked at it. I really do appreciate the work you’ve done here. Unfortunately, I am not going to be able to contribute to this project anymore for the foreseeable future, and I’m not going to be able to work through this and get it included. Maybe @ameyp might be able to see this through to completion?

Also, would you be interested in helping to maintain this plugin long term? If so, please say so and maybe we can get you added as a contributor so you can merge this change in and help keep this little plugin chugging along? =:)

Again, I’m so sorry I can’t keep working with you on this PR. I definitely do not like leaving this halfway done but my hands are tied. =:(

Filios92 added 5 commits February 4, 2018 18:44

Add navigation with j/k/PgUp/PgDn, command to close all results views…

943ad3b

…, fix for file path regex

Creating database in first project path

184ea07

Closing opened moved to main TextCommand, keyboard shortcuts for clos…

e5e94d9

…ing opened tabs and rebuilding DB, OSX fix

Better handling of selection updating

12e0562

Add option for treating next folders in project as additional source …

b9f9a1c

…dirs

vanrijn requested changes Apr 5, 2018

View reviewed changes

Filios92 added 2 commits April 8, 2018 21:54

Fix for ST2, comments and naming

580142c

Remove sublime-keymap changes

c59dc98

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Results keyboard navigation, keyboard shortcuts and fixes #67

Results keyboard navigation, keyboard shortcuts and fixes #67

Filios92 commented Feb 4, 2018

vanrijn commented Apr 5, 2018

vanrijn left a comment

vanrijn Apr 5, 2018

Filios92 Apr 8, 2018

vanrijn Apr 5, 2018

Filios92 Apr 8, 2018

vanrijn Apr 5, 2018

Filios92 Apr 8, 2018

vanrijn Apr 5, 2018

Filios92 Apr 8, 2018

vanrijn Apr 5, 2018

Filios92 Apr 8, 2018

vanrijn Apr 5, 2018

Filios92 Apr 8, 2018

vanrijn Apr 5, 2018

Filios92 Apr 8, 2018

vanrijn Apr 5, 2018

Filios92 Apr 8, 2018

vanrijn commented Nov 19, 2018

		if self.view.settings().get('syntax') == CSCOPE_SYNTAX_FILE:
		if args.get('direction'):

Results keyboard navigation, keyboard shortcuts and fixes #67

Are you sure you want to change the base?

Results keyboard navigation, keyboard shortcuts and fixes #67

Conversation

Filios92 commented Feb 4, 2018

vanrijn commented Apr 5, 2018

vanrijn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanrijn commented Nov 19, 2018