PowerToys icon indicating copy to clipboard operation
PowerToys copied to clipboard

Hide Video Conferencing when muted

Open akabhirav opened this issue 3 years ago • 6 comments

Summary of the Pull Request

PR Checklist

  • [x] Closes: #14123 & #18548
  • [ ] Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • [ ] Tests: Added/updated and all pass
  • [ ] Localization: All end user facing strings can be localized
  • [ ] Dev docs: Added/updated
  • [ ] New binaries: Added on the required places
  • [ ] Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Hide Video Conferencing toolbar when mic and camera are muted This PR adds to the feature of VideoConferencing that allows us to hide the toolbar when Mic and Camera are both unmuted. This checkbox is now a select box where the user can select the condition on which to hide the toolbar.

There are three options:

  1. Never
  2. When both Mic and Camera are unmuted
  3. When both Mic and Camera are muted

The options are self-explanatory to how they modify the toolbar behaviour upon selecting them

Validation Steps Performed

akabhirav avatar Jul 05 '22 10:07 akabhirav

CLA assistant check
All CLA requirements met.

Wait a minute, does this PR fix https://github.com/microsoft/PowerToys/issues/18548?

AnonymousWP avatar Jul 05 '22 12:07 AnonymousWP

@AnonymousWP It's supposed to. But I am very new to programming in C# and C++ 17. Right now, I am having trouble saving the settings I created. I am trying to figure out how it works, and only then I will be able to tell if my modifications even work

akabhirav avatar Jul 10 '22 08:07 akabhirav

@check-spelling-bot Report

:red_circle: Please review

See the :open_file_folder: files view or the :scroll:action log for details.

Unrecognized words (1)

stringstream

Previously acknowledged words that are now absent binaryformatter gsuberland ICONWARNING IStorage IWork messageboxes OSVERSIONINFOW PRTL REMOTEDISPLAY REMOTESESSION serializationexception SYSLIB upto
To accept :heavy_check_mark: these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the [email protected]:akabhirav/PowerToys.git repository on the features/hide-when-muted branch (:information_source: how do I use this?):

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
-H "Content-Type: application/json" \
"https://api.github.com/repos/microsoft/PowerToys/issues/comments/1179709357" > "$comment_json"
comment_body=$(mktemp)
jq -r ".body // empty" "$comment_json" | tr -d "\\r" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")

patch_add=$(perl -e '$/=undef; $_=<>; if (m{Unrecognized words[^<]*</summary>\n*```\n*([^<]*)```\n*</details>$}m) { print "$1" } elsif (m{Unrecognized words[^<]*\n\n((?:\w.*\n)+)\n}m) { print "$1" };' < "$comment_body")

update_files
rm $comment_body
git add -u

If the flagged items do not appear to be text

If items relate to a ...

  • well-formed pattern.

    If you can write a pattern that would match it, try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

  • binary file.

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

github-actions[bot] avatar Jul 10 '22 11:07 github-actions[bot]

@htcfreek made the change that you suggested but kept unmuted in the second position

@AnonymousWP The code is in working condition. Worked in my local environment.

Please review the code and help me improve it so that we are able to merge this to main as soon as possible

I would also like comments on whether the feature implemented here seems logical and easy to understand? Especially the behaviour related to when "Camera is not in use".

akabhirav avatar Jul 10 '22 11:07 akabhirav

@AnonymousWP It's supposed to. But I am very new to programming in C# and C++ 17. Right now, I am having trouble saving the settings I created. I am trying to figure out how it works, and only then I will be able to tell if my modifications even work

That's great, thanks! I was hyped for the Video Conference Mute feature, but I don't use it due to this feature missing. Glad you're attempting to add it. Unfortunately I don't have enough programming knowledge of C# to contribute/help.

I think you should also take a look at the PR Checklist at the top of this PR.

AnonymousWP avatar Jul 10 '22 12:07 AnonymousWP

@akabhirav , please make sure to let us know when we should give another review here ;) Thank you!

jaimecbernardo avatar Aug 08 '22 16:08 jaimecbernardo

Posting this as a general comment as this affects this PR greatly

I had an idea when I started taking a look at it again. Tell me if this sounds intuitive to you guys

  1. Hides when both camera and microphone are muted
    • If the camera was previously not in use then everything automatically gets muted(or maybe it should just change the camera's state)
    • If the camera was previously in use then nothing changes
  2. Hides when both camera and microphone are unmuted
    • If the camera was previously not in use everything gets unmuted(or maybe it should just change the camera's state)
    • If the camera was previously in use nothing changes

I have added these changes in my latest commit. Please check it out

akabhirav avatar Aug 23 '22 23:08 akabhirav

@jaimecbernardo, @yuyoyuppe This PR is ready for review

akabhirav avatar Aug 23 '22 23:08 akabhirav

Thank you for the update, we have a lot of stuff pending for the next release, so we'll take a look once it's rolled out.

yuyoyuppe avatar Aug 24 '22 17:08 yuyoyuppe

@yuyoyuppe if that one item is all that is needed can we merge and just do the fox in a new PR asap?

crutkas avatar Oct 16 '22 19:10 crutkas

I think so, yes. Should we wait for @akabhirav confirmation?

yuyoyuppe avatar Oct 17 '22 15:10 yuyoyuppe

@yuyoyuppe Hey, I am sorry about this, I have been very busy with my work lately, and I am not free before the first week of November. If you guys want to, you can merge this change and make the fix, or I will make the changes after I come back

akabhirav avatar Oct 17 '22 17:10 akabhirav