-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Better feedback for regexp errors in find/replace #5614
Conversation
@timhoffm thanks a lot for this. could you paste a couple of screenshots to better visualize the changes made by this PR? |
@rlaverde, please review this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, maybe some refactoring is needed.
spyder/widgets/findreplace.py
Outdated
stylesheet = self.STYLE[found] | ||
tooltip = self.TOOLTIP[found] | ||
self.search_text.lineEdit().setStyleSheet(stylesheet) | ||
self.search_text.setToolTip(tooltip) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code could be refactored, It has repeated calls to rexexp_error_msg
, also both "if" are checking for the error message:
stylesheet = self.STYLE[found]
tooltip = self.TOOLTIP[found]
if not found and regexp:
error_msg = rexexp_error_msg(text)
if error_msg:
stylesheet = self.STYLE['regexp_error']
tooltip = "{}: {}".format(self.TOOLTIP['regexp_error'], error_msg)
self.search_text.lineEdit().setStyleSheet(stylesheet)
self.search_text.setToolTip(tooltip)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks that's a more readable version (Note that calling rexexp_error_msg()
twice would not have been a big deal in itself since re.compile()
is cached).
I've applied your suggestion with the exception of not switching the string formatting. IMHO "{}: {}".format(
is not making it more readable in this case (And also I don't want pep8 to moan again about the line length).
spyder/widgets/findreplace.py
Outdated
@@ -50,7 +50,14 @@ class FindReplace(QWidget): | |||
"""Find widget""" | |||
STYLE = {False: "background-color:rgb(255, 175, 90);", | |||
True: "", | |||
None: ""} | |||
None: "", | |||
'regexp_error': "background-color:rgb(255, 20, 20);", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add some alpha here to tone down a bit this red? I mean, as it is, it's a bit too strong for the eye (in my opinion).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to choose any shade of red you want. I didn't put any effort in chosing the color. IMO it should be a little stronger than the color for "not found".
There's no need for alpha though because it's the background anyway. Just adapt the rgb values.
@timhoffm, I left a minor comment. The rest looks good to me. |
Based on [#5386] in which the user did not realize that he was searching with an invalid regular expression.
This patch gives feedback on invalid regular expressions by coloring the search field and adding a tooltip.