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

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