After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 689528 - switcherPopup: Factor out altTab and ctrlAltTab's common code
switcherPopup: Factor out altTab and ctrlAltTab's common code
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks: 688913
 
 
Reported: 2012-12-03 13:28 UTC by Rui Matos
Modified: 2012-12-04 09:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
switcherPopup: Factor out altTab and ctrlAltTab's common code (70.14 KB, patch)
2012-12-03 13:28 UTC, Rui Matos
committed Details | Review
windowManager: Make use of the switch-panels-backward keybinding (1.50 KB, patch)
2012-12-03 13:29 UTC, Rui Matos
committed Details | Review
AppSwitcher: Remove a lost timeout on destroy (998 bytes, patch)
2012-12-03 13:29 UTC, Rui Matos
committed Details | Review

Description Rui Matos 2012-12-03 13:28:54 UTC
Since I'm doing another switcher popup (for input sources) we might
start sharing code right away.
Comment 1 Rui Matos 2012-12-03 13:28:56 UTC
Created attachment 230523 [details] [review]
switcherPopup: Factor out altTab and ctrlAltTab's common code
Comment 2 Rui Matos 2012-12-03 13:29:00 UTC
Created attachment 230524 [details] [review]
windowManager: Make use of the switch-panels-backward keybinding

We have it in the schema so there's no reason for it not to work.
Comment 3 Rui Matos 2012-12-03 13:29:03 UTC
Created attachment 230525 [details] [review]
AppSwitcher: Remove a lost timeout on destroy

Doesn't look harmful but there's no point in running this code after
we're destroyed.
Comment 4 Rui Matos 2012-12-03 13:30:11 UTC
(In reply to comment #1)
> Created an attachment (id=230523) [details] [review]
> switcherPopup: Factor out altTab and ctrlAltTab's common code

Note that this is just code shuffling. There shouldn't be any logic changes here.
Comment 5 Jasper St. Pierre (not reading bugmail) 2012-12-03 17:45:01 UTC
I'd want to wait for Florian to land his alt-alt-tab proposal first.
Comment 6 Florian Müllner 2012-12-03 17:47:59 UTC
(In reply to comment #5)
> I'd want to wait for Florian to land his alt-alt-tab proposal first.

Nope, Rui and I agreed that it makes sense to land his changes first. I'm currently cleaning up an updated patch series on top of the patches here.
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-12-03 17:49:32 UTC
Review of attachment 230525 [details] [review]:

Sure.
Comment 8 Jasper St. Pierre (not reading bugmail) 2012-12-03 18:05:32 UTC
Review of attachment 230523 [details] [review]:

You should be able to use Layout.MonitorConstraint here to remove even more code.

Otherwise, fine. I can't spot any major flaws.

::: js/ui/windowManager.js
@@ +622,3 @@
         let modifiers = binding.get_modifiers();
         let backwards = modifiers & Meta.VirtualModifier.SHIFT_MASK;
+        Main.ctrlAltTabManager.popup(backwards, binding.get_name(), binding.get_mask());

I think having a manager for both is a bit silly. I'd see if we can share a generic SwitcherManager, or if we even need the manager anymore.
Comment 9 Jasper St. Pierre (not reading bugmail) 2012-12-03 18:05:46 UTC
Review of attachment 230524 [details] [review]:

Sure.
Comment 10 Matthias Clasen 2012-12-03 23:59:38 UTC
Review of attachment 230523 [details] [review]:

switcherPopup.js is missing from Makefile.am, causing it to not get installed, and the shell to crash on startup...
Comment 11 Rui Matos 2012-12-04 09:13:08 UTC
Thanks for the review. I'll think about further cleanups as I finish
the input sources switcher.

Pushing with the new file added to Makefile.am now.

Attachment 230523 [details] pushed as 00338bb - switcherPopup: Factor out altTab and ctrlAltTab's common code
Attachment 230524 [details] pushed as cd58f9f - windowManager: Make use of the switch-panels-backward keybinding
Attachment 230525 [details] pushed as 5faeaa2 - AppSwitcher: Remove a lost timeout on destroy