MIME-Version: 1.0
In-Reply-To: 
 <CAM5-9D_2h3uTOPagHNTXAFmgK_UmNjUJjc_j4zhS1NayccvEQg@mail.gmail.com>
References: 
 <CAM5-9D_k4UbuHWku7rKX-5TJ4sS=AbRy6-z5O2gfDz4zLoQ=2g@mail.gmail.com>
 <CA+OCxoyeh-fidUJBA2g8AoeOmh6vqZO4hYBwL-jpzAjEapOK9w@mail.gmail.com>
 <CAM5-9D8JPAErQPFe7i4bVT1FWZVHheUmCkUGCaVJxkHUbj1Fjg@mail.gmail.com>
 <CAM5-9D8=SR_EbapHfDGbHh=FNkszWswmh51FD8-Z0CuAn+zcyg@mail.gmail.com>
 <CAM5-9D_2h3uTOPagHNTXAFmgK_UmNjUJjc_j4zhS1NayccvEQg@mail.gmail.com>
From: Dave Page <dpage@pgadmin.org>
Date: Mon, 1 Aug 2016 15:12:36 +0100
Message-ID: 
 <CA+OCxoxQrNzdVhSwPFf9io1ybz+=uNc5sjGx+=HXHH54-eh+kA@mail.gmail.com>
Subject: Re: [pgAdmin4][Patch]: Using F5 for Query Execution in Query tool
To: Surinder Kumar <surinder.kumar@enterprisedb.com>
Cc: pgadmin-hackers <pgadmin-hackers@postgresql.org>,
	Ashesh Vashi <ashesh.vashi@enterprisedb.com>,
	Harshal Dhumal <harshal.dhumal@enterprisedb.com>
Content-Type: text/plain; charset=UTF-8
Precedence: bulk
Sender: pgadmin-hackers-owner@postgresql.org

Thanks - applied with a couple of fixes:

- The menu option for Explain Analyze showed the shortcut as F8, not Shift+F7

- Explain Analyze was broken - the patch checked for Shift+F7 and
called on_explain_analyze, but then also checked for F7 and called
on_explain.

On Wed, Jul 27, 2016 at 6:09 PM, Surinder Kumar
<surinder.kumar@enterprisedb.com> wrote:
> Hi
>
> Please find attached patch with following fixes:
>
> 1) This patch adds support for Function Keys(F5, F7, F8) as keyboard
> shortcuts.
> Following are the keyboard shortcuts for query tool operations such as:
>
> Execute query --> F5
> Explain query --> F7
> Explain analyze query --> Shift + F7
> Download query result as CSV --> F8
>
> 2) Add proper condition to check string in JS array in explain analyze. The
> code breaks while running explain analyse.
>
> This patch partially resolves RM1478.
>
> Please review.
>
> On Wed, Jul 27, 2016 at 5:48 PM, Surinder Kumar
> <surinder.kumar@enterprisedb.com> wrote:
>>
>> On Wed, Jul 27, 2016 at 2:31 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> >
>>
>> > Hi
>> >
>> > On Tue, Jul 26, 2016 at 7:13 PM, Surinder Kumar
>> > <surinder.kumar@enterprisedb.com> wrote:
>> > > Hi Dave
>> > >
>> > > I spend some time looking at RM#1412 - Function Keys in Query Tool
>> > > specific
>> > > to execute query using F5 key.
>> >
>> > Please note that RM1412 was rejected. 1478 is where we're tracking all
>> > the shortcut issues now.
>> >
>> > > As we know F5 key is bound to reload/refresh for browsers on Linux &
>> > > Windows.
>> > > But If we use F5 key to execute query, then F5 key event is first
>> > > captured
>> > > inside the code and execution works.
>> >
>> > OK.
>> >
>> > > It will not affect the browser reload behaviour. F5 key for execution
>> > > will
>> > > only work if focus is on Query tool.
>> > >
>> > > Ashesh's concern is:
>> > > If focus is not on query tool and user mistakenly pressed Fn+F5 key,
>> > > the
>> > > page will reload which shouldn't happened.
>> > >
>> > > In such case:
>> > > The page doesn't reload directly as we always ask user "Whether to
>> > > Stay on
>> > > page or Leave page".
>> >
>> > Right.
>> >
>> > > The other way is to disable reload page on F5 Key only when focus is
>> > > inside
>> > > the app.
>> > > If focus is on browser then F5 for reload will work. (not a good idea)
>> > >
>> > > @Dave What do you think?
>> >
>> > On Mac it works nicely, both in the runtime and in Chrome, Safari and
>> > Firefox - and given the number of people complaining about the
>> > shortcuts here, I think the limitation of focus on the text box is
>> > acceptable.
>> >
>> > > I have tested this attached patch on following platforms without any
>> > > issue:
>> > > Windows 7 - Runtime & IE 9 & 10.
>> > > Mac OSX Yosemite - Safari(8.0.3), Chrome(51) & Firefox(47)
>> > > Ubuntu-14.04.2 - Chrome & Firefox.
>> > >
>> > > Please review the patch.
>> >
>> > The patch doesn't update the tooltips/buttons/menus to display the
>> > correct shortcuts - plus, if we're doing this for F5, then I think we
>> > should also keep F7 for Explain, Shift+F7 for Explain Analyse and F8
>> > for Execute to File (CSV Downlaod), like pgAdmin 3.
>
> Tooltips/buttons/menus are updated with correct shortcuts keys in attached
> patch.
>>
>> I will send a patch later today.
>>
>>
>> >
>> >
>> > Please note though that that would only partially resolve RM1478. On
>> > some browsers/platforms, there is also some CodeMirror related
>> > shortcut weirdness that needs to be resolved. It may be worth seeing
>> > if a CodeMirror update would help.
>> Okay
>> >
>> > Thanks!
>> >
>> > --
>> > Dave Page
>> > Blog: http://pgsnake.blogspot.com
>> > Twitter: @pgsnake
>> >
>> > EnterpriseDB UK: http://www.enterprisedb.com
>> > The Enterprise PostgreSQL Company
>
>


-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers