Skip to content

Conversation

@LenczuVex
Copy link

@LenczuVex LenczuVex requested a review from shpaass as a code owner March 8, 2025 11:04
@shpaass shpaass linked an issue Mar 8, 2025 that may be closed by this pull request
@shpaass
Copy link
Owner

shpaass commented Mar 8, 2025

Rebased on fresh master.

@shpaass
Copy link
Owner

shpaass commented Mar 8, 2025

Asking just in case,
@LenczuVex, have you tested that the Welcome Screen, File Selector, and About windows work as intended?

@LenczuVex
Copy link
Author

I have rechecked that and this behaviour can be not actually intended one. My WM automatically focused the window so when I clicked away code worked, window stayed during welcome screen, and everything seamed good, but as I retested I realized that on most systems you would have to actually first focus on the window after closing for example welcome and then unclick that. I'll try to find better solution.

@LenczuVex
Copy link
Author

I have found new solution that maybe can work as permanent one, tested on Linux and Windows 11.

Copy link
Collaborator

@DaleStan DaleStan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to have the parent window send the focus back to the child window, if that's possible. Otherwise, this looks good to me.

Copy link
Owner

@shpaass shpaass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you and welcome to Yafc!

@LenczuVex
Copy link
Author

Thanks for warm welcome. ❤️

I can add the focus functionality in other pull request, I am currently dealing with some university stuff and don't have time to do new behaviour. I should be able to resolve it until Friday, so if you prefer to make this change here then I'll probably do it over the weekend. I think it's better to make seperate pull request for it, because it's something slightly different.

Also I forgot to run environment setup script and there are in fact missing braces in if statement, bit I can resolve it this weekend earliest, with the other feature, so it might be easier if you fix it depending on your availability.

@shpaass
Copy link
Owner

shpaass commented Mar 12, 2025

Take your time, there's no rush.

Copy link
Owner

@shpaass shpaass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the approval for now so I don't miss the changes.

@DaleStan
Copy link
Collaborator

I saw another comment about this in Discord, so I resolved the conflict and adjusted the PR to reactivate the child.

@veger
Copy link
Collaborator

veger commented Aug 29, 2025

You might need to add a changelog entry so others are notified the issue should be fixed

@shpaass
Copy link
Owner

shpaass commented Aug 29, 2025

You might need to add a changelog entry so others are notified the issue should be fixed

I can do the changelog part, no worries.

The reason why this PR has not been merged yet is that I am concerned about the UI-architecture point that @DaleStan mentioned. In other words, I need to understand the severity of the pointed-out issue. The main thing that we want to prevent is for the UI architecture to get even more convoluted.

@shpaass
Copy link
Owner

shpaass commented Aug 29, 2025

I'd prefer to have the parent window send the focus back to the child window

Could you please specify what part of the patch is related to that suggestion?
The only way I managed to interpret it is that there was a mixup in the suggestion, and it was about that the child window should give the focus back to the parent on being closed. But I am not sure.

@shpaass
Copy link
Owner

shpaass commented Aug 29, 2025

@DaleStan ^, forgot to ping you in the previous message.

@veger
Copy link
Collaborator

veger commented Aug 29, 2025

I understood that Dale's additional commit (Re-activate the child window if the parent gains focus) addressed his concern. But let's wait on confirmation

@shpaass
Copy link
Owner

shpaass commented Aug 29, 2025

Good catch, you're right. Apologies for the confusion.

@shpaass
Copy link
Owner

shpaass commented Aug 29, 2025

Then I'll squash the commits and add a changelog entry before the merge.

On Linux, the file-picker in the Welcome Window would close right
after opening it.

This patch changes the logic of when the utility windows are closed.
Previously, they closed on losing focus. That likely was the reason
for the bug because they lost focus on creation. Now they don't
close on losing focus.

Co-authored-by: Dale McCoy <[email protected]>
@shpaass
Copy link
Owner

shpaass commented Aug 29, 2025

@DaleStan, please check if I interpreted the implementation correctly in the commit message. Feel free to change it if necessary.

@DaleStan
Copy link
Collaborator

Looks good to me

@DaleStan DaleStan merged commit ae27cb3 into shpaass:master Aug 30, 2025
1 check passed
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.

File picker instantly closes

4 participants