Signal handler. Please review

[anatoly techtonik]

Hi,

I've fixed a long annoyance in Spyder - implemented "find next" on Enter. This is my first experience with Qt signal/event processing, so I'll appreciate if somebody could review this change and tell if it could be done better.

http://code.google.com/p/spyderlib/source/detail?r=9caff1b29c1499d11c90451442feec65be9e1795

In particular, I am interested if it is possible to pass parameters to signal handler.

Thanks for the feedback.

[Carlos Córdoba]

This is really great! I was trying to add it the other day but totally failed. It was very needed cause it makes the widget so much easier to use.

Unfortunately I can't help you with the signal/event review but from the little I know they seem fine. I just have one question: why was not it possible to add Enter as a keyboard shortcut to the widget here?

http://code.google.com/p/spyderlib/source/browse/spyderlib/widgets/findreplace.py?spec=svn9caff1b29c1499d11c90451442feec65be9e1795&r=9caff1b29c1499d11c90451442feec65be9e1795#143

(I say add it, because F3 has to continue working as usual). I mean, ¿why Enter has to be registered and processed as a keyboard event? I'm just asking because it seems a bit more complicated than adding a shortcut.

Anyway, great work on your side.

Cheers,
Carlos

El 04/05/12 02:54, anatoly techtonik escribió:

--
You received this message because you are subscribed to the Google Groups "spyder" group.
To view this discussion on the web visit https://groups.google.com/d/msg/spyderlib/-/-ToQ_i5iSjcJ.
To post to this group, send email to spyd...@googlegroups.com.
To unsubscribe from this group, send email to spyderlib+...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/spyderlib?hl=en.

[anatoly techtonik]

On Fri, May 4, 2012 at 3:54 PM, Carlos Córdoba <ccord...@gmail.com> wrote:
> This is really great! I was trying to add it the other day but totally
> failed. It was very needed cause it makes the widget so much easier to use.
>
> Unfortunately I can't help you with the signal/event review but from the
> little I know they seem fine. I just have one question: why was not it
> possible to add Enter as a keyboard shortcut to the widget here?
>
> http://code.google.com/p/spyderlib/source/browse/spyderlib/widgets/findreplace.py?spec=svn9caff1b29c1499d11c90451442feec65be9e1795&r=9caff1b29c1499d11c90451442feec65be9e1795#143
>
> (I say add it, because F3 has to continue working as usual). I mean, ¿why
> Enter has to be registered and processed as a keyboard event? I'm just
> asking because it seems a bit more complicated than adding a shortcut.

I understood that these shortcuts are global - even if the panel is
inactive, they still work. I needed Enter shortcut processed only when
widget is active. More than than - this shortcut is only active in
ComboBox widget. It was also possible to override keyPressEvent() in
parent FindReplace() widget, but then Enter will work as find_next()
when focus was on all other elements too.

I thought writing a tutorial of resolving this particular bug into
Spyder blog as an example of reverse engineering for new people, but
quickly realized that I've run out of time. =)

> Anyway, great work on your side.

Thanks. ;)
--
anatoly t.

[Carlos Córdoba]

Please read my answers below

El 04/05/12 08:35, anatoly techtonik escribió:

> On Fri, May 4, 2012 at 3:54 PM, Carlos Córdoba<ccord...@gmail.com> wrote:
>> This is really great! I was trying to add it the other day but totally
>> failed. It was very needed cause it makes the widget so much easier to use.
>>
>> Unfortunately I can't help you with the signal/event review but from the
>> little I know they seem fine. I just have one question: why was not it
>> possible to add Enter as a keyboard shortcut to the widget here?
>>
>> http://code.google.com/p/spyderlib/source/browse/spyderlib/widgets/findreplace.py?spec=svn9caff1b29c1499d11c90451442feec65be9e1795&r=9caff1b29c1499d11c90451442feec65be9e1795#143
>>
>> (I say add it, because F3 has to continue working as usual). I mean, ¿why
>> Enter has to be registered and processed as a keyboard event? I'm just
>> asking because it seems a bit more complicated than adding a shortcut.
> I understood that these shortcuts are global - even if the panel is
> inactive, they still work. I needed Enter shortcut processed only when
> widget is active. More than than - this shortcut is only active in
> ComboBox widget.

I tried it and you're totally right here. I understand know you solution
and think it's the right approach, thanks for the explanation.

> It was also possible to override keyPressEvent() in
> parent FindReplace() widget, but then Enter will work as find_next()
> when focus was on all other elements too.
>
> I thought writing a tutorial of resolving this particular bug into
> Spyder blog as an example of reverse engineering for new people, but
> quickly realized that I've run out of time. =)

If you finally find the time, it would be a great reading for the other
devs and future contributors :)

Cheers,
Carlos

[Jed Ludlow]

Looks great. You essentially did the same thing that Pierre had done in PatternComboBox, which is override keyPressEvent, so if there are complaints now there should have been complaints earlier :). It's the right way to go to accomplish what you intended to do as far as I know. I had just one question, which I attached to a code review here:

http://code.google.com/p/spyderlib/source/detail?r=9caff1b29c1499d11c90451442feec65be9e1795&url_prefix=p