Comment 1 by blindbhavya on 2014-09-14 14:57
Hi.
I too feel this feature could be very useful.
Something similar has been implemented in JAWS 16 as well.

Comment 4 by vgjh2005 on 2014-09-16 01:53
Hi:
http://community.nvda-project.org/ticket/3989

Comment 5 by jteh (in reply to comment 4) on 2014-09-16 02:46
Replying to vgjh2005:

http://community.nvda-project.org/ticket/3989

This ticket (#4458) does duplicate #3989 to some extent. However, #3989 is confusing because it covers multiple issues and this ticket is a bit clearer, so let's address the search functionality here.

Comment 6 by zahari_bgr on 2014-10-31 18:45
Hi,
Here is my solution (see the attached t4458.diff).
I've added a TextCtrl for the filter, extracted the code that populates the treeview in a separate method and used a regular expression for the actual filtering.
Additionally, I delete the empty categories. I've also mensioned the new field with one sentense in the user guide.

You can find this on my bitbucket repository under t4458 branch:
https://bitbucket.org/zahyur/nvda.git

Comment 7 by nvdakor on 2014-10-31 19:29
Hi,
The solution works.
A few suggestions:

• I suggest putting filter by field below gestures treeview for consistency (I should edit my description then...).
• When NVDA is filtering out commands, would it be possible to expand the gestures treeview and show the name of the command itself? That way the user can immediately see the name of the command (the docstring, that is). This has a problem: what if we get results from multiple categories?
  I think your solution works (at least from user's perspective; I dare not perform code review, as I believe the lead devs have better authority on that). Thanks again for your solution.

Comment 8 by jteh (in reply to comment 6) on 2014-11-01 02:08
Thanks! I haven't looked at this yet, but:

Replying to zahari_bgr:

used a regular expression for the actual filtering.

Why a regular expression? At least for the Elements List, it's a simple text filter, so you can just .lower() both strings and use the "in" operator.

Comment 9 by zahari_bgr (in reply to comment 7) on 2014-11-01 19:06
Hi,
the user doesn't necessarily know what they're searching fore. It may be a well known command they wanna change, or they may just want to see what is available.

Replying to jteh:

Why a regular expression? At least for the Elements List, it's a simple text filter, so you can just .lower() both strings and use the "in" operator.

I've used regular expression, cause it allows me to do a more complex search, i.e. I split the filter by words and then construct a regular expression using positive lookaheads for every word, which allows searching for the given keywords at any position and in any order.
filterReg = re.compile(r'(?=.?' + r')(?=.?'.join(filter.split(' ')) + r')', re.U|re.IGNORECASE)

Replying to nvdakor:

• I suggest putting filter by field below gestures treeview for consistency (I should edit my description then...).

I was led by the expression that in most of the programs (or websites) which provide some kind of search, the input field is above the content (or search results), so I didn't think when I placed it before the treeview - it looked natural to me; like you've said - type some words and press tab smile.
On the other hand, while the focus is on the treeview on initialization, if one doesn't know about the filter box, they could easily miss it. So may be we should move it, or give it the initial focus?

• When NVDA is filtering out commands, would it be possible to expand the gestures treeview and show the name of the command itself? That way the user can immediately see the name of the command (the docstring, that is). This has a problem: what if we get results from multiple categories?

Yeah, we could probably read the first command programmatically. But which items we should expand and when? And should we collapse them afterwords?
Should we do something on pressing Enter when the cursor is on the Filter field? Like execute the highlighted command?

Thanks,
Zahari

Comment 10 by nvdakor on 2014-11-01 19:11
Hi,
Comnet:8:
Those are good points. As for pressing ENTER key, I think it should move focus to the treeview, as the dialog is used to lookup the gesture and/or reassign commands.
Thanks.

Comment 11 by jteh (in reply to comment 9) on 2014-11-03 07:15
Very nice work!

Replying to zahari_bgr:

I've used regular expression, cause it allows me to do a more complex search, i.e. I split the filter by words and then construct a regular expression using positive lookaheads for every word, which allows searching for the given keywords at any position and in any order.

Very interesting and potentially quite useful. Makes sense. Please add a comment to this effect above the regexp.

I was led by the expression that in most of the programs (or websites) which provide some kind of search, the input field is above the content (or search results), so I didn't think when I placed it before the treeview - it looked natural to me; like you've said - type some words and press tab smile.

It's true that it's inconsistent with the Elements List, but I agree with your reasoning. The Elements List is a bit trickier because there are other controls before the tree as well that are arguably more important than the filter box.

On the other hand, while the focus is on the treeview on initialization, if one doesn't know about the filter box, they could easily miss it. So may be we should move it, or give it the initial focus?

While I do understand your argument, I think the initial focus should still stay on the tree view. As you note above, having to shift+tab to the field is a common pattern these days.

Yeah, we could probably read the first command programmatically. But which items we should expand and when?

I don't think we should read or move to any specific items by default, as there's no "best match" here. Perhaps all of the categories could be expanded whenever we filter, since the user wants to see filtered items.

And should we collapse them afterwords?

When the filter is cleared, they should be collapsed.

Should we do something on pressing Enter when the cursor is on the Filter field? Like execute the highlighted command?

I don't think so.

A few other points:

1. nit: General convention is to first import standard library modules, then external packages, then modules for this project. (Unfortunately, half our modules don't follow this convention, though this one does.) Therefore, please move the "import re" just below "import copy".
2. Unless I'm missing something, there's no need to call inputCore.manager.getAllGestureMappings every time we filter, as that's quite expensive. It should be saved to an instance variable in makeSettings and used in populateTree thereafter.
3. Filtering kills any gestures added/removed after the dialog was open. Added/removed gestures aren't actually committed until the user presses the OK button. Until then, they're kept in pendingAdds and pendingRemoves on the dialog. You should be able to fix this by updating the gestures in the original data structure that populateTree uses. See the _addChoice method, though you may need to pass the item data for the command to that method (and _addCapture which calls it). You'll need to do similarly in onRemove. This part of the code is possibly a bit hard to follow, so let me know if you have trouble.
4. While you're at it, please change "a random order" to "any order" in the User Guide addition.

Thanks!

Comment 12 by zahari_bgr on 2014-11-05 00:52
Hi,
I made the required changes and committed to my bitbucket repository.
Thanks.

Comment 13 by jteh on 2014-11-05 11:52
Fantastic. Just 3 comments/questions.

I forgot to ask you about this in the first code review; sorry. :( It's probably better to not compile/match against the regexp at all if there's no filter. You could just set it to None and don't try to match if it's None.

Is there any reason you expand/collapse the items (in onFilterChange) after adding them all? I imagine you could just expand the category tree items when adding them in populateTree if there is a filter. If there isn't a filter, you don't need to do anything, since I think new items will be collapsed by default.

In _addChoice, I just realised (at least, I'm pretty sure) that scriptInfo is actually exactly what you need, so you can just append to scriptInfo.gestures without having to get the parent tree item, etc. Same for onRemove. This needs testing, but if I'm right, sorry for the confusion. :)

Attachment t4458.diff added by zahari_bgr on 2014-11-05 17:30
Description:

Comment 14 by zahari_bgr (in reply to comment 13) on 2014-11-05 17:48
Hi,
Replying to jteh:

It's probably better to not compile/match against the regexp at all if there's no filter. You could just set it to None and don't try to match if it's None.

I wanted to avoid too many checks, but you're right - no need to compile the regexp in this case.

Is there any reason you expand/collapse the items (in onFilterChange) after adding them all?

At first I wanted to use ExpandAllChildren() and CollapseAllChildren() without realizing that ExpandAllChildren() will expand all ansesters , and after that I was misled by that. Now I changed it the way you suggest.

In _addChoice, I just realised (at least, I'm pretty sure) that scriptInfo is actually exactly what you need, so you can just append to scriptInfo.gestures without having to get the parent tree item, etc. Same for onRemove.

Yeah, you are right - it works exactly the same - I've changed it now.
Thanks.

Comment 15 by James Teh <jamie@... on 2014-11-06 03:40
In [9eaf19e]:

In the Input Gestures dialog, you can show only gestures containing specific words using the new "Filter by" field.

Re #4458.

Comment 16 by jteh on 2014-11-06 03:41
Changes:
Milestone changed from None to next

Comment 17 by James Teh <jamie@... on 2014-11-06 04:46
In [3f3b1bf]:

Merge branch 't4458' into next

Incubates #4458.

Changes:
Added labels: incubating

Comment 18 by blindbhavya on 2014-11-06 05:42
Hi,
'In the Input Gestures dialog, you can show only gestures containing specific words using the new "Filter by" field.'
I read this statement, and to me it suggests that now I have to enter some text into the Filter by field and get only specific mathcing results gestures, when i thought that using this field wasn't compulsory, is it? If it isn't compulsory, then, I think, (IMO, that was what it conveyed to me) this line should be reworded.

Comment 19 by jteh on 2014-11-06 06:02
No, it's optional. The sentence says "can show only" rather than 'can only show". The latter would imply it's not optional, whereas the former doesn't enforce this. Still, I can see why it would be confusing.

Here are two alternatives. Let me know which is easier for you to follow.

1. In the Input Gestures dialog, you can choose to show only gestures containing specific words by using the new "Filter by" field.
2. In the Input Gestures dialog, you can use the new "Filter by" field to show only gestures containing specific words.
   Personally, I'm leaning towards option 2.

Comment 20 by nvdakor on 2014-11-06 10:52
Hi,
Found a possible code error: in NVDA next.11235, when tyring to filter a script, I get error sounds.
STR:

1. Go to Input Gestures dialog.
2. Once there, press Shift+TAB to go to the filter box. Type part of the name of a script (say, "quit").
   Expected: No errors.
   Actual: Errors are logged into Log Viewer.
   Log output says:

ERROR - unhandled exception (02:51:05):
Traceback (most recent call last):
  File "gui\settingsDialogs.pyc", line 1642, in onFilterChange
AttributeError: 'NoneType' object has no attribute 'GetValue'

The problem line says evt.GetClientObject().GetValue() which returns None (the client object is None). Changing this to evt.GetEventObject().getValue() resolves this problem. Thanks.

Comment 21 by zahari_bgr on 2014-11-06 13:14
Hi,
Thanks for the report and the given solution.
I pulled the latest next branch from NVAccess and can't reproduce this. May be it exists only in the binary builds?
However, I changed GetClientObject() to GetEventObject() in my repository.
I think that GetClientObject() returns the object to which the event is bound, and GetEventObject() returns the object, which is passed to the Bind() function (which are the same in this case). It is strange that the former is distroied at some point while the dialog is still active.

Comment 22 by beqa on 2014-11-06 14:32
hi.

i am getting this error when trying to write something in filter by editbox

ERROR - unhandled exception (18:30:19):
Traceback (most recent call last):
File "gui\settingsDialogs.pyc", line 1642, in onFilterChange
AttributeError: 'NoneType' object has no attribute 'GetValue'

Comment 23 by zahari_bgr on 2014-11-06 14:48
Hi,
Ok, we should definitely use GetEventObject() then, as Joseph suggested.
I already made this change and I'm sure Jamie will merge it into next when he is available.
So it will be fixed soon.
Thanks.

Comment 24 by nvdakor on 2014-11-06 19:21
Hi,
What I noticed though is that source code version of t4458 branch does not produce the error when you used GetClientObject(). Somehow, when it was integrated into next is when the problem might have occurred (let's ask Jamie for his opinions). This might also be due to the fact that next branch uses WXPython 3.0.1, whereas master still uses 2.8.12.
Thanks.

Comment 25 by jteh (in reply to comment 21) on 2014-11-07 02:49
Replying to zahari_bgr:

I pulled the latest next branch from NVAccess and can't reproduce this. May be it exists only in the binary builds?

You probably didn't update submodules, so you were running wxPython 2 still. The problem occurs with wxPython 3.

I think that GetClientObject() returns the object to which the event is bound

According to the wx documentation, GetClientObject is only relevant for ListBox/Choice selection events. It seems this is another case of something that used to work in wx 2 but was actually incorrect according to the docs. We've been bitten by a few of those. :)

Btw, while I think of it, for future reference, don't worry about attaching a patch to the ticket if you have a public branch, as we'll just look at the branch.

Comment 26 by James Teh <jamie@... on 2014-11-07 02:54
In [f87ed4d]:

Merge branch 't4458' into next

Incubates #4458.

Comment 27 by blindbhavya (in reply to comment 19) on 2014-11-07 15:02
Replying to jteh:

No, it's optional. The sentence says "can show only" rather than 'can only show". The latter would imply it's not optional, whereas the former doesn't enforce this. Still, I can see why it would be confusing.

Here are two alternatives. Let me know which is easier for you to follow.

1. In the Input Gestures dialog, you can choose to show only gestures containing specific words by using the new "Filter by" field.
2. In the Input Gestures dialog, you can use the new "Filter by" field to show only gestures containing specific words.

Personally, I'm leaning towards option 2.

GO for option 2.

Comment 28 by James Teh <jamie@... on 2014-12-03 04:44
In [2fcf0fc]:

In the Input Gestures dialog, you can show only gestures containing specific words using the new "Filter by" field.

Fixes #4458.

Changes:
Removed labels: incubating
State: closed

Comment 29 by jteh on 2014-12-03 04:47
Changes:
Milestone changed from next to 2015.1

Comment 30 by bhavyashah on 2014-12-03 11:20
Hi,
'In the Input Gestures dialog, you can show only gestures containing specific words using the new "Filter by" field.'
Could you change this to '2. In the Input Gestures dialog, you can use the new "Filter by" field to show only gestures containing specific words. ' as we decided before?
Also, through this comment, let me also check if I have successfully changed my username.

Comment 31 by bhavyashah on 2014-12-03 11:21
Hi,
It looks like I've changed my username successfully, thanks for the help.

Comment 32 by jteh (in reply to comment 30) on 2014-12-03 23:29
Replying to bhavyashah:

'In the Input Gestures dialog, you can show only gestures containing specific words using the new "Filter by" field.'

Yeah, wrong commit message, but the entry in the What's New is correct, which is what users will read.

Comment 33 by bhavyashah on 2014-12-04 16:54
Oh, ok, and apologies, hadn't checked the Master branch.
Thanks

Comment 34 by leonarddr on 2015-02-16 19:56
I'm receiving an error whenever the filter value produces an empty trayview.
STR:

1. Open the input gestures dialog
2. enter 'test' in the filter field
   Result:
   Input: kb(laptop):tab ERROR - eventHandler.executeEvent (20:56:08): error executing event: gainFocus on <NVDAObjects.IAccessible.sysTreeView32.BrokenCommctrl5Item object at 0x04E7E610> with extra args of {} Traceback (most recent call last): File "eventHandler.pyc", line 138, in executeEvent File "eventHandler.pyc", line 151, in doPreGainFocus File "api.pyc", line 107, in setFocusObject File "baseObject.pyc", line 34, in __get__ File "baseObject.pyc", line 110, in _getPropertyViaCache File "NVDAObjects\__init__.pyc", line 469, in _get_container File "baseObject.pyc", line 34, in __get__ File "baseObject.pyc", line 110, in _getPropertyViaCache File "NVDAObjects\IAccessible\sysTreeView32.pyc", line 261, in _get_parent AttributeError: 'Dynamic_DialogIAccessibleWindowNVDAObject' object has no attribute 'UIAElement' IO - speech.speak (20:56:08): Speaking [view'](u'tree)

Comment 35 by jteh on 2015-02-18 03:51
Is there any user noticeable problem apart from the exception (which won't be noticed by most users)? There isn't that i can see. Obviously, we should still fix this, but I don't think it needs to block 2015.1 if I'm right.