Skip to content

Conversation

@pc223
Copy link
Contributor

@pc223 pc223 commented Jun 22, 2021

Bug reproduce steps:

  1. Right click on Flow icon in Notification Area, click settings
  2. Minimize Settings-window (or Alt+Tab to different window)
  3. Right click on Flow icon in Notification Area, click settings
  4. Settings-window will not show back up

I'm not really sure why this solution works tho 😅

Maybe .Show() fails when window.WindowState == Minimized (not Normal)
Not sure why need .Activate() too

Ref: https://stackoverflow.com/a/59719760/4230390

Please delete unnecessary comments if this is too obvious to C# devs 😅

…kbar by user

Not sure why this works tho
Maybe `.Show()` failed when `window.WindowState == Minimized` (not `Normal`)
Not sure why need .Activate() too
Ref: https://stackoverflow.com/a/59719760/4230390
@pc223 pc223 marked this pull request as ready for review June 22, 2021 10:52
@pc223 pc223 added the bug Something isn't working label Jun 22, 2021
@taooceros
Copy link
Member

https://docs.microsoft.com/en-us/dotnet/api/system.windows.window.activate?view=net-5.0

refer this, it will call a SetForeGround invoke (which is a pinvoke I learnt in WindowWalker plugin), so it will set specific window.

Question, do we need to set the windows' state? or calling Activate will be fine?

@taooceros taooceros added this to the 1.8.0 milestone Jun 22, 2021
@pc223
Copy link
Contributor Author

pc223 commented Jun 22, 2021

Question, do we need to set the windows' state? or calling Activate will be fine?

Lemme quick test that, see if it works without Activate()

@pc223
Copy link
Contributor Author

pc223 commented Jun 22, 2021

Question, do we need to set the windows' state? or calling Activate will be fine?

Lemme quick test that, see if it works without Activate()

Yes, seems that without Activate() still works, I think we can remove that line

@pc223
Copy link
Contributor Author

pc223 commented Jun 22, 2021

If we don't window.WindowState = WindowState.Normal, and only use Activate(), then it changes color of the Settings-window in taskbar (activated?) but not open Settings-window. The WindowState seems is the key 🤔

@taooceros
Copy link
Member

If we don't window.WindowState = WindowState.Normal, and only use Activate(), then it changes color of the Settings-window in taskbar (activated?) but not open Settings-window. The WindowState seems is the key 🤔

Interesting, I finally understand why sometimes windowwalker won't actually show up the window.

@taooceros taooceros requested review from jjw24 and taooceros June 22, 2021 16:24
@taooceros
Copy link
Member

Could you change the comment for Activate()?

@pc223
Copy link
Contributor Author

pc223 commented Jun 22, 2021

Done!

@taooceros
Copy link
Member

Done!

Ha🤔 Does Activate don't need to be included? Could you give it a quick test?
I just mean that it would be better to include more detailed interpretation why Activate() is included though lol.

@pc223
Copy link
Contributor Author

pc223 commented Jun 22, 2021

Done!

Ha🤔 Does Activate don't need to be included? Could you give it a quick test?
I just mean that it would be better to include more detailed interpretation why Activate() is included though lol.

Tested, still works without Activate, I'm to not sure why 😅 I tried remove .Focus(), still works 🤔

@taooceros
Copy link
Member

Tested, still works without Activate, I'm to not sure why 😅 I tried remove .Focus(), still works 🤔

If setting window is not minimized, but simply hided by other window, it may not work I guess. Though, I am not sure whether Focus can replace the need for Activate.

@taooceros
Copy link
Member

Could you give that a quick test?

@pc223
Copy link
Contributor Author

pc223 commented Jun 22, 2021

Could you give that a quick test?

Just tested, you're right! Remove .Focus() and only use Show(), Settings will be hidden by other windows.

But, still remove .Focus(), but add Activate(), it works, not hidden by other windows.

Seems that we can use either Focus() or Activate().

Any more test case that I can run? 😄

@taooceros
Copy link
Member

I guess that's enough. Thank you for testing! If Focus work, we can remove Activate().

Would you please remove one extra empty line? I think one is fine for separating the logic.

Then, I will approve, and merge later to let somebody else to review if they want.

Thank you for the fix.

@pc223
Copy link
Contributor Author

pc223 commented Jun 22, 2021

I guess that's enough. Thank you for testing! If Focus work, we can remove Activate().

Would you please remove one extra empty line? I think one is fine for separating the logic.

Then, I will approve, and merge later to let somebody else to review if they want.

Thank you for the fix.

Like this? I tend to go overboard on whitespaces haha

image

@taooceros
Copy link
Member

Yes, thank you!

@taooceros taooceros merged commit 4b5e62d into Flow-Launcher:dev Jun 23, 2021
@pc223 pc223 deleted the fixSettingsNotShowFromMinimized branch June 23, 2021 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants