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

Case sensitivity of searches in virtual buffers are not preserved with find next #5522

Closed
dkager opened this issue Nov 20, 2015 · 18 comments
Closed
Milestone

Comments

@dkager
Copy link
Collaborator

dkager commented Nov 20, 2015

STR:

  1. Find a webpage that contains some text both with and without capitals, e.g. The and the.
  2. Press NVDA+Ctrl+F, type the text with capitals.
  3. NVDA finds the text. Now rpess NVDA+F3.
  4. Eventually NVDA will find an occurrence without capitals.
@dkager
Copy link
Collaborator Author

dkager commented Nov 20, 2015

And a patch in dkager/nvda@d64fb8b. Feel free to not use it and do something far more sophisticated!

@josephsl
Copy link
Collaborator

Hi,

If I remember right, we had a discussion about this while I was implementing case sensitive find (#4584) last year. I’d say let’s leave it as is unless there is a strong reason to preserve case sensitivity (of course we could have a flag indicating that it is a case sensitive find in cursor manager).

Thanks.

From: Davy Kager [mailto:notifications@github.com]
Sent: Friday, November 20, 2015 7:29 AM
To: nvaccess/nvda nvda@noreply.github.com
Subject: [nvda] Case sensitivity of searches in virtual buffers are not preserved with find next (#5522)

STR:

  1. Find a webpage that contains some text both with and without capitals, e.g. The and the.
  2. Press NVDA+Ctrl+F, type the text with capitals.
  3. NVDA finds the text. Now rpess NVDA+F3.
  4. Eventually NVDA will find an occurrence without capitals.


Reply to this email directly or view it on GitHub #5522 .

@dkager
Copy link
Collaborator Author

dkager commented Nov 20, 2015

My ‘strong reason’ is that if I perform a case sensitive search, then ask for the next result, I expect that next search to also be case sensitive. This is especially true in the log viewer (#4929, which I also worked on today). But also when searching through developer documentation in browse mode, if you are looking for SOMETHING you probably don’t want to find SomeThing with the next search (note the caps). Similarly, it struck me as odd that the last search text is preserved but the state of the case sensitive checkbox isn’t.

@dkager
Copy link
Collaborator Author

dkager commented Jan 6, 2016

Having given this some more thought I still think this suggestion makes sense: consecutive searches for a phrase should use the same parameters that were used in the initial search for that phrase. Whether a new search should also remember the parameters is a topic of discussion. I think this is useful but I can imagine others may find it confusing. And of course these parameters should not be persistent, i.e. they revert to their defaults when you restart NVDA.

Any renewed thoughts on this Joseph? :) I'm not trolling (well, not intentionally). I just ran into real-world cases where I'd like my search parameters to be preserved as described above. For example, when searching in JavaDoc files.

@jcsteh
Copy link
Contributor

jcsteh commented Jan 6, 2016

I don't recall the previous discussion on this, but IMO, if someone is willing to code it, we should preserve this setting (but not across restarts of course). Aside from anything else, this is consistent with other programs that have find functionality. I can't think of any find dialog which doesn't keep the settings across executions.

@derekriemer
Copy link
Collaborator

In reguards of implantation:
I have refactored def onOk(self, evt):
text = self.findTextField.GetValue()
caseSensitive = self.caseSensitiveCheckBox.GetValue()

     wx.CallLater(100, self.activeCursorManager.doFindText, text, 

caseSensitive=caseSensitive)

to
def onOk(self, evt):
text = self.findTextField.GetValue()
caseSensitive = self.caseSensitiveCheckBox.GetValue()
self.caseSensitive = caseSensitive
wx.CallLater(100, self.activeCursorManager.doFindText, text)

and might be able to remove the whole arguement for caseSensitive from
doFindText and then have doFindText lookup the option from
self.caseSensitive. Is this a rework you guys are okay with?

On 1/6/2016 4:04 PM, James Teh wrote:

I don't recall the previous discussion on this, but IMO, if someone is
willing to code it, we should preserve this setting (but not across
restarts of course). Aside from anything else, this is consistent with
other programs that have find functionality. I can't think of any find
dialog which doesn't keep the settings across executions.


Reply to this email directly or view it on GitHub
#5522 (comment).


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.drupalgardens.com
Non-proffessional website. http://derekriemer.pythonanywhere.com/personal
Awesome little hand built weather app that rocks!
http://derekriemer.pythonanywhere.com/weather

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@jcsteh
Copy link
Contributor

jcsteh commented Jan 7, 2016

If you do self.caseSensitive, you're storing it on the instance of the dialog. That instance gets thrown away as soon as it's done, so it won't get preserved across finds. Instead, you need to store it as a class attribute (not an instance attribute) on CursorManager. That also allows you to get rid of the caseSensitive argument to doFindText.

@derekriemer
Copy link
Collaborator

Done. I will commit tomorrow. Btw, unless anyone objects (doubt it) I am
gonna update the copyright date of source/cursorManager.py to 2016 with
the pr.

On 1/6/2016 10:43 PM, James Teh wrote:

If you do self.caseSensitive, you're storing it on the instance of the
dialog. That instance gets thrown away as soon as it's done, so it
won't get preserved across finds. Instead, you need to store it as a
class attribute (not an instance attribute) on CursorManager. That
also allows you to get rid of the caseSensitive argument to doFindText.


Reply to this email directly or view it on GitHub
#5522 (comment).


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.drupalgardens.com
Non-proffessional website. http://derekriemer.pythonanywhere.com/personal
Awesome little hand built weather app that rocks!
http://derekriemer.pythonanywhere.com/weather

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@derekriemer
Copy link
Collaborator

See pull request https://github.com/nvaccess/nvda/pull/5662/files

On 1/6/2016 10:43 PM, James Teh wrote:

If you do self.caseSensitive, you're storing it on the instance of the
dialog. That instance gets thrown away as soon as it's done, so it
won't get preserved across finds. Instead, you need to store it as a
class attribute (not an instance attribute) on CursorManager. That
also allows you to get rid of the caseSensitive argument to doFindText.


Reply to this email directly or view it on GitHub
#5522 (comment).


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.drupalgardens.com
Non-proffessional website. http://derekriemer.pythonanywhere.com/personal
Awesome little hand built weather app that rocks!
http://derekriemer.pythonanywhere.com/weather

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@dkager
Copy link
Collaborator Author

dkager commented Jan 7, 2016

Also, the checkbox should get the right selection state based on this attribute of course. See my commit linked to in a comment above, which was rather dirty but shows what I had in mind.

From: James Teh [mailto:notifications@github.com]
Sent: Thursday, January 07, 2016 06:43
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] Case sensitivity of searches in virtual buffers are not preserved with find next (#5522)

If you do self.caseSensitive, you're storing it on the instance of the dialog. That instance gets thrown away as soon as it's done, so it won't get preserved across finds. Instead, you need to store it as a class attribute (not an instance attribute) on CursorManager. That also allows you to get rid of the caseSensitive argument to doFindText.


Reply to this email directly or view it on GitHub #5522 (comment) . https://github.com/notifications/beacon/AC9y8Wh17WPfHudkjH3n3qfFfwHrFLl1ks5pXfKBgaJpZM4Gmcxe.gif

@derekriemer
Copy link
Collaborator

Oh. We do indeed preserve the text's state as part of the find dialog. I
probably should preserve the case sensitivity as well.
On 1/7/2016 8:16 AM, Davy Kager wrote:

Also, the checkbox should get the right selection state based on this
attribute of course. See my commit linked to in a comment above, which
was rather dirty but shows what I had in mind.

From: James Teh [mailto:notifications@github.com]
Sent: Thursday, January 07, 2016 06:43
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] Case sensitivity of searches in virtual buffers
are not preserved with find next (#5522)

If you do self.caseSensitive, you're storing it on the instance of the
dialog. That instance gets thrown away as soon as it's done, so it
won't get preserved across finds. Instead, you need to store it as a
class attribute (not an instance attribute) on CursorManager. That
also allows you to get rid of the caseSensitive argument to doFindText.


Reply to this email directly or view it on GitHub
#5522 (comment)
.
https://github.com/notifications/beacon/AC9y8Wh17WPfHudkjH3n3qfFfwHrFLl1ks5pXfKBgaJpZM4Gmcxe.gif


Reply to this email directly or view it on GitHub
#5522 (comment).


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.drupalgardens.com
Non-proffessional website. http://derekriemer.pythonanywhere.com/personal
Awesome little hand built weather app that rocks!
http://derekriemer.pythonanywhere.com/weather

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@derekriemer
Copy link
Collaborator

Just updated the pr.

On 1/7/2016 8:16 AM, Davy Kager wrote:

Also, the checkbox should get the right selection state based on this
attribute of course. See my commit linked to in a comment above, which
was rather dirty but shows what I had in mind.

From: James Teh [mailto:notifications@github.com]
Sent: Thursday, January 07, 2016 06:43
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] Case sensitivity of searches in virtual buffers
are not preserved with find next (#5522)

If you do self.caseSensitive, you're storing it on the instance of the
dialog. That instance gets thrown away as soon as it's done, so it
won't get preserved across finds. Instead, you need to store it as a
class attribute (not an instance attribute) on CursorManager. That
also allows you to get rid of the caseSensitive argument to doFindText.


Reply to this email directly or view it on GitHub
#5522 (comment)
.
https://github.com/notifications/beacon/AC9y8Wh17WPfHudkjH3n3qfFfwHrFLl1ks5pXfKBgaJpZM4Gmcxe.gif


Reply to this email directly or view it on GitHub
#5522 (comment).


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.drupalgardens.com
Non-proffessional website. http://derekriemer.pythonanywhere.com/personal
Awesome little hand built weather app that rocks!
http://derekriemer.pythonanywhere.com/weather

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@dkager
Copy link
Collaborator Author

dkager commented Jan 7, 2016

Awesome!

From: derekriemer [mailto:notifications@github.com]
Sent: Thursday, January 07, 2016 16:44
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] Case sensitivity of searches in virtual buffers are not preserved with find next (#5522)

Just updated the pr.

On 1/7/2016 8:16 AM, Davy Kager wrote:

Also, the checkbox should get the right selection state based on this
attribute of course. See my commit linked to in a comment above, which
was rather dirty but shows what I had in mind.

From: James Teh [mailto:notifications@github.com]
Sent: Thursday, January 07, 2016 06:43
To: nvaccess/nvda <nvda@noreply.github.com mailto:nvda@noreply.github.com >
Cc: Davy Kager <mail@davykager.nl mailto:mail@davykager.nl >
Subject: Re: [nvda] Case sensitivity of searches in virtual buffers
are not preserved with find next (#5522)

If you do self.caseSensitive, you're storing it on the instance of the
dialog. That instance gets thrown away as soon as it's done, so it
won't get preserved across finds. Instead, you need to store it as a
class attribute (not an instance attribute) on CursorManager. That
also allows you to get rid of the caseSensitive argument to doFindText.


Reply to this email directly or view it on GitHub
#5522 (comment)
.
https://github.com/notifications/beacon/AC9y8Wh17WPfHudkjH3n3qfFfwHrFLl1ks5pXfKBgaJpZM4Gmcxe.gif


Reply to this email directly or view it on GitHub
#5522 (comment).


Derek Riemer

  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.drupalgardens.com
Non-proffessional website. http://derekriemer.pythonanywhere.com/personal
Awesome little hand built weather app that rocks!
http://derekriemer.pythonanywhere.com/weather

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194


Reply to this email directly or view it on GitHub #5522 (comment) . https://github.com/notifications/beacon/AC9y8WZ5vcP5l-hgdDIe10nLcDToLVVwks5pXn9NgaJpZM4Gmcxe.gif

@derekriemer
Copy link
Collaborator

Note that I have a pull request open for this, and in terms of code review, this should be an easy one because there's not that much code.

@josephsl
Copy link
Collaborator

josephsl commented May 6, 2016

Hi,

On my way to do code review.

From: derekriemer [mailto:notifications@github.com]
Sent: Friday, May 6, 2016 4:41 PM
To: nvaccess/nvda
Cc: josephsl; Comment
Subject: Re: [nvaccess/nvda] Case sensitivity of searches in virtual buffers are not preserved with find next (#5522)

Note that I have a pull request open for this, and in terms of code review, this should be an easy one because there's not that much code.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub #5522 (comment) Image removed by sender.

@derekriemer
Copy link
Collaborator

I think you reviewed the PR for this a while ago and it stalled. We had
to update a copyright date and after that it was not re reviewed.

On 5/6/2016 5:42 PM, josephsl wrote:

Hi,

On my way to do code review.

From: derekriemer [mailto:notifications@github.com]
Sent: Friday, May 6, 2016 4:41 PM
To: nvaccess/nvda
Cc: josephsl; Comment
Subject: Re: [nvaccess/nvda] Case sensitivity of searches in virtual
buffers are not preserved with find next (#5522)

Note that I have a pull request open for this, and in terms of code
review, this should be an easy one because there's not that much code.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#5522 (comment)
Image removed by sender.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#5522 (comment)


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.com
Awesome little hand built weather app!
http://django.derekriemer.com/weather/

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@josephsl
Copy link
Collaborator

josephsl commented May 6, 2016

Hi,

Uh oh, we need to nudge devs a bit then… If there’s virtually no change between the code I reviewed and the new commit, then I’d say we should wait for a little while until @jcsteh can take a look at this further. Thanks.

From: derekriemer [mailto:notifications@github.com]
Sent: Friday, May 6, 2016 4:47 PM
To: nvaccess/nvda
Cc: josephsl; Comment
Subject: Re: [nvaccess/nvda] Case sensitivity of searches in virtual buffers are not preserved with find next (#5522)

I think you reviewed the PR for this a while ago and it stalled. We had
to update a copyright date and after that it was not re reviewed.

On 5/6/2016 5:42 PM, josephsl wrote:

Hi,

On my way to do code review.

From: derekriemer [mailto:notifications@github.com]
Sent: Friday, May 6, 2016 4:41 PM
To: nvaccess/nvda
Cc: josephsl; Comment
Subject: Re: [nvaccess/nvda] Case sensitivity of searches in virtual
buffers are not preserved with find next (#5522)

Note that I have a pull request open for this, and in terms of code
review, this should be an easy one because there's not that much code.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#5522 (comment)
Image removed by sender.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#5522 (comment)


Derek Riemer

  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.com
Awesome little hand built weather app!
http://django.derekriemer.com/weather/

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194


You are receiving this because you commented.
Reply to this email directly or view it on GitHub #5522 (comment) Image removed by sender.

@tmthywynn8
Copy link

jcsteh commented on Jan 7:

this is consistent with other programs that have find functionality.

  1. Speaking of consistency, should we change the check-box's label to "Match case", like in almost all Find dialogs?
  2. Also, should the accelerator key be Alt+C rather than Alt+S (e.g. Notepad, Wordpad, and Firefox)?
  3. Oh, and for future reference, should I be opening a new ticket even though the issue somewhat relates, i.e., expected or standard behavior?

@jcsteh jcsteh added this to the 2016.3 milestone Aug 5, 2016
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

No branches or pull requests

5 participants