Search for JPanel in optionPane that contains the buttons#220
Search for JPanel in optionPane that contains the buttons#220andrasfuchs merged 2 commits intofreerouting:masterfrom
Conversation
|
@andrasfuchs Can I trouble you for a review of this please? It affects command line usage in Ubuntu (maybe others) in It appears at some point Java changed the layout of the option pane dialogue. Rather than assume we know where the buttons are, this performs a quick search for them. This approach should also be backwards compatible encase somebody uses outdated libraries. Not sure how much you want to engineer safety into this, we could:
Regarding the exception handling message, it would be great if it also gave a file name and line number for where the exception was triggered. |
|
Hey @daniel-theia, thank you for the PR, it sounds great, and I appreciate your help! I'm on holiday at the moment, and I need to focus on other things too as soon as I'm back, but I plan to block a day or two for freerouting in the near future. When this happens your PR will be one of the first things to check. Thanks again, I'll keep you updated here. |
|
@daniel-theia Hi, I haven't come across the issue but looks like the commit e1fd77a in my PR #224 might have fixed the problem. Instead of searching for the button I supplied the dialog with my own. Can you check if that works? |
@maksz42 Haven't currently got time to check, but looking through the code it does appear to address the casting issue from the wrong index. |
|
@daniel-theia I didn't find the part where you check if the buttons are flipped. Their text might mislead that check because freerouting applies the local language by default. I also added a check for the case where we don't find an optionPanel to prevent an exception. Otherwise it looks good, thank you, I'll merge it now. |
We find the buttons: https://github.com/daniel-theia/freerouting/blob/748b390772e197534f94d453233d2cc077b779f3/src/main/java/app/freerouting/gui/MainApplication.java#L371 And then we set the text to the locale, meaning we control their text meaning. As long as the locales are all stored together (which they should be), there is no possibility for them being flipped. Let me check this merge, I see a difference between the start and cancel button setup and want to make sure that was pushed correctly. |
daniel-theia
left a comment
There was a problem hiding this comment.
Hopefully some useful comments on the changes.
| JButton cancelButton; | ||
| if ((optionPanel != null) && (cancelButtonIndex >= 0)) { |
There was a problem hiding this comment.
cancelButton is declared in name only outside of the if-statement, whereas startButton is fully declared. I know that startButton is not used in a timer, but for readability it makes sense to have both of these buttons similar so that in the future they are coded in the same way.
| if(c instanceof JPanel && ((JPanel)c).getComponents()[0] instanceof JButton){ | ||
| if(c instanceof JPanel && ((JPanel)c).getComponents()[0] instanceof JButton && ((JPanel)c).getComponents()[1] instanceof JButton){ |
There was a problem hiding this comment.
Should make sure that there is more than one component packed into the JPanel first, something like:
if(
c instanceof JPanel &&
((JPanel)c).getComponents().length >= 2 && // Ensure that there is at least two components
((JPanel)c).getComponents()[0] instanceof JButton &&
((JPanel)c).getComponents()[1] instanceof JButton
){|
I've made these modifications. Thank you! |
|
@andrasfuchs No problem, thank you. |
Related issue: #218