Store window sizes in prefs at JabRef frame quit #10143
Conversation
Follow up fix for #4393
Siedlerchr
added
the
status: ready-for-review
|
Your code currently does not meet JabRef's code guidelines. The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues. More information on code quality in JabRef is available at https://devdocs.jabref.org/getting-into-the-code/development-strategy.html. |
| private void saveWindowState(Stage mainStage) { | ||
| GuiPreferences preferences = prefs.getGuiPreferences(); | ||
| preferences.setPositionX(mainStage.getX()); | ||
| preferences.setPositionY(mainStage.getY()); | ||
| preferences.setSizeX(mainStage.getWidth()); | ||
| preferences.setSizeY(mainStage.getHeight()); | ||
| preferences.setWindowMaximised(mainStage.isMaximized()); | ||
| preferences.setWindowFullScreen(mainStage.isFullScreen()); | ||
| debugLogWindowState(mainStage); | ||
| } |
There was a problem hiding this comment.
Hm, this is not part of the frame, but of the stage. I was hoping I could untangle all that GUI-CLI-opendatabase stuff on jabcon. Right now this is just a code duplication of JabRefGUI#saveWindowState. But maybe you can make this method in JabRefFrame public and remove the one in JabRefGui?
There was a problem hiding this comment.
Good idea. The key problem is that cmd + Q (like alt + f4 ) does not trigger the onCloseRequest listener of the stage
There was a problem hiding this comment.
Yes, i did a little bit of reading on this one. I believe the main problem is that the whole startup and shutdown process has to be rewritten.
JabRefGui and MainApplication should probably be merged, the creation of ExecutorService be moved to JabRefGui so it can be cleanly be shutdown, so we can solve the problem with the runaway process when all windows are closed (see also Platform.setImplicitExit(true); ), the different openDatabase methods be unified ...
There was a problem hiding this comment.
We had this before, but then the decision was made to avoid init of javafx directly and thus we have this Gui and CLI etc.
There was a problem hiding this comment.
CMD+Q does and would trigger onCloseRequest, except you're binding that key combination (via "ctrl+Q" in KeyBinding) yourself and calling quit() in CloseAction's execute(); that's why the stage isn't being closed and calling the listener; it isn't getting the chance.
Alt-F4 also already calls onCloseRequest with no issue, at least on Linux; and that is the operating system.
Also, shouldn't "private void saveWindowState(Stage mainStage)" be removed from JabRefGUI? It's not used and its existence only adds to an already-confusing situation ;) ...
Finally, Mac-OS specific comments like this:
// General info dialog. The MacAdapter calls this method when "Quit" is selected from the application menu, Cmd-Q
// is pressed, or "Quit" is selected from the Dock. The function returns a boolean indicating if quitting is ok or
// not.
...seem all wrong to me. There is no MacAdapter (it's mentioned three times in the codebase), and there is no specific/special behaviour that I've observed that's Mac-specific here. It all makes sense (based on what I said above) and seems consistent between operating systems. Either something has changed fundamentally to make this commentary untrue/inaccurate, or else the original author has possibly misinterpreted things.
There was a problem hiding this comment.
We had this before, but then the decision was made to avoid init of javafx directly and thus we have this Gui and CLI etc.
MainApplication is part of the GUI component. The Launcher class is the super class deciding on opening GUI or cli.
About MacOS stuff: parts of the codebase and parts of the architecture of jabref are 20 years old. It was necessary at one point, as it has fixed a bug once. Would probably have to be tested if this is still necessary.
|
The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build. |
Follow up fix for #4939
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)