Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure feature parity with other AppWindows for vault options #2988

Closed
wants to merge 4 commits into from

Conversation

JaniruTEC
Copy link
Contributor

@JaniruTEC JaniruTEC commented Jul 5, 2023

This PR ensures feature parity between the other methods in FxApplicationWindows and the new #showVaultOptionsWindow() added by #2985. Therefore this PR depends on #2985.

"Feature parity" in this case consists of:

  • allowing the caller to provide their own parent stage instead of using the primaryStage provided by FxApplicationWindows (4a191ac)
  • checking the provided stage and making it visible if it was not previously visible (0ce77cb)

Also see: #2981

@JaniruTEC JaniruTEC requested a review from infeo July 5, 2023 14:38
@JaniruTEC JaniruTEC mentioned this pull request Jul 7, 2023
@infeo
Copy link
Member

infeo commented Jul 10, 2023

@JaniruTEC Can you merge develop into this branch? Maybe just the UI is buggy, but even after merging its parent branch the Github UI is showing a diff.

@JaniruTEC
Copy link
Contributor Author

Merging fixed it for me. I also noticed the additional diffs.

@infeo
Copy link
Member

infeo commented Jul 11, 2023

I'm not convinced of this change. We enforce showing the main window, but in the next step we allow to have an arbitrary owner, not necessarily related to the main window. I would vote to remove showMainWindow() and just use the owner, in the sense of var actualowner = Optional.ofNullable(owner).elseGet(mainWindow). But looking at the functions of showUnlockWorkflow(), we also check if the vault is locked/unlocked. So overall, there is something missing here.

In the end, we want ensure that the user flow is sequential. First do something in the options, then close the options and only then unlock. If it is unlocked, do not allow opening the vault options.

By showing the mainWindow before opening the vault options, we ensure this. If we allow now arbitrary owner, unrelated owner, we could loose such sequentiality. I would say we apply YAGNI for now.

@infeo infeo closed this Jul 11, 2023
@tobihagemann tobihagemann deleted the feature/2988-app-windows-parity branch July 11, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants