Merge lp:~quequotion/slingshot/fix-1635776 into lp:~elementary-pantheon/slingshot/trunk
Status: | Rejected |
---|---|
Rejected by: | Cody Garver on 2017-01-07 |
Proposed branch: | lp:~quequotion/slingshot/fix-1635776 |
Merge into: | lp:~elementary-pantheon/slingshot/trunk |
Diff against target: |
106 lines (+20/-12) 1 file modified
src/SlingshotView.vala (+20/-12) |
To merge this branch: | bzr merge lp:~quequotion/slingshot/fix-1635776 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Cody Garver | Disapprove on 2017-01-07 | ||
Santiago | 2016-11-12 | Needs Fixing on 2016-11-21 | |
Daniel Fore | Needs Fixing on 2016-11-17 | ||
Kirill Romanov (community) | test | Needs Fixing on 2016-11-17 | |
Review via email:
|
Commit message
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.
Description of the change
Update, rebased on upstream and gandalfn's branch:
Give event_box proper event masks, remove (redundant?) "search_
Previously:
use "im_context_
//scrolling over the application icons probably requires the eventbox
Daniel Fore (danrabbit) wrote : | # |
I can confirm that this commit broke scrolling :(
Kirill Romanov (djaler1) wrote : | # |
@danrabbit, so, can you revert this commit for now?
Santiago (santileortiz) wrote : | # |
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) wrote : | # |
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:/
https:/
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_
Could someone, if just for the science of it, attempt to duplicate my results with wingpanel-
quequotion (quequotion) wrote : | # |
Do you need the EventBox (with gala)?
quequotion (quequotion) wrote : | # |
I really didn't write this well; /omitted details/:
This behavior is not affected (for me) with the "im_context_
quequotion (quequotion) wrote : | # |
> Do you need the EventBox (with gala)?
Tried with patch reduced to only:
@@ -562,7 +549,7 @@
- search_
+ search_
(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) wrote : | # |
With a little adjustment, gandalfn's branch works:
https:/
Both gandalfn's patch and the debian patch additionally allow scrolling over application icons!
- 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) wrote : | # |
Not sure how fcitx works without im_context_
xhhqzxg24 (xhhqzxg24) wrote : | # |
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) wrote : | # |
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/SlingshotV
--- src/SlingshotVi
+++ src/SlingshotVi
@@ -364,6 +364,7 @@
}
+ print ("Key: %s\n", key);
switch (key) {
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) wrote : | # |
> 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) wrote : | # |
> 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_
Cody Garver (codygarver) wrote : | # |
I am rejecting this branch because we are accepting the simpler proposal (https:/
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.
Ezekiel Michael Angel (nolenumar) wrote : | # |
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:/
> --
> You are subscribed to branch lp:slingshot.
>
quequotion (quequotion) wrote : | # |
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
It broke scrolling for me.