Fix for application dialogs opening in wrong displays by eludif · Pull Request #7273 · JabRef/jabref

eludif · 2020-12-29T19:55:21Z

What:

Fix for issue with application dialogs opening in the wrong display

Why:

Dialog opens in the display the application opened at startup (usually the primary display), regardless of the current position of the application main window
This is a draft to discuss the approach taken.

Fixes #7272

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

tobiasdiez

Thanks for your contribution!
Setting the owner of the sub-dialogs is a good idea, indeed.

I was thinking about the best way to do this, without too much changes. Here is what I came up with:

The DialogService should be able to get the main stage from

jabref/src/main/java/org/jabref/gui/JabRefDialogService.java

Line 75 in 020cc97

public JabRefDialogService(Window mainWindow, Pane mainPane, PreferencesService preferences) {
Thus, for all dialogs created with the help of the DialogService we can easily set the owner.
The dialogs that are currently shown by invoking dialog.show() or dialog.showAndWait() need to be shown via https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/gui/JabRefDialogService.java#L267

What do you think?

eludif · 2020-12-29T23:37:37Z

Thanks for your contribution!
Setting the owner of the sub-dialogs is a good idea, indeed.

I was thinking about the best way to do this, without too much changes. Here is what I came up with:

The DialogService should be able to get the main stage from

jabref/src/main/java/org/jabref/gui/JabRefDialogService.java

Line 75 in 020cc97

public JabRefDialogService(Window mainWindow, Pane mainPane, PreferencesService preferences) {

Thus, for all dialogs created with the help of the DialogService we can easily set the owner.

The dialogs that are currently shown by invoking dialog.show() or dialog.showAndWait() need to be shown via https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/gui/JabRefDialogService.java#L267

What do you think?

Hi @tobiasdiez, thanks for the comments.

I agree based on your suggestion; for dialogs created with the help of DialogService, owners of such dialogs can easily be set.
It seems like we might have to consider adding a Window object to the BaseDialog constructor here:

jabref/src/main/java/org/jabref/gui/util/BaseDialog.java

Line 22 in 020cc97

protected BaseDialog() {

That way initOwner(mainWindow); can be called within the constructor, for dialogs not created with the help of DialogService, but rather extending BaseDialog.
If not, we need a way to find out what what Window creates dialogs that extend BaseDialog, eg. AboutDialogView in order to set its owner.

tobiasdiez · 2020-12-30T11:54:24Z

It seems like we might have to consider adding a Window object to the BaseDialog constructor here:

That would work indeed. However, then one needs to pass references of the main stage everywhere (like what you did in the current changes), and that's something I don't really like. Instead, I was proposing to pass references to the DialogService and then using something like

DialogService dialogService = ... coming from somewhere
AboutDialogView aboutDialogView = new AboutDialogView();
dialogService.show(aboutDialogView);

to show the dialog with the proper owner (set in the dialogservice class). In this way, only the dialogservice needs to know about the main stage. Moreover, there are already a lot of places that have the dialog service floating around, so hopefully less changes are required.

eludif · 2020-12-30T13:51:24Z

DialogService dialogService = ... coming from somewhere

Sounds good, can you explain how dependency inject is handled in this project, I'm unable to correctly inject DialogSerivce into AboutDialogView, thanks.

tobiasdiez · 2020-12-30T15:35:26Z

You can use @Inject DialogService dialogService in View classes, e.g here:

jabref/src/main/java/org/jabref/gui/help/AboutDialogView.java

Line 23 in c8f7be8

@Inject private DialogService dialogService;

This is then usually passed to the ViewModel, which uses it to open a dialog.

jabref/src/main/java/org/jabref/gui/help/AboutDialogView.java

Line 45 in c8f7be8

    
           viewModel = new AboutDialogViewModel(dialogService, clipBoardManager, buildInfo);

But for our purposes here that's not very helpful as there are almost no dialogs created from other dialogs. For actions, the usual pattern is to pass it as a constructor argument, e.g. https://github.com/JabRef/jabref/blob/4cf2fb06d3d876d2043f65ca0e51c3c4185fd05c/src/main/java/org/jabref/gui/undo/UndoRedoAction.java

eludif · 2020-12-30T16:50:48Z

What are your thoughts regarding instantiating via the Injector.instantiateModelOrService() method.

tobiasdiez

Using the Injectior is actually a very good idea!

tobiasdiez · 2020-12-31T13:13:51Z

     */
    Optional<Path> showFileOpenFromArchiveDialog(Path archivePath) throws IOException;
+
+    void show(BaseDialog<?> aboutDialogView);


I would rename it to showCustomDialog to be coherent with the above method. Or rename that to showAndWait...yeah maybe the later?

Please also rename the argument, and add a bit of documentation.

I would rename it to showCustomDialog to be coherent with the above method. Or rename that to showAndWait...yeah maybe the later?

Please also rename the argument, and add a bit of documentation.

Decided to go with showCustomDialog() as it seems to be more consistent with other methods in that interface.

- Rename DialogService.show() method to showCustomDialog() - Rename showCustomDialog()'s argument - Add documentation to showCustomDialog method - Make createDialog and createDialogWithOptOut non static

tobiasdiez

Thanks a lot for the quick follow-ups! The code looks good to me!

eludif · 2020-12-31T18:35:25Z

The added test seems to be failing, I am unable to run the tests locally. Do you have an idea what I might be missing in the test?

tobiasdiez

Testing javafx code is a bit more involved as you have to start the gui interface, see e.g. https://github.com/JabRef/jabref/blob/c8f7be89ad704065474decb61851e42416caf88d/src/test/java/org/jabref/gui/entryeditor/SourceTabTest.java for an example.
To be honest, here it would be fine not to add a test (although it's always good to have tests of course).

tobiasdiez

Thanks a lot for your PR! LGTM

eludif · 2021-01-02T12:08:42Z

Thanks a lot for your PR! LGTM

Thanks for the help and comments. 👍🏾

Siedlerchr

LGTM!
It would be nice if you add some summary and explanation to our devdocs what to do when creating a new dialog, e.g using dialogService to start
https://devdocs.jabref.org/getting-into-the-code/code-howtos#javafx

Siedlerchr · 2021-01-02T16:13:22Z

Please have a look at the markdownlint checkstlye output. Then I would be happy to merge

* upstream/master: fix checsktyle Fix for application dialogs opening in wrong displays (#7273) Only disable move to file dir when path equals (#7269) Improved detection of long DOI's within text (#7260) Add missing author and fix name Fix style of highlighted checkboxes while searching in preferences (#7258) Updates to institution citation keys (#7210) Bump xmlunit-core from 2.8.1 to 2.8.2 (#7251) Bump classgraph from 4.8.97 to 4.8.98 (#7250) Bump bcprov-jdk15on from 1.67 to 1.68 (#7249) Bump xmlunit-matchers from 2.8.1 to 2.8.2 (#7252) Bump unirest-java from 3.11.06 to 3.11.09 (#7254) Bump org.beryx.jlink from 2.23.0 to 2.23.1 (#7253) Bump pascalgn/automerge-action from v0.12.0 to v0.13.0 (#7255) # Conflicts: # src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

koppor · 2021-01-04T15:46:10Z

This PR causes the about dialog not opening a second time (see #7287). Some dialogs can also be opened twice (Customized Entry Types).

@faeludire Could you have a look please?

[WIP] Fix for application dialogs opening in wrong displays

ce5fd11

tobiasdiez reviewed Dec 29, 2020

View reviewed changes

[WIP] Fix for application dialogs opening in wrong displays

7aacc88

eludif added 2 commits December 30, 2020 19:25

[WIP] Fix for application dialogs opening in wrong displays

e856dda

[WIP] Fix for application dialogs opening in wrong displays

edc6d4b

eludif marked this pull request as ready for review December 30, 2020 23:39

eludif changed the title ~~[WIP] Fix for application dialogs opening in wrong displays~~ Fix for application dialogs opening in wrong displays Dec 31, 2020

tobiasdiez reviewed Dec 31, 2020

View reviewed changes

Fix for application dialogs opening in wrong displays

f2dc4f2

- Rename DialogService.show() method to showCustomDialog() - Rename showCustomDialog()'s argument - Add documentation to showCustomDialog method - Make createDialog and createDialogWithOptOut non static

eludif requested a review from tobiasdiez December 31, 2020 13:43

tobiasdiez approved these changes Dec 31, 2020

View reviewed changes

eludif added 2 commits December 31, 2020 13:50

Merge branch 'master' into fix-for-issue-7272

cd6649e

Add change description to CHANGELOG.md

62ebcad

eludif requested a review from tobiasdiez December 31, 2020 17:47

tobiasdiez reviewed Jan 1, 2021

View reviewed changes

Drop unneeded test AboutActionTest

770273e

eludif requested a review from tobiasdiez January 1, 2021 17:07

tobiasdiez approved these changes Jan 2, 2021

View reviewed changes

Siedlerchr approved these changes Jan 2, 2021

View reviewed changes

Document guidelines for creating a new dialog

f5b9da5

eludif requested review from Siedlerchr and tobiasdiez January 2, 2021 15:38

Document guidelines for creating a new dialog

bea0e31

Siedlerchr merged commit 0b4be8a into JabRef:master Jan 2, 2021

This was referenced Jan 4, 2021

Fix for issue 7287 #7289

Closed

Fix for issue 7287 #7290

Merged

Uh oh!

Conversation

eludif commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

eludif commented Dec 29, 2020

Uh oh!

tobiasdiez commented Dec 30, 2020

Uh oh!

eludif commented Dec 30, 2020

Uh oh!

tobiasdiez commented Dec 30, 2020

Uh oh!

eludif commented Dec 30, 2020

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tobiasdiez Dec 31, 2020

Choose a reason for hiding this comment

Uh oh!

eludif Dec 31, 2020

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

eludif commented Dec 31, 2020

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

eludif commented Jan 2, 2021

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Jan 2, 2021

Uh oh!

koppor commented Jan 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eludif commented Dec 29, 2020 •

edited

Loading