Thanks guys - patch applied. On Mon, Feb 26, 2018 at 5:31 PM, Murtuza Zabuawala < murtuza.zabuaw...@enterprisedb.com> wrote:
> 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.zabuaw...@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.zabuaw...@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.zabuaw...@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 >>>>>>> >>>>>>> >>> > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
