Thank you Joao for reviewing. On Mon, Feb 26, 2018 at 10:43 PM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.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 < >> jdealmeidapere...@pivotal.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 < >>>> jdealmeidapere...@pivotal.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 >>>>>> >>>>>> >>
