Merge lp:~quequotion/slingshot/fix-1635776 into lp:~elementary-pantheon/slingshot/trunk

[quequotion]

gandalfn (2016-05-06):
The scroll and key press/release event forwading from event box does not
work on debian testing.
Activate the default event mask for this events for event box.
Disconnect search entry key press/release event from event forwarding.
Send key press/release event forwarding from event box to search entry in all case.

Update, rebased on upstream and gandalfn's branch:
Give event_box proper event masks, remove (redundant?) "search_entry.key_press_event.connect (on_key_press);", always "search_entry.key_press_event (event);" and "return true;" instead of "return false;"

Previously:
use "im_context_filter_keypress (event)" to enable input method editors to focus on slingshot; remove eventbox
//scrolling over the application icons probably requires the eventbox

[Santiago (santileortiz)]

review: Approve

[Kirill Romanov (djaler1)]

It broke scrolling for me.

review: Needs Fixing (test)

[Danielle Foré (danrabbit)]

I can confirm that this commit broke scrolling :(

review: Needs Fixing

[Kirill Romanov (djaler1)]

@danrabbit, so, can you revert this commit for now?

[Santiago (santileortiz)]

review: Needs Fixing

[Santiago (santileortiz)]

The merge was reverted because it breaks scrolling with the mouse wheel for people even without IMs, I think events are being "eaten" by the search_entry widget because it has the focus. Previously even though this happened too, event_box was getting all of them too.

Anyway this is just speculation, I haven't tested all of this, but maybe adding EventBox back will fix it? (I have no idea what will happen to fcitx here).

[quequotion (quequotion)]

I do not have that problem now, buuuuuuut, I did until I patched all the gala and dbus I could find out of wingpanel:
https://github.com/quequotion/pantheon-bzr-qq/blob/master/wingpanel-standalone-bzr/minus-backgroundmanager.patch
https://github.com/quequotion/pantheon-bzr-qq/blob/master/wingpanel-standalone-bzr/minus-galaplugin.patch

I had that problem with vanilla wingpanel (bzr 154); but not after removing the background manager and gala plugins.

Note, when I say "scrolling is working for me with these patches" I mean that it works to scroll from page to page, but only when the mouse is over the category list or on either side of page numbers; my category list is not long enough to scroll, scroll always goes from page to page of application icons. Scrolling does not work when the mouse is over the icons. Is that normal?

This behavior is not affected (for me) with the "im_context_filter_keypress (event)" patch. In vanilla wingpanel scrolling is broken for me (and possibly other users running wingpanel without gala), in wingpanel-standalone scrolling works; in wingpanel with only the "im_context_filter_keypress (event)" patch scrolling still does not work (for me, and aparently everyone else).

Could someone, if just for the science of it, attempt to duplicate my results with wingpanel-standalone patches {and,or} the "im_context_filter_keypress (event)" patch? This could be a bug revealed, rather than caused, by my proposed merge.

[quequotion (quequotion)]

Do you need the EventBox (with gala)?

[quequotion (quequotion)]

I really didn't write this well; /omitted details/:

This behavior is not affected (for me) with the "im_context_filter_keypress (event)" patch /against slingshot/. In vanilla wingpanel scrolling /in vanilla slingshot/ is broken for me (and possibly other users running wingpanel/+slingshot/ without gala), in wingpanel-standalone scrolling /in vanilla slingshot/ works; in /vanilla/ wingpanel with only the "im_context_filter_keypress (event)" patch /against slingshot/ scrolling still does not work (for me, and aparently everyone else).

[quequotion (quequotion)]

> Do you need the EventBox (with gala)?
Tried with patch reduced to only:
@@ -562,7 +549,7 @@
                     if (!search_entry.has_focus) {
                         search_entry.grab_focus ();
                         search_entry.move_cursor (Gtk.MovementStep.BUFFER_ENDS, 0, false);
- search_entry.key_press_event (event);
+ search_entry.im_context_filter_keypress (event);
                     }
                     return false;

(restored EventBox code)

This did not help. Although scrolling (still) works, the EventBox causes two problems: each input character is doubled and slingshot does not refresh view while typing in the search box (or possibly process input at all) until changing desktops (ergo, likely on full screen redraw).

I really think the EventBox is problematic.

[quequotion (quequotion)]

With a little adjustment, gandalfn's branch works:
https://code.launchpad.net/~gandalfn/slingshot/fix-input-event/+merge/312215/comments/811190

Both gandalfn's patch and the debian patch additionally allow scrolling over application icons!

[lp:~quequotion/slingshot/fix-1635776]

703. By quequotion on 2016-12-05

gandalfn's fix, slightly updated

704. By quequotion on 2016-12-05

Revert last

705. By quequotion on 2016-12-05

Rebase

706. By quequotion on 2016-12-05

gandalfn's patch retry

[quequotion (quequotion)]

Not sure how fcitx works without im_context_filter_keypress here, but it does so updated gandalfn's method, minus (stupendously minor) conflicts.

[xhhqzxg24 (xhhqzxg24)]

Thanks for your works, quequotion.
But I have to report a bug about this fixed.
I have two fcitx IM engines: fcitx-googlepinyin and fcitx-rime.
When I switch from rime to google (or from google to rime) on the searchbar,
everything has been stoped without keyboard.
Thanks again.

[Santiago (santileortiz)]

I just tested it, and it does fix scrolling and fcitx, but when I use Escape to quit Slingshot, if the search bar has focus, it won't close. I did the following to see what was going on:

=== modified file 'src/SlingshotView.vala'
--- src/SlingshotView.vala 2016-12-05 18:39:48 +0000
+++ src/SlingshotView.vala 2016-12-07 15:45:38 +0000
@@ -364,6 +364,7 @@
                 return true;
             }

+ print ("Key: %s\n", key);
             switch (key) {
                 case "F4":
                     if ((event.state & Gdk.ModifierType.MOD1_MASK) != 0) {

It seems that the when search_entry gains focus it grabs all events, and the only ones we receive in Slingshot are the ones it does not process, like EventScroll, or several modifiers like Alt_L (which is why Alt+F4 still works), but for some reason it's consuming Escape key presses, so it stops working.

Also, when pressing F1 while search_entry has focus, Slingshot enters an infinite loop and starts receiving a keypress event for F1 like crazy, maybe this is what the original comment was referring to?.

[quequotion (quequotion)]

> I have two fcitx IM engines: fcitx-googlepinyin and fcitx-rime.
> When I switch from rime to google (or from google to rime) on the searchbar,
> everything has been stoped without keyboard.
> Thanks again.

I have pinyin and anthy and this does not happen.
What key do you press to scroll input methods?

I have set CTRL+L_Shift to scroll input methods and CTRL+Spacebar to trigger input methods.

[quequotion (quequotion)]

> It seems that the when search_entry gains focus it grabs all events, and the
> only ones we receive in Slingshot are the ones it does not process, like
> EventScroll, or several modifiers like Alt_L (which is why Alt+F4 still
> works), but for some reason it's consuming Escape key presses, so it stops
> working.
Alt+F4 does not work for me--are you sure that's working through slingshot or by the window manager? Neither does Shift+Backspace. Tab and the other keys work, so long as there's no text in the search box. I can't see how this is caused by this patch, although it apparently is.

> Also, when pressing F1 while search_entry has focus, Slingshot enters an
> infinite loop and starts receiving a keypress event for F1 like crazy, maybe
> this is what the original comment was referring to?.
I don't think so. That comment regarded the case of overriding key_press_event to change key handling (which requires im_context_filter_keypress to avoid an infinite loop and enable input methods), rather than using an EventBox to intercept input. I think you have found a different infinite loop. What should F1 do?

[Cody Garver (codygarver)]

I am rejecting this branch because we are accepting the simpler proposal (https://code.launchpad.net/~santileortiz/slingshot/fix-1635776/+merge/314255), which removes old hacky code that appears to be the source of the problem.

However, we've already reverted 1 commit that intended to fix this problem, so please stick around to make sure the problem is finally solved.

review: Disapprove

[Ezekiel Michael Angel (nolenumar)]

Hey how can I unsubscribe from this emails? I get so many :(

On Jan 7, 2017 6:28 AM, "Cody Garver" <email address hidden> wrote:

> The proposal to merge lp:~quequotion/slingshot/fix-1635776 into
> lp:slingshot has been updated.
>
> Status: Needs review => Rejected
>
> For more details, see:
> https://code.launchpad.net/~quequotion/slingshot/fix-1635776/+merge/310705
> --
> You are subscribed to branch lp:slingshot.
>

[quequotion (quequotion)]

The removal of outdated, hacky code sounds like a good idea.

Santiago's version is working for me. fcitx works, scrolling works, and special keys also work.

Unmerged revisions

706. By quequotion on 2016-12-05

gandalfn's patch retry

705. By quequotion on 2016-12-05

Rebase

704. By quequotion on 2016-12-05

Revert last

703. By quequotion on 2016-12-05

gandalfn's fix, slightly updated

=== modified file 'src/SlingshotView.vala'
--- src/SlingshotView.vala	2016-11-21 04:43:16 +0000
+++ src/SlingshotView.vala	2016-12-05 18:40:45 +0000
@@ -204,6 +204,7 @@
             container.attach (stack, 0, 1, 1, 1);
 
             event_box = new Gtk.EventBox ();
+            event_box.add_events (Gdk.EventMask.SCROLL_MASK | Gdk.EventMask.KEY_PRESS_MASK | Gdk.EventMask.KEY_RELEASE_MASK);
             event_box.add (container);
             // Add the container to the dialog's content area
 
@@ -233,7 +234,6 @@
             });
 
             event_box.key_press_event.connect (on_key_press);
-            search_entry.key_press_event.connect (on_key_press);
             // Showing a menu reverts the effect of the grab_device function.
             search_entry.search_changed.connect (() => {
                 if (modality != Modality.SEARCH_VIEW)
@@ -384,7 +384,8 @@
                 case "Enter": // "KP_Enter"
                 case "Return":
                 case "KP_Enter":
-                    return false;
+                    search_entry.key_press_event (event);
+                    return true;
 
                 case "Alt_L":
                 case "Alt_R":
@@ -402,8 +403,10 @@
                 case "9":
                     int page = int.parse (key);
 
-                    if (event.state != Gdk.ModifierType.MOD1_MASK)
-                        return false;
+                    if (event.state != Gdk.ModifierType.MOD1_MASK) {
+                        search_entry.key_press_event (event);
+                        return true;
+                    }
 
                     if (modality == Modality.NORMAL_VIEW) {
                         if (page < 0 || page == 9)
@@ -416,7 +419,8 @@
                         else
                             category_view.app_view.go_to_number (page);
                     } else {
-                        return false;
+                        search_entry.key_press_event (event);
+                        return true;
                     }
                     search_entry.grab_focus ();
                     break;
@@ -515,17 +519,19 @@
                     if (event.state == Gdk.ModifierType.SHIFT_MASK) { // Shift + Delete
                         search_entry.text = "";
                     } else if (search_entry.has_focus) {
-                        return false;
+                        search_entry.key_press_event (event);
+                        return true;
                     } else {
                         search_entry.grab_focus ();
                         search_entry.move_cursor (Gtk.MovementStep.BUFFER_ENDS, 0, false);
-                        return false;
+                        return true;
                     }
                     break;
 
                 case "Home":
                     if (search_entry.text.length > 0) {
-                        return false;
+                        search_entry.key_press_event (event);
+                        return true;
                     }
 
                     if (modality == Modality.NORMAL_VIEW) {
@@ -538,7 +544,8 @@
 
                 case "End":
                     if (search_entry.text.length > 0) {
-                        return false;
+                        search_entry.key_press_event (event);
+                        return true;
                     }
 
                     if (modality == Modality.NORMAL_VIEW) {
@@ -554,7 +561,8 @@
                     if ((event.state & (Gdk.ModifierType.CONTROL_MASK | Gdk.ModifierType.SHIFT_MASK)) != 0) {
                         search_entry.paste_clipboard ();
                     } else {
-                        return false;
+                        search_entry.key_press_event (event);
+                        return true;
                     }
                     break;
 
@@ -562,9 +570,9 @@
                     if (!search_entry.has_focus) {
                         search_entry.grab_focus ();
                         search_entry.move_cursor (Gtk.MovementStep.BUFFER_ENDS, 0, false);
-                        search_entry.key_press_event (event);
                     }
-                    return false;
+                    search_entry.key_press_event (event);
+                    return true;
 
             }