Re: [pgAdmin4][RM#2900] Adding accessibility features in query tool

Hello Murtuza,
We just passed the patch through our CI pipeline and all tests pass. The
patch looks good.

Sorry that the patch I sent didn't help. Nevertheless when I finish the
tasks that we want to try to get accepted for pgAdmin4 3.0 release I will
try to change the way the preferences are retrieved, because at this point
in time they work because we only ask for them after some time we try to
get them, or else the code does not work.
Unless someone want to give it a shot first. From my point of view this
functionality should work like a separate service that we include where we
need it and work with a promise system. There are some nice article about
promises and how to handle them instead of using event bouncing all around:
https://medium.com/dev-bits/writing-neat-asynchronous-node-js-code-with-promises-32ed3a4fd098

https://developers.google.com/web/fundamentals/primers/promises (this is
quite a complete guide on promises, and how to think about them)

Thanks
Joao

On Mon, Feb 26, 2018 at 4:58 AM Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> Hi Joao,
>
> I tried the solution/patch you suggested but it is not working for me, I
> always get *undefined* for preferences values.
>
> Thanks,
> Murtuza
>
> On Thu, Feb 22, 2018 at 3:17 AM, Joao De Almeida Pereira <
> jdealmeidapereira(at)pivotal(dot)io> wrote:
>
>> Hello Murtuza,
>> I created a small patch on the way I believe the function should work.
>> Not 100% sure why we are piggybacking on the window.top, window.opener and
>> so on to give us information. Nowadays I do not think that loading the JS
>> files again costs that much in terms of time.
>>
>> The issue I came across was that cache_preferences/get_preference behave
>> in a very strange way.
>>
>> Example:
>> If the cache is populated no problem it returns a value and it is fine
>> If the cache is not populated it sets a timeout that will check in 1
>> second if is the cache is populated if it is not then does nothing, if it
>> is then returns the value.
>> The problem is that it returns the value to no place.
>>
>> This caching implementation works until this point because for some good
>> fortune we never call get_preference before we have the cache full.
>> Is my assessment correct?
>>
>> Nevertheless if the caching was using promises we could avoid the problem
>> above.
>>
>> Thanks
>> Joao
>>
>> On Wed, Feb 21, 2018 at 12:50 PM Murtuza Zabuawala <
>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Joao,
>>>
>>> Thank you for your time in reviewing the patch.
>>>
>>>
>>> On Wed, Feb 21, 2018 at 9:11 PM, Joao De Almeida Pereira <
>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>
>>>> Hello Murtuza,
>>>> After reviewing the code I have some suggestions:
>>>> - We should split the PEP and the features into different patches, or
>>>> else it becomes very hard to separate Feature code from Code Style change
>>>>
>>> ​I'll keep that in mind next time onwards.
>>>
>>>> - The function: `getTextRepresentaionShortcut` has a typo
>>>>
>>> ​I'll fix​
>>>
>>> ​that.​
>>>
>>>> - As a personal point I have a hard time reading multiple declarations
>>>> of variables in javascript under a single `var`, I prefer 1 `let` per
>>>> variable
>>>>
>>> - I think we should use cameCase for our variables in Javascript code
>>>> - What is the reason behind the move of the key shortcuts back into
>>>> the SQLEditorController? This look like something that could be extracted
>>>> from SQLEditor file into its own
>>>>
>>> I'll try to move that code.​
>>>
>>>
>>>> - Looks like we are missing some test coverage on the new implemented
>>>> parts
>>>>
>>> ​I didn't added any new code as such, I just moved out preferences code
>>> out from main file.
>>>
>>>> - The getKeyboardShortcuts function is retrieving information from
>>>> window.top and window.opener I think that we can come up with a better
>>>> pattern then this. Relying on window information doesn't look very good.
>>>> That was the pattern for JQuery that the new frameworks are trying to avoid
>>>> because polluting the global scope is almost always a Code Smell.
>>>>
>>> ​What do you suggest on this?​
>>>
>>>
>>>>
>>>>
>>>> I see there was some refactoring on the backend with the creation of
>>>> RegisterQueryToolPreferences and it is looking good, hope that we can do
>>>> this in more places specially in the front-end.
>>>>
>>>> Using the patch that you sent I passed it through our CI and everything
>>>> is Passing.
>>>>
>>>> Thanks
>>>> Joao
>>>>
>>>> On Tue, Feb 20, 2018 at 1:13 PM Murtuza Zabuawala <
>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> PFA patch for adding accessibility features in query tool.
>>>>> RM#2900
>>>>>
>>>>> Please review.
>>>>>
>>>>> --
>>>>> Regards,
>>>>> Murtuza Zabuawala
>>>>> EnterpriseDB: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>>
>