Fix BLE in SearchEngine and add test #428
Conversation
| @@ -72,7 +72,7 @@ public static SearchResult find(JTextArea textArea, SearchContext context) { | |||
| boolean doMarkAll = textArea instanceof RTextArea && context.getMarkAll(); | |||
|
|
|||
| String text = context.getSearchFor(); | |||
| if (text==null || text.length()==0) { | |||
| if (text==null || text.length()==0 || textArea.getText().isEmpty()) { | |||
There was a problem hiding this comment.
This empty-check is expensive for large documents, can you update this to be:
| if (text==null || text.length()==0 || textArea.getText().isEmpty()) { | |
| if (text == null || text.isEmpty() || textArea.getDocument().getLength() == 0) { |
| @@ -262,7 +262,7 @@ private static String getFindInText(JTextArea textArea, int start, | |||
| } | |||
| } catch (BadLocationException ble) { | |||
| // Never happens; findIn will be null anyway. | |||
| ble.printStackTrace(); | |||
| throw new RuntimeException(ble); | |||
There was a problem hiding this comment.
I'm worried this will screw up the EDT, if an uncaught exception is thrown. At least that's my recollection. That's why I'm currently just doing a printStackTrace() (which I agree with you is pretty hokey). Is that a concern?
There was a problem hiding this comment.
Hm, I think you're right about the EDT... but if it's not re-thrown, it can't actually be caught in a test. I guess that's probably the lesser of two evils here though.
There was a problem hiding this comment.
Yeah, I'm not necessaarily happy with it but like the comment says - it's not supposed to happen :). Fortunately you caught a bug where it does!
RSTA follows this pattern of calling Throwable.printStackTrace() everywhere that e.g. BadLocationException is thrown by Swing API's but shouldn't happen "in real life" due to using offsets fetched from the Document, etc. I was reluctant to rethrow checked exceptions like BadLocationException everywhere, since that adds complexity for client apps, and also didn't want to add a logging dependency to keep RSTA lightweight. I'm happy folks are looking at other ways to approach this kind of thing though.
In any case, thanks for the PR!
|
I'm not sure what happened with the failing build - seems unrelated to my changes? |
|
@paul-griffith Yes, the Java 17 build currently fails; we have one test that uses Mockito and it looks like something changed starting in Java 15, where the mock's behavior is different and no longer works properly. It's not related to your change, or RSTA at all. I've opened this Mockito ticket to track it. If it can't be addressed easily I'll try to look at a different way to verify stuff in that test to avoid the problem. |
Fixes #427
Catching and rethrowing the BLE is pretty hokey... but to be fair, catching it and dumping the stack trace wasn't much better 馃槈