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

Hi

On Mon, Jul 4, 2016 at 3:00 PM, Harshal Dhumal
<harshal.dhumal@enterprisedb.com> wrote:
> 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.

Thanks.

>> >> 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)

You are missing my point I think - the shortcuts should not be
different in the runtime; if they are, they need to be fixed. If I'm
on a Mac, the shortcuts should be the same in both the browser and
runtime for example.

>> >> 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.

OK.

>> 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.

I think the point is that you need to verify that the set of shortcuts
in your text file work correct in both browsers and the runtime, on
Windows, Mac and Linux - and if not, which ones are broken, and how do
we fix them.

The ultimate aim is to ensure we have consistent, working shortcuts
(with only Ctrl vs. Cmd being a different on Macs).

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

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