Merge lp:~jeremywootten/slingshot/fix-1213321 into lp:~elementary-pantheon/slingshot/trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | David Gomes on 2013-09-30 | ||||
Approved revision: | 381 | ||||
Merged at revision: | 386 | ||||
Proposed branch: | lp:~jeremywootten/slingshot/fix-1213321 | ||||
Merge into: | lp:~elementary-pantheon/slingshot/trunk | ||||
Diff against target: |
377 lines (+60/-49) 3 files modified
src/Backend/AppSystem.vala (+4/-4) src/SlingshotView.vala (+44/-34) src/Widgets/CategoryView.vala (+12/-11) |
||||
To merge this branch: | bzr merge lp:~jeremywootten/slingshot/fix-1213321 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
if (community) | Needs Fixing on 2013-10-01 | ||
David Gomes | 2013-08-26 | Approve on 2013-09-30 | |
elementary Pantheon team | 2013-08-26 | Pending | |
Review via email:
|
Commit message
Prevents infinite loop in key_press_event handling when an input manager (ibus) is active, which appears to be due to a bug in either Gtk3 or ibus, by using an event box and custom handler rather than overriding the default handler.
Two other minor changes:
1) prevent some critical error messages when apps have no keywords.
2) prevent critical error message due to empty_cat_label not being created.
Fixes bug #1213321.
Description of the change
Prevents infinite loop in key_press_event handling when an input manager (ibus) is active, which appears to be due to a bug in either Gtk3 or ibus, by using an event box and custom handler rather than overriding the default handler.
Two other minor changes:
1) prevent some critical error messages when apps have no keywords.
2) prevent critical error message due to empty_cat_label not being created.
David Gomes (davidgomes) wrote : | # |
Very much agree with Shnatsel here. Only in rare occasions does code need to be commented, and this doesn't seem like one of those.
Besides, I approve of using the EventBox and the code style.
Jeremy Wootten (jeremywootten) wrote : | # |
The reason I left the old code in was in case you wanted to revert to the
old method if and when the Gtk/ibus bug is fixed, but I can take it out. I
will also put in some comments as you suggest.
thanks for the review
On 26 August 2013 14:06, David Gomes <email address hidden> wrote:
> Very much agree with Shnatsel here. Only in rare occasions does code need
> to be commented, and this doesn't seem like one of those.
>
> Besides, I approve of using the EventBox and the code style.
> --
>
> https:/
> You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
>
--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
- 377. By Jeremy Wootten on 2013-08-26
-
remove old code, add comment
David Gomes (davidgomes) wrote : | # |
https:/
Try that please.
Jeremy Wootten (jeremywootten) wrote : | # |
OK, but the Elementary coding style guide says that comments should be
indented along with the code. Your example is only indented by one space.
Is that intentional?
On 30 August 2013 09:15, David Gomes <email address hidden> wrote:
> Review: Needs Fixing
>
> https:/
>
> Try that please.
> --
>
> https:/
> You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
>
--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
David Gomes (davidgomes) wrote : | # |
It should be the current level of indentation + 2 spaces I believe.
Jeremy Wootten (jeremywootten) wrote : | # |
OK, so is this correct in this case (8 + 2 spaces indentation of comment
text)?
.
.
.
}
/*
messages
when an input method is in use (Gtk3 bug?). Key press events are
captured by an Event Box and passed to this function instead.
Events not dealt with here are propagated to the searchbar by the
usual mechanism.
*/
public bool on_key_press (Gdk.EventKey event) {
.
.
.
.
On 30 August 2013 19:02, David Gomes <email address hidden> wrote:
> It should be the current level of indentation + 2 spaces I believe.
> --
>
> https:/
> You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
>
--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
David Gomes (davidgomes) wrote : | # |
+ /*Overriding the default handler results in infinite loop of error messages
+ * when an input method is in use (Gtk3 bug?). Key press events are
+ * captured by an Event Box and passed to this function instead.
+ * Events not dealt with here are propagated to the searchbar by the
+ * usual mechanism.
+ */
+ /*
+ * Overriding the default handler results in infinite loop of error messages
+ * when an input method is in use (Gtk3 bug?). Key press events are
+ * captured by an Event Box and passed to this function instead.
+
+ * Events not dealt with here are propagated to the searchbar by the
+ * usual mechanism.
+ */
I'd prefer that and then I approve. Besides, I don't want this merged without testing (both on people experiencing the bug and people who don't use ibus and never experienced the bug).
Jeremy Wootten (jeremywootten) wrote : | # |
Did you want asterisks on each line? Your original suggestion did not have
them but this one does.
I agree that thorough testing by others first is advisable.
On 4 September 2013 13:27, David Gomes <email address hidden> wrote:
> Review: Needs Fixing
>
> + /*Overriding the default handler results in infinite loop of error
> messages
> + * when an input method is in use (Gtk3 bug?). Key press events are
> + * captured by an Event Box and passed to this function instead.
> + * Events not dealt with here are propagated to the searchbar by the
> + * usual mechanism.
> + */
>
> + /*
> + * Overriding the default handler results in infinite loop of error
> messages
> + * when an input method is in use (Gtk3 bug?). Key press events are
> + * captured by an Event Box and passed to this function instead.
> +
> + * Events not dealt with here are propagated to the searchbar by the
> + * usual mechanism.
> + */
>
> I'd prefer that and then I approve. Besides, I don't want this merged
> without testing (both on people experiencing the bug and people who don't
> use ibus and never experienced the bug).
> --
>
> https:/
> You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
>
--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
David Gomes (davidgomes) wrote : | # |
Oh, shit I don't, I'm dumb sometimes:
+ /*
+ Overriding the default handler results in infinite loop of error messages
+ when an input method is in use (Gtk3 bug?). Key press events are
+ captured by an Event Box and passed to this function instead.
+
+ Events not dealt with here are propagated to the searchbar by the
+ usual mechanism.
+ */
David Gomes (davidgomes) wrote : | # |
+ /*
+ Overriding the default handler results in infinite loop of error messages
+ when an input method is in use (Gtk3 bug?). Key press events are
+ captured by an Event Box and passed to this function instead.
+
+ Events not dealt with here are propagated to the searchbar by the
+ usual mechanism.
+ */
David Gomes (davidgomes) wrote : | # |
Launchpad is dumb - http://
- 378. By Jeremy Wootten on 2013-09-06
-
reformat comment
- 379. By Jeremy Wootten on 2013-09-06
-
Merge changes from trunk revision 378
Jeremy Wootten (jeremywootten) wrote : | # |
OK, done. I have also merged changes from trunk up to revision 378 and
resolved conflicts
On 4 September 2013 19:47, David Gomes <email address hidden> wrote:
> Launchpad is dumb - http://
> --
>
> https:/
> You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
>
--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
David Gomes (davidgomes) wrote : | # |
I approve of the code now, but I believe this needs testing.
I'll be running this version daily. I don't use iBus, so I'll look out for regressions.
This still needs verification from an iBus user. To test this fix, run:
sudo apt-get install elementary-
elementary-
David Gomes (davidgomes) wrote : | # |
It still doesn't fix focus 100% of times on Ubuntu 13.10, I think ibus is still ruining it.
Anyway, a branch got merged in this afternoon that makes it so that the Escape key hides slingshot if the search bar is empty, but clears the search bar if there's text on the search bar.
I branched trunk (lp:slingshot) and the bzr merged this branch and it breaks that new functionality.
iBus is not the only thing that causes focus issues. There seems to be a race condition or something as well. There was a branch floating around that fixed it.
Also, I haven't spotted any regressions against the current bzr trunk.
David Gomes (davidgomes) wrote : | # |
So if you merge this with the latest trunk you can use Escape to hide Slingshot?
Jeremy Wootten (jeremywootten) wrote : | # |
This branch was only intended to allow text entry into the search bar when
ibus was running. It was not intended to address the focus issues which, I
think, were pre-existing and not necessarily related to ibus. I have been
advised to keep branches "atomic".
I'll merge trunk and make sure this branch is compatible with the new
functionality.
On 10 September 2013 18:56, David Gomes <email address hidden> wrote:
> Review: Needs Fixing
>
> It still doesn't fix focus 100% of times on Ubuntu 13.10, I think ibus is
> still ruining it.
>
> Anyway, a branch got merged in this afternoon that makes it so that the
> Escape key hides slingshot if the search bar is empty, but clears the
> search bar if there's text on the search bar.
>
> I branched trunk (lp:slingshot) and the bzr merged this branch and it
> breaks that new functionality.
> --
>
> https:/
> You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
>
--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
- 380. By Jeremy Wootten on 2013-09-10
-
Merge from trung up to revision 382
- 381. By Jeremy Wootten on 2013-09-10
-
Make recent changes in trunk compatible with this branch
Jeremy Wootten (jeremywootten) wrote : | # |
I have now made the latest changes in trunk compatible with this branch
(only one line required changing). The "Escape" function did not require
changing but the "Shift-Ctrl-V" function did. Changes to the "on_key_press"
function in SlingshotView.vala in trunk could be incompatible with this
branch. In this branch "return base.key_
in the switch - it is replaced with "break;"
On 10 September 2013 19:37, David Gomes <email address hidden> wrote:
> So if you merge this with the latest trunk you can use Escape to hide
> Slingshot?
> --
>
> https:/
> You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
>
--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
Jeremy Wootten (jeremywootten) wrote : | # |
Hello David
Your review of this branch is still "Needs Fixing" - I am not sure what the
outstanding problem is?
On 10 September 2013 19:37, David Gomes <email address hidden> wrote:
> So if you merge this with the latest trunk you can use Escape to hide
> Slingshot?
> --
>
> https:/
> You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
>
--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
if (ifshuaishuai) wrote : | # |
hi,I tested this fix, and i found out it doesn't response to ENTER key, cause public bool on_key_press (Gdk.EventKey event) will never be called when only ENTER pressed
Jeremy Wootten (jeremywootten) wrote : | # |
You are right! I always selected the application with the mouse so did not
notice this. I will look into this.
On 1 October 2013 08:43, if <email address hidden> wrote:
> Review: Needs Fixing
>
> hi,I tested this fix, and i found out it doesn't response to ENTER key,
> cause public bool on_key_press (Gdk.EventKey event) will never be called
> when only ENTER pressed
> --
>
> https:/
> You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
>
--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
if (ifshuaishuai) wrote : | # |
what i did for fixing this, I'm not familiar with vala, is keep the code
handles the KP_Enter remains in the
public override bool key_press_event (Gdk.EventKey event)
{
// only deal with KP_Enter
}
and other code is exactly you fixed. I know it is bad fix, but that seems
work fine.
2013/10/2 Jeremy Wootten <email address hidden>
> You are right! I always selected the application with the mouse so did not
> notice this. I will look into this.
>
>
> On 1 October 2013 08:43, if <email address hidden> wrote:
>
> > Review: Needs Fixing
> >
> > hi,I tested this fix, and i found out it doesn't response to ENTER key,
> > cause public bool on_key_press (Gdk.EventKey event) will never be called
> > when only ENTER pressed
> > --
> >
> >
> https:/
> > You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
> >
>
>
>
> --
> Jeremy Wootten
> GPG Key ID CB585BCD
> Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
>
>
> https:/
> You are reviewing the proposed merge of
> lp:~jeremywootten/slingshot/fix-1213321 into lp:slingshot.
>
Jeremy Wootten (jeremywootten) wrote : | # |
Thanks for this. I have uploaded a different fix already - I dealt with
the Enter key-press by connecting to the activate signal of the searchbar
instead. I am waiting to see whether anyone finds a flaw in this approach
and will bear your alternative solution in mind.
best regards
On 7 October 2013 19:36, if <email address hidden> wrote:
> what i did for fixing this, I'm not familiar with vala, is keep the code
> handles the KP_Enter remains in the
> public override bool key_press_event (Gdk.EventKey event)
> {
>
> // only deal with KP_Enter
>
> }
> and other code is exactly you fixed. I know it is bad fix, but that seems
> work fine.
>
>
> 2013/10/2 Jeremy Wootten <email address hidden>
>
> > You are right! I always selected the application with the mouse so did
> not
> > notice this. I will look into this.
> >
> >
> > On 1 October 2013 08:43, if <email address hidden> wrote:
> >
> > > Review: Needs Fixing
> > >
> > > hi,I tested this fix, and i found out it doesn't response to ENTER key,
> > > cause public bool on_key_press (Gdk.EventKey event) will never be
> called
> > > when only ENTER pressed
> > > --
> > >
> > >
> >
> https:/
> > > You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
> > >
> >
> >
> >
> > --
> > Jeremy Wootten
> > GPG Key ID CB585BCD
> > Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
> >
> >
> >
> https:/
> > You are reviewing the proposed merge of
> > lp:~jeremywootten/slingshot/fix-1213321 into lp:slingshot.
> >
>
> --
>
> https:/
> You are the owner of lp:~jeremywootten/slingshot/fix-1213321.
>
--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD
I see old code is commented out but there's no explanation why. The revision control system already takes care of storing old code, so this is not needed.
I'd very much prefer the code comments to explain why this new code is needed.