add case sensitivity filter option in NVDA Find dialog box #4584

Closed
nvaccessAuto opened this Issue Oct 28, 2014 · 29 comments

1 participant

@nvaccessAuto

Reported by blindbhavya on 2014-10-28 14:15
The summary explains it all, and probably a use case isn't required.
Thanks.

@nvaccessAuto

Comment 1 by jteh on 2014-10-28 21:12
A use case would still be at least interesting. You obviously filed this for a specific reason.

For anyone interested in implementing this, the TextInfo.find method (which is used to do the actual search) already supports case sensitivity. Therefore, the task here is to implement a custom dialog for the Find dialog; see cursorManager.CursorManager.doFindTextDialog. We currently use wx.TextEntryDialog, but we can't add additional options with that.

@nvaccessAuto

Comment 2 by nvdakor on 2014-10-28 23:19
Hi,
If we add case sensitivity checkbox, then the actual find routine (and scripts for NVDA+F3 and NvDA+Shift+F3) may need to be modified as follows:
1. As Jamie suggested, create FindTextDialog which inherits from WX.Dialog. Make sure it is a singleton.
2. Move the code for FindDialog to this new dialog and add a checkbox to set whether NVDA should do case sensitive search or not (I think it should be case insensitive by default).
3. Modify find dialog script to have the variable "D" be an instance of the new find dialog.
4. Modify find next/previous scripts by adding a check for case sensitivity (a boolean that will be set from the checkbox mentioned above).
as for the actual code, I'm thinking we could recycle one of the dialogs (say, code from Config Profiles dialog; I chose this dialog as it already contains logic which guarantees that the new find dialog will be a singleton).
@blindbavya: can you elaborate more on your suggestion - that is, can you provide a use case with examples?
Thanks.

@nvaccessAuto

Comment 3 by nvdakor on 2014-10-28 23:26
Hi,
Come to think of it, there is a better solution:
@mhameed: can we borrow your Unicode Braille Input dialog code, as it contains necessary pieces to implement this ticket? We'll then add the logic to make sure the new dialog is a singleton.
@jteh: in case someone does implement this dialog, where would you like the dialog to be placed in (in the cursor manager module or as part of some other module or object)?
Thanks.

@nvaccessAuto

Comment 4 by jteh on 2014-10-29 01:25
There's not much point in taking an existing dialog and stripping it down, as that will be harder than just writing one from scratch. This will be a pretty simple dialog. You can of course look at existing code for the boilerplate; calling the constructor, setting up sizers, etc.

We probably need to create a subclassable singleton dialog, as there are several cases where this is needed now.

I'd probably just leave the dialog in cursorManager.

@nvaccessAuto

Comment 5 by nvdakor (in reply to comment 4) on 2014-10-29 05:49
Replying to jteh:

There's not much point in taking an existing dialog and stripping it down, as that will be harder than just writing one from scratch. This will be a pretty simple dialog. You can of course look at existing code for the boilerplate; calling the constructor, setting up sizers, etc.

I'm looking at various dialogs to see which one might be the "template" for the new find dialog (welcome dialog seems promising). A new dialog from scratch is indeed in order.

We probably need to create a subclassable singleton dialog, as there are several cases where this is needed now.

If so, can you do it in GUI/init so add-ons can also benefit from this refactor?

I'd probably just leave the dialog in cursorManager.

I agree. This means two things: a FindDialog will be created in CursorManager module, which means CursorManager.doFindDialog is no longer needed, as all the scripts need to do is call FindDialog.run. As for passing the new find text, that can be done from the new dialog, with onOK calling CursorManager.doFind (this guy may need caseSensitive argument too).
Thanks.

@nvaccessAuto

Comment 6 by blindbhavya on 2014-10-29 06:20
Hi,
In Wikipedia articles, where there are very long (informative as well) articles, there are multiple (sometimes too many) occurences of the same word, sometimes used in headings (in headings those words are written in capital), sometimes in the start of sentences (again upper case) and sometimes in the middle of a sentence (here lower case if the word is not a proper noun).
So, I had forgotten exactly where that word was, and had to go through all its instances, and I did not remember exactly what words were around it. I eventually did find the correct and desired instance, but a case sensitivity option would have made me a bit faster.
This doesn't apply to Wikipedia, but all webpages where there is lots and lots of text.
Thanks.
BTW, hope I was clear enough.

@nvaccessAuto

Comment 7 by nvdakor on 2014-10-29 10:52
Hi,
Initial implementation is ready for code review (a proof of concept):

  • URL: https://bitbucket.org/josephsl/nvda-dev
  • Branch: t4584
  • Commit: 709c68e I didn't commit to NV Access server yet as the code needs some finishing touches and to wait until more people agree with the original suggestions. Few things to keep in mind:
  • For now, the title of the dialog is "case sensitive find" to distinguish it from the current find routine.
  • When trying to perform doFindText, Python throws type error, saying int is not callable.
  • I put FindDialog class at the top of Cursor Manager module for now (borrowed code from various dialogs, including that of systray add-on).
  • Modified description of Control+NVDA+F command from browse mode to say that one can tell NVDA to perform case sensitive search. Thanks.
@nvaccessAuto

Comment 8 by blindbhavya on 2014-10-30 08:10
Just a thought...
Are you sure a separate case sensitive NVDA Find dialog is really required, rather than a check box in the existing NVDA Find dialog? In about all the Find dialog boxes I have encountered, there is simply a case sensitivity check box, and that IMO, is a bit cleaner implementation. Also, (though I don't require it, but someone else may ask for it), what if there comes a request for a match whole word only filter in the NVDA Find, creating another NVDA Find dialog wouldn't be ideal, in my views.
These are just my thoughts, and if you disagree with me here, its completely fine, the current implementation is usable as well.

@nvaccessAuto

Comment 9 by blindbhavya on 2014-10-30 08:14
Hi,
I have interpreted the current implementation as creating a separate Find dialog for case sensitive search from the following:
'• For now, the title of the dialog is "case sensitive find" to distinguish it from the current find routine. '
Is my interpretation right?

@nvaccessAuto

Comment 10 by nvdakor (in reply to comment 8) on 2014-10-30 08:18
Replying to blindbhavya:

Just a thought...

Are you sure a separate case sensitive NVDA Find dialog is really required, rather than a check box in the existing NVDA Find dialog? In about all the Find dialog boxes I have encountered, there is simply a case sensitivity check box, and that IMO, is a bit cleaner implementation.

For now, I've titled it that way to let people try both implementations (I'm working on this from a separate branch). At the moment, it's a proof of concept which may need some further touches from other devs before it can be tried out by others.

Also, (though I don't require it, but someone else may ask for it), what if there comes a request for a match whole word only filter in the NVDA Find, creating another NVDA Find dialog wouldn't be ideal, in my views.

As for match whole word, I'm personally against adding this, as the find facility is meant to search for anything (whole word, part of a word, etc.). Case sensiticity might be something to consider (after all, this is what the ticket is about), but not match whole word.

These are just my thoughts, and if you disagree with me here, its completely fine, the current implementation is usable as well.

@nvaccessAuto

Comment 11 by nvdakor on 2014-10-30 08:22
Hi,
Just to add to my last comment: from the outside, it's the same find dialog that we know and love, but on the inside, it's a completely new beast. Both "dialogs" are accessible by pressing Control+NVDA+F - that's the magic of branching.
Hope this helps. Thanks.

@nvaccessAuto

Comment 12 by jteh (in reply to comment 7) on 2014-10-30 09:29
Thanks for your work.

Replying to nvdakor:

  • For now, the title of the dialog is "case sensitive find" to distinguish it from the current find routine.

IMO, this sort of thing isn't necessary. You're working in a branch, so there's no need to distinguish because the fact that it's a branch distinguishes it, if that makes sense. Also, you want to avoid unnecessary effort when you come to finalise your work.

  • Modified description of Control+NVDA+F command from browse mode to say that one can tell NVDA to perform case sensitive search.

Again, I think this is unnecessary. Case sensitivity is just an option; users don't need to know about that before they even use the command. That's why we have a What's New. :)

@nvaccessAuto

Comment 13 by Joseph Lee <joseph.lee22590@... on 2014-10-30 20:01
In [709c68e03a6b3a2c47bdfecb4aadc3bd4cd07bc1]:
```CommitTicketReference repository="" revision="709c68e03a6b3a2c47bdfecb4aadc3bd4cd07bc1"
CursorManager: added a FindDialog which contains case sensitivity check box in addition to find text field. re #4584
A limitation with current find dialog implementation is that it can only get text to be found. To allow case sensitivity check box to be added , a custom wx.Dialog class was created. If case sensitive checkbox is checked, one can limit search scope to cse sensitive results (TextInfos already has this facility).
At this point, the search dialog does show up, but it throws type error when trying to call CursorManager.doFindText. Also to distinguish the new code from the old one, the title of the dialog is case sensitive find, and once type error is resolved, it'll become find.
Also added documentation in user guide.

@nvaccessAuto

Comment 14 by Joseph Lee <joseph.lee22590@... on 2014-10-30 20:01
In [2e7139de05fcd89ca87021206a1ecdcddb38ce54]:
```CommitTicketReference repository="" revision="2e7139de05fcd89ca87021206a1ecdcddb38ce54"
CursorManager: it is possible to perform case sensitive search. re #4584
Added a new FindDialog object that performs what the old find dialog did, and added a checkbox to tell NVDA to perform case sensitive search.
'Changed form last commit: spelling mistake (it is not CallAfter, but CallLater).

@nvaccessAuto

Comment 15 by nvdakor (in reply to comment 12) on 2014-10-30 20:03
Replying to jteh:

Thanks for your work.

Replying to nvdakor:

  • For now, the title of the dialog is "case sensitive find" to distinguish it from the current find routine.

IMO, this sort of thing isn't necessary. You're working in a branch, so there's no need to distinguish because the fact that it's a branch distinguishes it, if that makes sense. Also, you want to avoid unnecessary effort when you come to finalise your work.

Fixed in the commit prior to the latest commit.

  • Modified description of Control+NVDA+F command from browse mode to say that one can tell NVDA to perform case sensitive search.

Again, I think this is unnecessary. Case sensitivity is just an option; users don't need to know about that before they even use the command. That's why we have a What's New. :)

Got it - also done.
Tanks for comments.

@nvaccessAuto

Comment 16 by jteh on 2014-11-12 01:29
Nice work; thanks! Code review:

Re the docstring for FindDialog: I don't think this should mention virtual buffers, document review, etc., as it applies to all CursorManagers and will only work with CursorManagers. Perhaps it could just say "A dialog used to specify text to find in a cursor manager."

Right now, you have two vertical sizers inside mainSizer: one with the options and the other with the buttons. Instead, I'd put the find label and text control in a horizontal sizer and then add that, the check box and the buttons sizer to mainSizer. Reasons:
1. A separate sizer for the controls is probably redundant here.
2. The label for the text control should probably be beside it rather than above it, hence the horizontal sizer.

In onOk, you call Destroy before doing anything else. That's not safe, since the controls could theoretically be destroyed before you get their values. Call Destroy at the end.

I'd move all of the code from doFindTextDialog into script_find, especially given how short it is now. The second method is redundant. I suspect we didn't do that initially because GUI stuff used to be a bit more complicated than it is now.

Thanks.

@nvaccessAuto

Comment 17 by nvdakor (in reply to comment 16) on 2014-11-12 04:23
Replying to jteh:

Re the docstring for FindDialog: I don't think this should mention virtual buffers, document review, etc., as it applies to all CursorManagers and will only work with CursorManagers. Perhaps it could just say "A dialog used to specify text to find in a cursor manager."

Done.

Right now, you have two vertical sizers inside mainSizer: one with the options and the other with the buttons. Instead, I'd put the find label and text control in a horizontal sizer and then add that, the check box and the buttons sizer to mainSizer. Reasons:

  1. A separate sizer for the controls is probably redundant here.

  2. The label for the text control should probably be beside it rather than above it, hence the horizontal sizer.

Trying to fix this - my code produces overlapping controls, so I'm working on how to prevent this from happening.

In onOk, you call Destroy before doing anything else. That's not safe, since the controls could theoretically be destroyed before you get their values. Call Destroy at the end.

Also done.

I'd move all of the code from doFindTextDialog into script_find, especially given how short it is now. The second method is redundant. I suspect we didn't do that initially because GUI stuff used to be a bit more complicated than it is now.

I will do so - I do agree that it is better to eliminate redundant methods now that the code is much simpler.
Thanks for the review.

@nvaccessAuto

Comment 18 by jteh on 2014-11-12 04:56
Oh, and please centre the dialog. :)

@nvaccessAuto

Comment 19 by Joseph Lee <joseph.lee22590@... on 2014-11-12 05:23
In [0598905c48c34992927f14dbe005479e8b08bcf5]:
```CommitTicketReference repository="" revision="0598905c48c34992927f14dbe005479e8b08bcf5"
Cursor Manager: removed now redundant Find Dialog script, as the dialog itself will be called from actual find script. re #4584.
As suggested by Jamie from Nv access, the find sizer in the find dialog will now contain just the find text box. Also shortened the docstring and put self.Destroy at the end of onOk method, thanks to code review from Jamie.

@nvaccessAuto

Comment 20 by nvdakor (in reply to comment 18) on 2014-11-12 05:33
Replying to jteh:

Oh, and please centre the dialog. :)

Done. Thanks for reminding me.
Besides this change, the latest commits include shortened docstring, rewritten sizers in find dialog and revised find script (removed find dialog method, as suggested). Thanks.

@nvaccessAuto

Comment 21 by blindbhavya on 2014-11-12 16:10
Sorry for interrupting, but it seems that one approach has been finalized, which o the following two approaches has been finalized?
1 adding a separate dialog for case sensitive search
2 adding a case sensitivity toggle checkk box in the existing NVDA Find dialog

@nvaccessAuto

Comment 22 by nvdakor (in reply to comment 21) on 2014-11-12 16:34
Replying to blindbhavya:

Sorry for interrupting, but it seems that one approach has been finalized, which o the following two approaches has been finalized?

1 adding a separate dialog for case sensitive search

2 adding a case sensitivity toggle checkk box in the existing NVDA Find dialog

I worked on the second option. There was no need to add a separate case sensitive find dialog. Adding the functionality required more than just creating the dialog (see the commit comments). Thanks.

@nvaccessAuto

Comment 23 by blindbhavya on 2014-11-12 16:36
Hi,
I too prefer the second approach.
Thanks

@nvaccessAuto

Comment 24 by James Teh <jamie@... on 2014-11-19 04:59
In [322d577]:
```CommitTicketReference repository="" revision="322d577975bc51c5887964e3c03ee58c9f2c1fb1"
Merge branch 't4584' into next

Incubates #4584.

Changes:
Added labels: incubating
@nvaccessAuto

Comment 25 by jteh on 2014-11-19 05:01
Thanks. Note that I've squashed your commits into one commit in the t4584 branch, since the intermediate commits aren't really useful in this case. So, you'll need to git pull -f first if you are making updates to that branch.

@nvaccessAuto

Comment 26 by James Teh <jamie@... on 2014-12-03 04:44
In [74d1c9c]:
```CommitTicketReference repository="" revision="74d1c9cd0333f5c87f37cd0f8f6d7bbf08cdef23"
In the browse mode Find dialog, there is now an option to perform a case sensitive search.

Fixes #4584.

Changes:
Removed labels: incubating
State: closed
@nvaccessAuto

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

@nvaccessAuto

Comment 28 by twynn92 on 2015-01-09 08:38
Should this ticket be reopened? The "Case sensitive" check-box's state is not remembered in subsequent searches.
Example:

  1. On this page, press CTRL+Home to move to the top of the document, then CTRL+NVDA+F.
  2. In the "Type the text you wish to find" edit box, type in "Summary" without the quotes.
  3. Check the "Case sensitive" check-box.
  4. Press Enter or activate the "OK" button.
  5. Now, either a. press CTRL+NDVA+F again, or a. NVDA+F3. If you did the former, note that the "Case sensitive" check-box is unchecked. If the latter, note that NVDA found a lowercase variant of the search query.

Results: On subsequent access to the search function, the "Case sensitive" check-box's state is not remembered and reverts to the unchecked state.

Expected results: In step five, the latter case should still remember the "Case sensitive" check-box's status. As for the former, I am not sure as it may be expected that the user is performing a new search entirely. This state should also be remembered for Shift+NVDA+F3 as well.

Random thought: Maybe a radio-button should be added for search direction? This should be a new ticket I am assuming, or the summary should be modified.

@nvaccessAuto

Comment 29 by nvdakor on 2015-01-09 08:48
Hi,
I once thought about making NVDA remember the state of that checkbox, buuut I decided against it, seeing that it might be best to implement one thing at a time. I think the request for making NvDA remember the state of this checkbox might be a new ticket (quite trivial to implement, I think).
Thanks.

@nvaccessAuto nvaccessAuto added this to the 2015.1 milestone Nov 10, 2015
@josephsl josephsl added a commit that referenced this issue Nov 23, 2015
@josephsl josephsl In the browse mode Find dialog, there is now an option to perform a c…
…ase sensitive search.

Fixes #4584.
74d1c9c
@josephsl josephsl referenced this issue in derekriemer/nvda Jan 8, 2016
@derekriemer derekriemer The case sensitivity of the last find is remembered when searching us…
…ing NVDA's find dialog. For Example, if Food is searched for with case sensitivity on Food will be found when pressing NVDA+f3, but food will not.
33189af
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment