Re: Fix for issue RM1336 [pgadmin4] - Mailing list pgadmin-hackers

From Harshal Dhumal
Subject Re: Fix for issue RM1336 [pgadmin4]
Date
Msg-id CAFiP3vypRdQAmRX+uk_4rZVTGYF+mnmq6uOQB9girRiukpHbSQ@mail.gmail.com
Whole thread Raw
In response to Re: Fix for issue RM1336 [pgadmin4]  (Dave Page <dpage@pgadmin.org>)
Responses Re: Fix for issue RM1336 [pgadmin4]  (Dave Page <dpage@pgadmin.org>)
List pgadmin-hackers
Hi,

PFA updated patch for RM1336 and keyboard shortcuts file separately.

This patch is same as last patch except I have removed keyboard shortcut list from this patch.

Also please see my inline response below.

-- 
Harshal Dhumal
Software Engineer

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

On Fri, Jul 1, 2016 at 5:46 PM, Dave Page <dpage@pgadmin.org> wrote:
Hi

On Fri, Jul 1, 2016 at 11:10 AM, Harshal Dhumal
<harshal.dhumal@enterprisedb.com> wrote:
> Hi,
>
> PFA patch
>
> This contains:
> 1] All shortcuts' list which we are using in pgAdmin4.
> 2] Fixed shortcut display tooltips.
>
>
> --
> Harshal Dhumal
> Software Engineer
>
> EnterpriseDB India: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Thu, Jun 30, 2016 at 2:34 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Hi Harshal,
>>
>> On Thu, Jun 30, 2016 at 7:59 AM, Harshal Dhumal
>> <harshal.dhumal@enterprisedb.com> wrote:
>> > Hi Dave,
>> >
>> > Somehow control key is mapped to Command key in Mac. So on Mac shortcuts
>> > are
>> > Shift+Command+E, Shift+Command+X, Shift+Command+A
>>
>> OK, those work, but I think we need to take a step back here (partly
>> because Cmd+Shift+A seems to be Select All in CodeMirror on Mac). I've
>> committed the patch now, but changed Ctrl+Shift+A to Ctrl+Shift+N.
>>
>> Can you please work on the following:
>>
>> 1) Create a table of shortcuts from our runtime, our web app, and
>> CodeMirror. This should list:
>>
>> Component (e.g. runtime, CodeMirror, Query Tool)
>> Standard shortcut (e.g. Ctrl+Shift+A)
>> Mac shortcut (e.g. Cmd+Shift+A)
>> Function (e.g. Explain Analyze or Select All)
>>
>> Let's make this as complete and accurate as possible, so it can be
>> included in the documentation, and used by us to select or
>> de-duplicate shortcuts.
>
> Added shortcuts' list.

Let's keep it as a separate file for now, and not part of the source
tree or any patches.

I notice that it's missing much of what I wanted to collect though, e.g.

Ctrl+Space - Autocomplete(?)
Cut
Copy
Paste
Select All
...
 
The other CodeMirror keys can be found at
https://codemirror.net/doc/manual.html#keymaps. Please add at least
the basic editting commands.
Yes I have added basic codemirror shortcuts in file.

 

>> 2) Confirm that the shortcuts we're using in our runtime and web
>> application don't conflict with any in CodeMirror (or web browsers, in
>> the case of the web app).
>
> Tested. No other conflicting shortcuts found.

Good.

>>
>> 3) Update the web application so the shortcuts are correctly displayed
>> on Mac automatically - e.g. the tooltips and menus should show
>> Cmd+Shift not Ctrl+Shift
>
> Fixed
>
>>
>>
>> 4) Investigate #1360, and ensure that the CodeMirror shortcuts work
>> consistently between the runtime and browsers on all platforms.
>
> Investigating now.

You've already found one inconsistency - Cmd vs. Ctrl on Mac browsers
vs. the runtime. It should be Cmd in either UI, to avoid user
confusion.
Yes, UI will now show Ctrl or Cmd depending on os platform and application mode (web/runtime). (This was also there in last patch)

 
>>
>> 5) Investigate any remaining shortcuts that don't work as expected.
>
>
> Investigating now.: Codemirror shortcut (Cmd/Ctrl+Shift+A)  "Select all"
> only works on Mac (web/runtime) not on linux platform (haven't tested on
> windows)
"Select all" was working on linux as well. It was my mistake. I have added shortcut key details in keyboard shortcut file.

 

Yeah - see also some of the comments I made on
https://redmine.postgresql.org/issues/1360

I spend some time debugging this but didn't get what's going wrong here.
 
Let's figure out what's wrong first, then come up with a set of
fixes/changes that will get us to a consistent set of keys that work
as per the list you're preparing.

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

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

Attachment

pgadmin-hackers by date:

Previous
From: Dave Page
Date:
Subject: Re: pgAdmin IV API test cases patch
Next
From: Harshal Dhumal
Date:
Subject: Re: Re: [pgAdmin 4 - Bug #1292] ERROR: template database "!@#$%^&*()_+{}|:"<>?=-\\][';/.," does not exist message throws if template database contain special charterers