[MM-62205] Dynamically toggle widget window visibility when popout opens/closes #3253

streamer45 · 2024-12-12T20:08:35Z

Summary

PR adds some logic to dynamically change the global Calls widget visibility when the popout window is open. This is to address the inconvenience of having this window always on top even when focusing on the popout.

Hiding the widget completely is also possible, but I still feel it's a more impactful change in behavior. My hope is that the proposed changes are enough to address the reported issue. That said, let me know if you feel strongly about this.

The previous conversation for extra context is at https://hub.mattermost.com/private-core/pl/wbf4jd67g7fnpmon8fafwnxmuy

Video

widget_visibility.mp4

Ticket Link

https://mattermost.atlassian.net/browse/MM-62205

Release Note

Calls: while the popout window is open, the widget window's visibility will change so that it is not always on top of other windows.

matthewbirtch · 2024-12-12T20:34:56Z

Nice solve @streamer45. I like this as a solution and I think it will solve the biggest part of the issue. I've triggered a build to test and play with a bit and I'll report back.

matthewbirtch · 2024-12-12T21:02:56Z

I think this feels good to me. While testing a few things I noticed (that may or may not be as a results of this PR)

I'm not sure if this is a results of this change, but when I tried to share my screen I was unable to from the popout view. I had not yet provided permissions, it did not trigger the OS permission dialog either. It just did nothing. Once I cliked the share button from the widget, the OS prompt came up and allowed me to share. However, even after allowing the OS permission from there, I was still unable to share my screen from the popout.
The widget window shows up in the list of screens to share. I don't believe it does this currently in the desktop app and we should exclude it.

streamer45 · 2024-12-12T21:15:45Z

I think this feels good to me. While testing a few things I noticed (that may or may not be as a results of this PR)

I'm not sure if this is a results of this change, but when I tried to share my screen I was unable to from the popout view. I had not yet provided permissions, it did not trigger the OS permission dialog either. It just did nothing. Once I cliked the share button from the widget, the OS prompt came up and allowed me to share. However, even after allowing the OS permission from there, I was still unable to share my screen from the popout.

The widget window shows up in the list of screens to share. I don't believe it does this currently in the desktop app and we should exclude it.

Thanks @matthewbirtch . I think both are unrelated to this PR.

For 1, I am honestly surprised this wasn't reported before but I'll have a look asap. Created https://mattermost.atlassian.net/browse/MM-62241

For 2, from what I remember, that's always been the case since the widget is effectively its own window. Excluding it may require us to do some pattern matching on the window title because (as far as I know) there isn't a great way to uniquely identify that specific window from the sources returned by the desktop capturing engine. I'll see what can be done. Created https://mattermost.atlassian.net/browse/MM-62242

matthewbirtch · 2024-12-12T21:17:12Z

Hmm, interesting. I tried to replicate both of these things on hub with desktop v5.10.1 and didn't get the same result.

streamer45 · 2024-12-12T21:22:28Z

Hmm, interesting. I tried to replicate both of these things on hub with desktop v5.10.1 and didn't get the same result.

On Linux (both 5.9 and 5.10), I can definitely see the widget being among the windows available to share. But yeah, I can't reproduce the sharing from popout issue outside of master branch so there could be something there.

devinbinnie

Nice little QoL fix :)

davidkrauser

Thank you for tackling this so quickly - this addresses my concerns with the widget 👍

Approving, but just have a minor non-blocking suggestion about the name of a function.

davidkrauser · 2024-12-13T15:04:12Z

src/main/windows/callsWidgetWindow.ts

@@ -233,16 +233,31 @@ export class CallsWidgetWindow {
        ev.preventDefault();
    };

+    private toggleWidgetVisibility = (flag: boolean) => {


suggestion: I initially found the visibility terminology here confusing, and thought the purpose of this function was to hide/show the widget. Could we use a name more specific to what we're toggling? e.g. it seems the purpose here is to toggle if the widget should be shown on all workspaces on top of other windows.

@davidkrauser Thanks. You know what they say about naming things :p

For fun, I asked Copilot for a better function name, but its response was far from convincing (it is still using the word visibility). Do you happen to have a suggestion in mind?

Maybe we could use something like priority since we are essentially mutating the level of the window.

You know what they say about naming things

I know, right? 😅

I like priority - maybe something like setWindowHighPriority(bool)?

Priority could be confusing too... Maybe setWindowHighZIndex because we're kind of talking about window z-indexes?

(apologies for the bikeshedding)

matthewbirtch · 2024-12-13T18:00:38Z

I can't reproduce the sharing from popout issue outside of master branch so there could be something there.

Ok, let me know if you want to look in to this as part of this PR or not. Happy to help test

matthewbirtch

Approving as the issues I identified don't appear to be a part of this PR. Thanks @streamer45!

cpoile

Thanks!

cpoile · 2024-12-17T14:10:51Z

src/main/windows/callsWidgetWindow.ts

@@ -233,16 +233,31 @@ export class CallsWidgetWindow {
        ev.preventDefault();
    };

+    private toggleWidgetVisibility = (flag: boolean) => {


Priority could be confusing too... Maybe setWindowHighZIndex because we're kind of talking about window z-indexes?

(apologies for the bikeshedding)

cpoile · 2024-12-17T14:12:44Z

src/main/windows/callsWidgetWindow.ts

@@ -233,16 +233,31 @@ export class CallsWidgetWindow {
        ev.preventDefault();
    };

+    private toggleWidgetVisibility = (flag: boolean) => {


Another: flag has little meaning... Anything else would be better, imo. highIndex highVisibility etc.?

@cpoile Interestingly, I copied that arg name straight from the Electron API for one of those methods :) I'll see what I can do.

streamer45 · 2024-12-17T21:24:44Z

@cpoile Check 59a3d93 and let me know if it's better. I changed the method's name and also made its argument explicit so that it's easier to understand its meaning from the calling point.

cpoile

Awesome! :)

Dynamically toggle widget window visibility when popout opens/closes

c418450

streamer45 added 2: Dev Review Requires review by a core committer 1: UX Review Requires review by a UX Designer labels Dec 12, 2024

streamer45 requested review from cpoile, matthewbirtch and devinbinnie December 12, 2024 20:08

streamer45 self-assigned this Dec 12, 2024

mm-cloud-bot added release-note labels Dec 12, 2024

matthewbirtch added the Build Apps for PR Builds signed builds for testing label Dec 12, 2024

devinbinnie approved these changes Dec 13, 2024

View reviewed changes

davidkrauser approved these changes Dec 13, 2024

View reviewed changes

streamer45 mentioned this pull request Dec 13, 2024

[MM-62001] Calls: switch tab on actions requiring focus #3235

Merged

matthewbirtch approved these changes Dec 13, 2024

View reviewed changes

cpoile approved these changes Dec 17, 2024

View reviewed changes

Improve method naming

59a3d93

cpoile approved these changes Dec 17, 2024

View reviewed changes

streamer45 added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer 1: UX Review Requires review by a UX Designer labels Dec 17, 2024

streamer45 merged commit f418475 into master Dec 17, 2024
22 checks passed

streamer45 deleted the MM-62205 branch December 17, 2024 21:57

amyblais added this to the v5.11.0 milestone Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-62205] Dynamically toggle widget window visibility when popout opens/closes #3253

[MM-62205] Dynamically toggle widget window visibility when popout opens/closes #3253

streamer45 commented Dec 12, 2024

matthewbirtch commented Dec 12, 2024

matthewbirtch commented Dec 12, 2024 •

edited

Loading

streamer45 commented Dec 12, 2024

matthewbirtch commented Dec 12, 2024

streamer45 commented Dec 12, 2024

devinbinnie left a comment

davidkrauser left a comment

davidkrauser Dec 13, 2024

streamer45 Dec 13, 2024

davidkrauser Dec 13, 2024

cpoile Dec 17, 2024

matthewbirtch commented Dec 13, 2024

matthewbirtch left a comment

cpoile left a comment

cpoile Dec 17, 2024

cpoile Dec 17, 2024

streamer45 Dec 17, 2024

streamer45 commented Dec 17, 2024

cpoile left a comment

[MM-62205] Dynamically toggle widget window visibility when popout opens/closes #3253

[MM-62205] Dynamically toggle widget window visibility when popout opens/closes #3253

Conversation

streamer45 commented Dec 12, 2024

Summary

Video

Ticket Link

Release Note

matthewbirtch commented Dec 12, 2024

matthewbirtch commented Dec 12, 2024 • edited Loading

streamer45 commented Dec 12, 2024

matthewbirtch commented Dec 12, 2024

streamer45 commented Dec 12, 2024

devinbinnie left a comment

Choose a reason for hiding this comment

davidkrauser left a comment

Choose a reason for hiding this comment

davidkrauser Dec 13, 2024

Choose a reason for hiding this comment

streamer45 Dec 13, 2024

Choose a reason for hiding this comment

davidkrauser Dec 13, 2024

Choose a reason for hiding this comment

cpoile Dec 17, 2024

Choose a reason for hiding this comment

matthewbirtch commented Dec 13, 2024

matthewbirtch left a comment

Choose a reason for hiding this comment

cpoile left a comment

Choose a reason for hiding this comment

cpoile Dec 17, 2024

Choose a reason for hiding this comment

cpoile Dec 17, 2024

Choose a reason for hiding this comment

streamer45 Dec 17, 2024

Choose a reason for hiding this comment

streamer45 commented Dec 17, 2024

cpoile left a comment

Choose a reason for hiding this comment

matthewbirtch commented Dec 12, 2024 •

edited

Loading