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

Thank you Joao for reviewing.

On Mon, Feb 26, 2018 at 10:43 PM, Joao De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:

> 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)
>
> Sure, I'll go through the articles you provided.

Thanks,
Murtuza

> Thanks
> Joao
>
> On Mon, Feb 26, 2018 at 4:58 AM Murtuza Zabuawala <murtuza.zabuawala@
> enterprisedb.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.zabuawala@
>>> enterprisedb.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.zabuawala@
>>>>> enterprisedb.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
>>>>>>
>>>>>>
>>