Re: PATCH: Enable backwards search in SQL Box

From: Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: "J(dot)F(dot) Oster" <jinfroster(at)mail(dot)ru>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: PATCH: Enable backwards search in SQL Box
Date: 2014-03-13 11:50:40
Message-ID: CANxoLDdk_rJiatTjmbt61m135PcQusoVQUzuAk1iO8hcfFFMKA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

Hi

On Mon, Mar 10, 2014 at 3:13 PM, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com
> wrote:

>
>
>
> On Mon, Mar 10, 2014 at 3:10 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> On Sat, Mar 8, 2014 at 5:15 PM, J.F. Oster <jinfroster(at)mail(dot)ru> wrote:
>> > Hello Dave,
>> >
>> > Wednesday, February 26, 2014, 12:56:51 AM, you wrote:
>> >
>> > DP> Shift-F3 should always do the opposite of what the radio
>> > DP> buttons are set to. So, by default with the Forward radio button
>> > DP> selected, Shift-F3 will search backwards, whilst if the Backwards
>> > DP> radio button is selected, Shift-F3 should search forwards.
>> >
>> > You are right. For example, SciTE does the same.
>> >
>> > According to the above implemented it in a simplier way.
>> > Please see the patch.
>>
>> Thanks. Akshay, can you take another look please?
>>
>
> Sure.
>

I have reviewed and tested the patch. Working fine and code looks
good to me.

>
>> > By the way, I was puzzled why search dialog's settings are saved
>> > sometimes and sometimes are not. Depending on dialog's close method:
>> > 1. Pressing ESC or "Close" form button calls OnCancel() which does not
>> > save;
>> > 2. Pressing title [x] button or Alt-F4 (or similar OS hotkey) calls
>> > OnClose() which does save.
>> >
>> > Shouldn't the behaviour be the same regardless of dialog's close
>> > method? Or at least the difference should be noted in documentation.
>>
>> Yes, it should. Patches welcome :-)
>>
>> > Wednesday, February 26, 2014, 12:56:51 AM, you wrote:
>> >
>> > DP> On Tue, Feb 25, 2014 at 9:38 AM, Akshay Joshi
>> > DP> <akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>> > DP> Hi Dave═
>> >
>> >
>> > DP> Ashesh asked me to review the patch. I have reviewed the
>> > DP> patch and tested it. Found one behavior which gets change after
>> > DP> applying the patch.
>> >
>> >
>> > DP> Behavior Before:- When search direction is forward and user
>> > DP> press F3, it searches the string in forward direction. Similarly
>> > DP> if search direction is backward and user press F3, it searches the
>> > DP> string in the backward direction. Nothing will be happen when user
>> > DP> press═Shift-F3.═
>> >
>> >
>> > DP> Behavior After :- ═When user press F3, it searches the string
>> > DP> in forward direction while on pressing Shift-F3, it searches the
>> > DP> string in backward direction. It is not dependent on the search
>> > DP> direction selected from the dialog. Also the selection of radio
>> > DP> button is dependent on the key press, if F3 is pressed then
>> > DP> forward button is selected and if Shift-F3 is pressed then
>> > DP> backward is selected.═
>> >
>> >
>> > DP> Is this behavior acceptable?═
>> >
>> >
>> >
>> >
>> >
>> > DP> No, I don't think so (if I'm understanding correctly). I
>> > DP> believe what should happen is that:
>> >
>> >
>> > DP> - F3 should honour the search direction on the radio buttons,
>> > DP> in all cases. If the Forward button is selected, F3 searches
>> > DP> forward, and if the Backwards button is selected, F3 searches
>> > DP> backwards.
>> >
>> >
>> > DP> - Shift-F3 should always do the opposite of what the radio
>> > DP> buttons are set to. So, by default with the Forward radio button
>> > DP> selected, Shift-F3 will search backwards, whilst if the Backwards
>> > DP> radio button is selected, Shift-F3 should search forwards.
>> >
>> >
>> > DP> The reason for this is that the radio buttons give the most
>> > DP> obvious visual cue, so F3 (Find Next) should follow what they are
>> > DP> set to.
>> > DP> ═
>> >
>> >
>> > DP> On Thu, Feb 20, 2014 at 5:09 PM, Ashesh Vashi
>> > DP> <ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>> > DP> Sure.
>> > DP> I will do that.
>> >
>> >
>> >
>> > DP> On Thu, Feb 20, 2014 at 4:52 PM, Dave Page <dpage(at)pgadmin(dot)org>
>> wrote:
>> > DP> Ashesh, can you look at, and if appropriate, commit this please?
>> >
>> > DP> Thanks.
>> >
>> > DP> On Mon, Feb 17, 2014 at 5:46 PM, J.F. Oster <jinfroster(at)mail(dot)ru>
>> wrote:
>> >>> Hello!
>> >>>
>> >>> Pressing F3 in query editor makes the previously started search
>> >>> continue forwards.
>> >>> This patch adds expected behavior for Shift-F3.
>> >>>
>> >>> There is a minor inconvenience when just changing the search direction
>> >>> - you have to press (Shift-)F3 *twice* for the (previous)next
>> occurence
>> >>> to get highlighted. That's because first time goes for cursor movement
>> >>> to the corresponding boundary of current occurence. But that also
>> >>> happens when search direction is changed with dialog radio-buttons, so
>> >>> I think it's ok.
>> >>>
>> >
>> >
>> > --
>> > Best regards,
>> > J.F.
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
>
> --
> *Akshay Joshi*
> *Principal Software Engineer *
>
>
>
> *Phone: +91 20-3058-9517 Mobile: +91 976-788-8246*
>

--
*Akshay Joshi*
*Principal Software Engineer *

*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Guillaume Lelarge 2014-03-13 13:50:49 Re: New functionality - Table tree view
Previous Message Ashesh Vashi 2014-03-13 09:55:51 Re: New functionality - Table tree view