Re: Fix for issue RM1336 [pgadmin4]

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(at)pgadmin(dot)org> wrote:

> Hi
>
> On Fri, Jul 1, 2016 at 11:10 AM, Harshal Dhumal
> <harshal(dot)dhumal(at)enterprisedb(dot)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(at)pgadmin(dot)org> wrote:
> >>
> >> Hi Harshal,
> >>
> >> On Thu, Jun 30, 2016 at 7:59 AM, Harshal Dhumal
> >> <harshal(dot)dhumal(at)enterprisedb(dot)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
>