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

Improve regex checking #1258

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

windymilla
Copy link
Collaborator

User can type regexes in 4 places: Search/Replace, Quick Search, Word Frequency, Search->Highlight Character, String or Regex.

Only the first had regex checking, changing the text color to red if the regex gave an error on compilation, and a weak error message if the user tried to actually search with the bad regex.

This commit improves the error message to report a sanitized version of the error or warning message for Search/Replace. It also applies the same color changing, error messages, and checks for switching between exact/regex as appropriate for the other 3 cases.

Validating doesn't always work well with having a variable bound to the text widget, so that has been removed if present, and the necessary functionality coded separately.

User can type regexes in 4 places: Search/Replace, Quick Search,
Word Frequency, Search->Highlight Character, String or Regex.

Only the first had regex checking, changing the text color to red
if the regex gave an error on compilation, and a weak error
message if the user tried to actually search with the bad regex.

This commit improves the error message to report a sanitized
version of the error or warning message for Search/Replace.
It also applies the same color changing, error messages, and
checks for switching between exact/regex as appropriate for
the other 3 cases.

Validating doesn't always work well with having a variable
bound to the text widget, so that has been removed if present,
and the necessary functionality coded separately.
@windymilla windymilla requested review from cpeel and srjfoo October 1, 2023 19:37
@windymilla windymilla linked an issue Oct 1, 2023 that may be closed by this pull request
@windymilla
Copy link
Collaborator Author

Testing notes: In addition to typing good/bad regexes in WF, S/R, Quicksearch, Select->Highlight Char, String, Regex (with regex flag on/off where possible), please also check that if you try to actually use the good/bad regex it either works or gives a sensible error popup. Finally, please also check that you don't get further errors if you dismiss the error message dialog after/before dismissing the dialog that generated it, e.g. cross off WF dialog before the Bad Regex error popup, and vice versa.

@srjfoo
Copy link
Member

srjfoo commented Oct 2, 2023

Testing notes: In addition to typing good/bad regexes in WF, S/R, Quicksearch, Select->Highlight Char, String, Regex (with regex flag on/off where possible), please also check that if you try to actually use the good/bad regex it either works or gives a sensible error popup. Finally, please also check that you don't get further errors if you dismiss the error message dialog after/before dismissing the dialog that generated it, e.g. cross off WF dialog before the Bad Regex error popup, and vice versa.

I think I managed to get all that tested, and I didn't see errors when closing the popups. Does it matter how you close the popup? I.e. whether you use the gumdrop in the upper left, or the provided button?

What I did find in my random poking at some buttons is that I can't seem to get the count button in the S/R dialog to work. (It works fine in 1.6.0, which is the only reason I know where to expect the count to appear). Can you double-check me on that and let me know if it's just something that I'm doing wrong?

(note to self -- we really need a warning to Mac users that trying to use GG with dual monitors is rather fraught.)

Copy/paste error in previous commit meant count label was only
shown if *Quick*Search dialog was popped, not *Search* dialog.
@windymilla
Copy link
Collaborator Author

I think I managed to get all that tested, and I didn't see errors when closing the popups. Does it matter how you close the popup? I.e. whether you use the gumdrop in the upper left, or the provided button?

What happens in 1.6.0, I think, is the following:

  1. Type bad regex into S/R dialog
  2. Click Count
  3. Dismiss the S/R dialog
  4. Dismiss the error dialog that says you had a bad regex
  5. The general error message box (and command window) show an error about non Tk object - the user shouldn't see that.

It's because the "update count" code occurs after the use of the regex, so if the regex pops its error dialog, then you destroy the S/R dialog (with the Count label) it fails to write the count to the Count label. There were a few things like that that you could make happen by closing the original dialog before dismissing the warning about the regex. Hopefully I've trapped them all. :)

@srjfoo
Copy link
Member

srjfoo commented Oct 13, 2023

I think I managed to get all that tested, and I didn't see errors when closing the popups. Does it matter how you close the popup? I.e. whether you use the gumdrop in the upper left, or the provided button?

What happens in 1.6.0, I think, is the following:

  1. Type bad regex into S/R dialog

  2. Click Count

  3. Dismiss the S/R dialog

  4. Dismiss the error dialog that says you had a bad regex

  5. The general error message box (and command window) show an error about non Tk object - the user shouldn't see that.

It's because the "update count" code occurs after the use of the regex, so if the regex pops its error dialog, then you destroy the S/R dialog (with the Count label) it fails to write the count to the Count label. There were a few things like that that you could make happen by closing the original dialog before dismissing the warning about the regex. Hopefully I've trapped them all. :)

In 1.6.0, error as described; in this branch, no error in message log, but a useful message, and closing S/R before popup did not cause a message log.

With QS, no message log popped up in 1.6.0, even though I closed the qs dialog before I closed the error message. Bad regex not colored red.

WF, in 1.6.0, does identify it in the WF results area as an invalid regular expression :D -- in the regex-check branch, the output matches the other, and the bad regex is colored red. No error message in message log that I saw, but WF isn't an error popup, and there wasn't anything to close.

Search->Highlight Character, String or Regex did open the message log with an error upon using the bad regex.

Confirmed that in all cases in the regex-check branch, the bad regex is red 👍 , and the same error popup is shown for all.

@windymilla windymilla merged commit 9d53496 into DistributedProofreaders:master Oct 13, 2023
1 check passed
@windymilla windymilla deleted the regex-check branch October 13, 2023 20:21
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.

Highlight character, string or regex does not trap bad regex
3 participants