pop-os / gnome-control-center

Pop!_OS fork of https://git.launchpad.net/~ubuntu-desktop/ubuntu/+source/gnome-control-center
GNU General Public License v2.0
29 stars 10 forks source link

Backport 3.38.0 patched with multiple shortcuts UI #116

Closed ids1024 closed 4 years ago

ids1024 commented 4 years ago

This is based on the 3.38.0 tag of gnome-control-center. It adds the keyboard panel redesign changes that are pending upstream (expected to land in the next Gnome release, since it didn't make this one). It also adds the multiple shortcuts UI, which upstream seems unhappy with, so we expect to be maintaining it in our fork baring a re-design that the Gnome Design Team is more happy with.

This also adds the Debian directory from the current focal_master, with patches updated from https://salsa.debian.org/gnome-team/gnome-control-center and the pop patches that weren't applying fixed.

Closes https://github.com/pop-os/gnome-control-center/issues/104. The details of the UI changes are described there.

If approved, force push to focal_master.

ids1024 commented 4 years ago

Oh, I almost forgot, there are still two patches disabled here because they weren't applying.

0011-region-Add-Language-Selector-button.patch I'll just want to update to apply again. Not really a problem.

Then there's 0030-temporarily-revert-alt-char-key.patch. Ubuntu disabled that due to concern about the way it behaves, until it's improved upstream (which hasn't changed; it doesn't really seem clear exactly how it should be fixed). It would be easy to update the patch to apply again, if that's what we want.

ids1024 commented 4 years ago

Also might be worth noting that the changes here are begining to land upstream now that 3.38.0 has been branch off. My two refactoring merge requests have just been merged yesterday (this one and this one) and I've just opened another to continue getting this upstream.

jacobgkau commented 4 years ago

I've tested every page of the updated Control Center. GCC Testing.txt

The only regression I found is that Screen Sharing is not showing up as an option on the Sharing page. Before, Screen Sharing would show up as an option if Vino was installed (which we do have installed by default.) After the update, GCC is supposed to use gnome-remote-desktop instead of Vino (gnome-remote-desktop pulls in Pipewire and should work on X11 and Wayland.) However, even after installing gnome-remote-desktop and restarting GCC (and eventually rebooting), I am not seeing Screen Sharing appear as an option.

This is the upstream merge request that replaced Vino with gnome-remote-desktop: https://gitlab.gnome.org/GNOME/gnome-control-center/-/merge_requests/767

We might need to update gnome-settings-daemon to match gnome-control-center. I see that we have an Ubuntu patch adding Vino support back into GSD, which should no longer be needed: https://github.com/pop-os/gnome-settings-daemon/blob/829615379cec3371b66614c03055fb4685434fe4/debian/patches/ubuntu_vino_handling.patch (upstream GSD dropped Vino support before gnome-remote-desktop supported X11, which is why Ubuntu patched Vino support back in.)

ids1024 commented 4 years ago

However, even after installing gnome-remote-desktop and restarting GCC (and eventually rebooting), I am not seeing Screen Sharing appear as an option.

It seems this requires support in Mutter as well, which appears to be disabled: https://github.com/pop-os/mutter/blob/fa12717e84c1665ce316dc01c497a8a436bce39b/debian/rules#L38-L47

dpkg-vendor --query vendor does return Ubuntu, so it presumably is being build without that.

Edit: Or, I guess that disables it in any case...

ids1024 commented 4 years ago

Pushed a commit reverting the changes removing Vino support, since the patches revert cleanly and any other solution is likely both more complicated and more likely to break things.

jacobgkau commented 4 years ago

I can confirm screen sharing displays again and works as expected now.

Something I noticed in the new Keyboard Shortcuts section while going through everything again: if I modify a shortcut, then go back to the list of categories, that category correctly shows "1 modified". If I click Reset All..., the reset does work, but the category still shows "1 modified". If I click into the category and then back to the list of categories, it no longer shows "1 modified".

This makes it look like Reset All... is not working when the number of modified shortcuts is not going away. Is it possible to refresh that when resetting?

ids1024 commented 4 years ago

Good catch. Luckily that one is quite easy to fix.

ids1024 commented 4 years ago

If I try to add a custom shortcut using a keybinding that was already in use for another shortcut, I'm correctly shown a warning and the button changes its label to Replace. However, clicking the Replace button seems to do the same thing as the Cancel button-- it just exits the dialog without adding that custom shortcut.

Looks like upstream master has the problem as well. Looking at the code, it doesn't look new.

In any case, I'll submit a MR upstream and push an update here.

ids1024 commented 4 years ago

I've pushed a fixed, and send an MR upstream: https://gitlab.gnome.org/GNOME/gnome-control-center/-/merge_requests/839

ids1024 commented 4 years ago

The other problems I'm seeing are regarding overriding built-in shortcuts that don't appear in the Keyboard Shortcuts section for customization-- for example, Super+P for GNOME changing display output, and Super+S for Pop Shell stacking. The difference between the old and new implementation

Existing upstream issue for this: https://gitlab.gnome.org/GNOME/gnome-control-center/-/issues/891

The best part is that in the case of an invisible conflict, it seems Mutter behaves non-deterministically regarding which actually works...

As for the issue with super+m and super+shift+m custom shortcuts... Hm, I wonder what's going on there.

ids1024 commented 4 years ago
  • Add two custom shortcuts, one bound to Super+M and the other bound to Super+Shift+M.
  • Test both shortcuts, they both work.
  • Remove the Super+M shortcut.
  • The Super+Shift+M shortcut no longer works.

Hm. I can't seem to reproduce this on this branch, or on upstream master.

jacobgkau commented 4 years ago

Hm. I can't seem to reproduce this on this branch, or on upstream master.

Interesting. I was able to make it happen several more times, although now that I've been trying it more, it's not happening every time. After I Reset All..., I've tried manually removing the default Toggle maximization state before adding the first custom shortcut (the only option in 3.36), and I've also tried leaving it there and using the Replace button when I add the first custom shortcut. Either way, I've seen the issue occur several times but I've also seen it not occur at least once.

When it happens, gsettings get ... binding still shows <Shift><Super>m, but typing Shift+Super+m just types a capital M into whatever application is open. Also, restarting GNOME Shell makes that remaining shortcut start working again. Other shortcuts still worked before I restarted GNOME Shell, though.

ids1024 commented 4 years ago

Also, restarting GNOME Shell makes that remaining shortcut start working again.

Ah, I thought this might be the case. Given this, and gsettings appears to have the values you would expect, I think the bug here is in Mutter, not Gnome Control Center. Unless there is in fact a conflicting <Shift><Super>m binding somewhere, which could cause the same behavior to to the previously mentioned nondeterminism about which shortcut takes effect...

Add this to the list of ways in which keybindings in Gnome are impressively broken.

jacobgkau commented 4 years ago

Also, restarting GNOME Shell makes that remaining shortcut start working again.

Ah, I thought this might be the case. Given this, and gsettings appears to have the values you would expect, I think the bug here is in Mutter, not Gnome Control Center.

I'm seeing the issue happen on a serw12 and a gaze14, but I've been trying to make it happen on a galp4 and am not seeing it there. On the gaze14, it happens in both NVIDIA mode and Integrated (Intel) mode, so I don't think it's an NVIDIA thing.

Just to re-iterate, since whatever this is isn't a regression, I don't think it's a reason to hold on this GCC update. We can open an issue for this Super+M problem once we're able to figure out why it's only happening on some machines.

ids1024 commented 4 years ago

Hm, some funny behavior from the "Manage Installed Languages" button (which is added by a patch from Ubuntu). It sometimes opens an empty window. But I don't know if the issue can be in Gnome Control Center, since it just opens a separate program (gnome-language-selector).

Screenshot from 2020-09-18 12-22-52

jacobgkau commented 4 years ago

Hm, some funny behavior from the "Manage Installed Languages" button (which is added by a patch from Ubuntu). It sometimes opens an empty window. But I don't know if the issue can be in Gnome Control Center, since it just opens a separate program (gnome-language-selector).

That was part of my checklist before, and I haven't been able to replicate this issue yet. I've been able to open Language Support about 40 times in a row on one machine, and numerous times on another machine as well. Does starting gnome-language-selector from a terminal ever give you the empty window? Do you not see the Checking available language support window when this happens?

ids1024 commented 4 years ago

Do you not see the Checking available language support window when this happens?

No, not when that issue occurs.

It seems to sometimes happen running from the terminal as well. No idea what's going on here, but it doesn't seem to be an issue in gnome-control-center, existing or new.

ids1024 commented 4 years ago

@maria-komarova Did we decide we wanted to include the Alternate Character Key, or continue patching it out?

It's disabled on Ubuntu, and thus Pop!_OS currently for the reason stated here:

https://github.com/pop-os/gnome-control-center/blob/master_focal/debian/patches/0030-temporarily-revert-alt-char-key.patch:

From: Gunnar Hjalmarsson gunnarhj@ubuntu.com Date: Mon, 6 Jul 2020 17:54:19 +0200 Subject: Revert the "Alternate Characters Key" commit temporarily

Bug-Ubuntu: https://launchpad.net/bugs/1867548 Bug-GNOME: https://gitlab.gnome.org/GNOME/gnome-control-center/issues/918

Currently the "Right Alt" key is set automatically as dconf value without user action, i.e. only by opening the Keyboard Shortcuts panel. This is too intrusive, and the design should be modified so no value is set without the user asking for it. This patch reverts the commit awaiting such a modificaion.

Fedora in contrast leaves it in, like stock Gnome (just check with Fedora 32).

As I understand, the behavior Ubuntu removed it for is pretty broken, though not having it available is also pretty broken on keyboard layouts where it's important...

Or actually, perhaps it would be better to patch it to have an off toggle like the Compose key. That seems best in lieu of a more major design change that ties it to the keyboard layout in use.

ids1024 commented 4 years ago

Or actually, perhaps it would be better to patch it to have an off toggle like the Compose key. That seems best in lieu of a more major design change that ties it to the keyboard layout in use.

Done. Hopefully everything should be fine to merge now.

maria-komarova commented 4 years ago

Or actually, perhaps it would be better to patch it to have an off toggle like the Compose key. That seems best in lieu of a more major design change that ties it to the keyboard layout in use.

I agree. Thanks for taking care of that.