IGBF-1150
First Level Review #2
Functional Review:
On main master, when I choose file > load session, the window has the mac os style. I’m not sure what the swing window looks like. This window looks very similar to the one I get in chrome when I try to open a file. See image MacOsOpenFileDialog.png.
On branch IGBF-1150, I did the same thing, and I get the same window. However there is a pull down option now for file type and it defaults to XML, and only folders and xml files are enabled.
See MacOpenFileDialog_WithPullDown.png
In both cases, selecting the saved session seemed to be effective.
Git History Review:
ok
Code Review:
I’m no file-choosers expert, but this looks reasonable to me, and it has comments, good.
Be careful with commenting on what the code used to be like.
In this comment,
// <Ashwini Kadam> IGBF-1150 : Related to IGBF-1140
+ // JFileChooser displays JavaFX style Open File Session dialog
+ // while loading a session. Instead we want OS Native file choooser.
+ // Thus we are turning to FileChooser for opening a dialog.
Someone could read the first sentence, pause to look at the code and get confused. The first sentence is actually about what the code USED to be, not what it is. Try to make comments focus on what the code does now. Only describe what is was in past as much as you need to in order to make the current code easy to understand.
This is a general note for the future, I don’t think you necessarily need to change this now.
Action:
I was the second reviewer, BUT, this issue really needs to be tested on a PC. The window was already the right type for mac. I’m going to do a second functional review on a windows machine.
I have fixed this issue by converting file chooser from JFileChooser to OS Native File Chooser (through FileChooser Utils).
I have also tested it by verifying that only XML files are allowed for user to choose from and when valid XML session file is selected, it gets loaded correctly.
On invalid XML file selection, error message is shown to user saying 'Selected file is invalid. Only file saved by using save session will be opened'. This behavior is consistent with earlier design, only load session window style is change which was expected.
NOTE:
I have not tested this on linux or Mac OS.